This repository has been archived by the owner on Apr 22, 2023. It is now read-only.
v8: don't busy loop in cpu profiler thread #9439
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The change in v0.10 (issue 8789) to sleep instead of busy looping in the profiler processing thread is not effective in v0.12 due to alterations in V8 to the main loop in the processing thread. As part of this V8 change, the responsibility for taking stack samples been added to this loop to allow for the sampling interval to be configured via the API. This resulted in removing the previously present call to YieldCPU(). The purpose of this commit is to recreate a similar fix in v0.12 to the one provided for v0.10 in issue 8789. The call to YieldCPU is reintroduced to the loop in the case where there is no work to perform. Since YieldCPU was previously modified to nanosleep (rather than sched_yield) on posix platforms, and that change has been carried forward to 0.12, this will result in a sleep for a period rounding up to the scheduler's granularity on posix. In v0.12 this could delay samples by the sleep period which would be accurate on the unpatched code (since the loop now manages the time at which samples are taken).
|
Other relevant thing I noticed is that the original fix for issue #8789 on 0.10 did not alter the behaviour on Windows, which would still yield instead of sleeping. For 0.12 where none of the platforms are performing a yield or a sleep, this fix introduces a sleep on POSIX platforms and a yield on Windows. |
|
A change (https://codereview.chromium.org/1118533003) has been landed to V8 master to address https://code.google.com/p/v8/issues/detail?id=3967. I will close this pull request and raise a new issue to backport https://codereview.chromium.org/1118533003. Let me know if there's a problem with that! Thanks. |
|
I have raised issue #25137 to address backporting https://codereview.chromium.org/1118533003. |
tunniclm
added a commit
to tunniclm/node-v0.x-archive
that referenced
this pull request
May 8, 2015
Backport 6964a9e0685fa186d9d9b7907be17505e839db1a from upstream v8. Original commit message: Make CPU profiler do not hog 100% of CPU. Tick event processor should not stay in a tight loop when there's nothing to do. It can go sleep until next sample event. LOG=N BUG=v8:3967 Committed: https://crrev.com/6964a9e0685fa186d9d9b7907be17505e839db1a Cr-Commit-Position: refs/heads/master@{#28211} Fixes nodejs#25137 Related: nodejs#9439, nodejs#8789
misterdjules
pushed a commit
to misterdjules/node
that referenced
this pull request
May 12, 2015
Backport 6964a9e0685fa186d9d9b7907be17505e839db1a from upstream v8. Original commit message: Make CPU profiler do not hog 100% of CPU. Tick event processor should not stay in a tight loop when there's nothing to do. It can go sleep until next sample event. LOG=N BUG=v8:3967 Committed: https://crrev.com/6964a9e0685fa186d9d9b7907be17505e839db1a Cr-Commit-Position: refs/heads/master@{#28211} Fixes nodejs#25137 Related: nodejs#9439, nodejs#8789 PR: nodejs#25268 PR-URL: nodejs#25268 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Julien Gilli <jgilli@fastmail.fm>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The change in v0.10 (issue #8789) to sleep instead of
busy looping in the profiler processing thread is not
effective in v0.12 due to alterations in V8 to the
main loop in the processing thread.
As part of this V8 change, the responsibility for taking
stack samples been added to this loop to allow for the
sampling interval to be configured via the API.
This resulted in removing the previously present call to
YieldCPU().
The purpose of this commit is to recreate a similar fix in
v0.12 to the one provided for v0.10 in issue #8789.
The call to YieldCPU is reintroduced to the loop in the case
where there is no work to perform. Since YieldCPU was previously
modified to nanosleep (rather than sched_yield) on posix platforms,
and that change has been carried forward to 0.12, this will result
in a sleep for a period rounding up to the scheduler's granularity
on posix.
In v0.12 this could delay samples by the sleep period which would
be accurate on the unpatched code (since the loop now manages the
time at which samples are taken).
Additional:
This change (like issue #8789) is a patch to deps/v8 rather than node itself.
My understanding is that this was accepted previously because the v8 version (3.14) used by node (in 0.10) was significantly backlevel that it would not be patched upstream. I assume this is the case with 0.12 as well (which uses V8 3.28).
I have raised a V8 issue to address the problem on the latest V8 level (https://code.google.com/p/v8/issues/detail?id=3967).