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.

Sign up for GitHub

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

Jump to bottom

V8's CpuProfiler uses busy wait on Windows. #170

Closed
hashseed opened this issue Feb 22, 2018 · 4 comments
Closed

V8's CpuProfiler uses busy wait on Windows. #170

hashseed opened this issue Feb 22, 2018 · 4 comments

Comments

@hashseed
Copy link
Member

The reason for that is described in this comment:

// Do not use Sleep on Windows as it is very imprecise.
// Could be up to 16ms jitter, which is unacceptable for the purpose.

This in turn is not acceptable for some APM vendors who run the CpuProfiler in production. For sampling intervals significantly larger than 16ms we should be fine with using Sleep though.

@Hollerberg
@hashseed
Copy link
Member Author

Upstream fix is in flight: https://chromium-review.googlesource.com/c/v8/v8/+/931102

@hashseed
Copy link
Member Author

@Flarna @Hollerberg feel free to have this cherry-picked onto Node.js. I don't think we will backmerge this change in V8 upstream.

kisg pushed a commit to paul99/v8mips that referenced this issue Feb 22, 2018
@hashseed
[profiler] use Sleep() on windows for long profile intervals.
15c0c3a 
See nodejs/diagnostics#170

[email protected]

Change-Id: Iecc3bb27707b0d2afbb23fd9823d5cd4d725be6e
Reviewed-on: https://chromium-review.googlesource.com/931102
Reviewed-by: Franziska Hinkelmann <[email protected]>
Commit-Queue: Yang Guo <[email protected]>
Cr-Commit-Position: refs/heads/master@{#51466}
@Flarna
Copy link
Member

Flarna commented Feb 28, 2018

@hashseed Thanks for the change!
But I think there is an else missing in current code. Now there is no wait at all for long intervals...

This was referenced Mar 7, 2018
Closed
Closed
MylesBorins pushed a commit to nodejs/node that referenced this issue Mar 20, 2018
@Flarna @MylesBorins
deps: v8: cherry-pick fixes for v8:7535
5966b8c 
These changes avoid a busy wait loop in V8 CPU Profiler thread for
windows (except for short intervals).

It would be good if this is also backported to Node.js v9 and LTS
releases as this busy loop effectively disallows the use of
cpu-profiler in windows production setups.

Original commit message 15c0c3a8ba:
```
[profiler] use Sleep() on windows for long profile intervals.

See nodejs/diagnostics#170

[email protected]

Change-Id: Iecc3bb27707b0d2afbb23fd9823d5cd4d725be6e
Reviewed-on: https://chromium-review.googlesource.com/931102
Reviewed-by: Franziska Hinkelmann <[email protected]>
Commit-Queue: Yang Guo <[email protected]>
Cr-Commit-Position: refs/heads/master@{#51466}
```

Original commit message 43e2fb1c3d:
```
[profiler] fix sleeping on windows for long intervals.
[email protected]

Change-Id: I5717db794fc797e7c3b0b8f122ddb6dc0702a99e
Reviewed-on: https://chromium-review.googlesource.com/941126
Reviewed-by: Franziska Hinkelmann <[email protected]>
Commit-Queue: Yang Guo <[email protected]>
Cr-Commit-Position: refs/heads/master@{#51755}

```

PR-URL: #19333
Refs: nodejs/diagnostics#170
Refs: #19200
Refs: v8/v8@15c0c3a
Refs: v8/v8@43e2fb1
Reviewed-By: Myles Borins <[email protected]>
@Flarna
Copy link
Member

Flarna commented Jul 9, 2019

Will close this as this is included in NodeJS 10 onwards.

@Flarna Flarna closed this as completed Jul 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@hashseed @Flarna