Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[v12.x backport] v8: implement v8.takeCoverage() and v8.stopCoverage() #36352

Merged
merged 2 commits into from
Mar 17, 2021

Conversation

rickyes
Copy link
Contributor

@rickyes rickyes commented Dec 2, 2020

Checklist

PR-URL: #33807

Reviewed-By: @gengjiawen
Reviewed-By: @bcoe
Reviewed-By: @bnoordhuis
Reviewed-By: @addaleax

  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. v12.x labels Dec 2, 2020
@rickyes
Copy link
Contributor Author

rickyes commented Dec 2, 2020

/cc @joyeecheung

@rickyes rickyes added semver-minor PRs that contain new features and should be released in the next minor version. coverage Issues and PRs related to native coverage support. request-ci Add this label to start a Jenkins CI on a PR. labels Dec 2, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 2, 2020
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Dec 2, 2020

@bcoe
Copy link
Contributor

bcoe commented Dec 18, 2020

@Trott I'm not sure what the back-porting process entails, perhaps you could give a hand here?

@Trott
Copy link
Member

Trott commented Dec 24, 2020

@Trott I'm not sure what the back-porting process entails, perhaps you could give a hand here?

Because 12.x is LTS, I believe only people in @nodejs/releasers should land these kinds of PRs. 12.x is Maintenance and so I think semver-minor PRs like this one typically don't land there, but I could be wrong. So they may need you or someone else to make the case for landing it.

@MylesBorins
Copy link
Contributor

12.x is now in maintenance and we are unlikely to do another semver minor release

@rickyes
Copy link
Contributor Author

rickyes commented Jan 14, 2021

Maybe we can make another semver minor release? @nodejs/releasers may have to ask for your help.

@atian25
Copy link
Contributor

atian25 commented Jan 14, 2021

12.x is now in maintenance and we are unlikely to do another semver minor release

This PR was proposed when it's still at the LTS phase, but has been waiting for the Review to land and it is no longer LTS.

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

Add an v8.takeCoverage() API that allows the user to write the
coverage started by NODE_V8_COVERAGE to disk on demand.
The coverage can be written multiple times during the lifetime
of the process, each time the execution counter will be reset.
When the process is about to exit, one last coverage will
still be written to disk.

Also refactors the internal profiler connection code
so that we use the inspector response id to identify
the profile response instead of using an ad-hoc flag in C++.

PR-URL: nodejs#33807
Backport-PR-URL: nodejs#36352
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Ben Coe <[email protected]>
Add a v8.stopCoverage() API to stop the coverage collection
started by NODE_V8_COVERAGE - this would be useful in
conjunction with v8.takeCoverage() if the user don't want
to emit the coverage at the process exit but still want
to collect it on demand at some point.

PR-URL: nodejs#33807
Backport-PR-URL: nodejs#36352
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Ben Coe <[email protected]>
@richardlau
Copy link
Member

Landed in d7a4ccd...86f34ee

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. coverage Issues and PRs related to native coverage support. lib / src Issues and PRs related to general changes in the lib or src directory. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants