-
Notifications
You must be signed in to change notification settings - Fork 30k
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
deps: cherry-pick 22858cb from V8 upstream #11998
Conversation
Original commit message: Only flush not yet started and finished compile jobs from gc We shouldn't block during GC for arbitrarily long intervals. BUG=chromium:686153,chromium:642532 R=verwaest@chromium.org,hpayer@chromium.org Review-Url: https://codereview.chromium.org/2658313002 Cr-Commit-Position: refs/heads/master@{nodejs#42761}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. For my own curiosity, the second call to Unblock() when blocking_behavior == BlockingBehavior::kBlock
and --block_concurrent_recompilation
is on could be omitted?
Paging @jeisinger as the original author of the upstream change. |
Original commit message: Only flush not yet started and finished compile jobs from gc We shouldn't block during GC for arbitrarily long intervals. BUG=chromium:686153,chromium:642532 R=verwaest@chromium.org,hpayer@chromium.org Review-Url: https://codereview.chromium.org/2658313002 Cr-Commit-Position: refs/heads/master@{nodejs#42761} PR-URL: nodejs#11998 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Landed in |
@ofrobots should this land on v6.x? |
sorry, didn't see the question before. Yes, the first Unblock() call can be moved into the if () block below. However, blocking concurrent recompilation is a unit test support feature, so it doesn't really matter that much.. |
@MylesBorins no, this is not relevant to 6.x. |
Original commit message:
This is needed on
v7.x
(V8 5.5). It is already merged to 5.6 and 5.7.Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
deps:v8
R=@nodejs/v8
EDIT:
CI: https://ci.nodejs.org/job/node-test-pull-request/6990/
V8-CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/618/