Skip to content
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 - backport #25137

Closed
tunniclm opened this issue May 5, 2015 · 5 comments
Closed

v8: don't busy loop in cpu profiler thread - backport #25137

tunniclm opened this issue May 5, 2015 · 5 comments

Comments

@tunniclm
Copy link

tunniclm commented May 5, 2015

As described in issue #9439, the profiler consumes 100% of a core on 0.12 when enabled via the CpuProfiler API. This was already improved in 0.10 but the fix did not carry forward correctly to 0.12 due to changes in V8.

I submitted a pull request in #9439 for a fix, but a change has now landed in V8 master for this issue and should probably be backported instead.

The V8 change is https://codereview.chromium.org/1118533003, worked under issue https://code.google.com/p/v8/issues/detail?id=3967.

@misterdjules
Copy link

@tunniclm Thank you! Could you please create a PR against the v0.12 branch that backports this change? There are instructions on how to do so in the wiki. If they're incomplete, please feel free to suggest improvements.

Adding this to the 0.12.3 milestone, and closing #9439 in favor of this one.

@misterdjules misterdjules added this to the 0.12.3 milestone May 6, 2015
tunniclm added a commit to tunniclm/node-v0.x-archive that referenced this issue 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
@tunniclm
Copy link
Author

tunniclm commented May 8, 2015

Note that this backport does not address the problem on Windows, where the coarse scheduler granularity makes it more difficult to make the trade-off between CPU usage and precision.

I would like to investigate some alternatives on Windows (eg: using Sleep(0), SwitchToThread(), or a more significant change involving locking)--it is unclear to me what potential scheduling delays would be introduced by each of these approaches, however.

@misterdjules
Copy link

@tunniclm Thank you for backporting the change from upstream. What do you mean by:

Note that this backport does not address the problem on Windows

? Do you mean that on Windows, the profiler still consumes 100%, or that it doesn't but the precision is not as good as before?

EDIT: Just took a look at the patch, and on Windows it is still busy looping, so I guess it still takes 100% of the CPU.

@tunniclm
Copy link
Author

@misterdjules : Yes, on Windows the code busy loops and consumes a core.

misterdjules pushed a commit to misterdjules/node that referenced this issue 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 <[email protected]>
Reviewed-By: Julien Gilli <[email protected]>
@misterdjules
Copy link

Fixed by 80cdae8, thank you @tunniclm 👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants