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

deps: cherry-pick 15c0c3a8ba and 43e2fb1c3d from upstream V8 #19200

Closed
wants to merge 1 commit into from
Closed

deps: cherry-pick 15c0c3a8ba and 43e2fb1c3d from upstream V8 #19200

wants to merge 1 commit into from

Conversation

Flarna
Copy link
Member

@Flarna Flarna commented Mar 7, 2018

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}

Refs: nodejs/diagnostics#170
Refs: v8/v8@15c0c3a
Refs: v8/v8@43e2fb1

Checklist

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. v8 engine Issues and PRs related to the V8 dependency. labels Mar 7, 2018
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}

```

Refs: nodejs/diagnostics#170
Refs: v8/v8@15c0c3a
Refs: v8/v8@43e2fb1
@targos
Copy link
Member

targos commented Mar 7, 2018

@nodejs/v8 Could they be backported upstream?

@ofrobots
Copy link
Contributor

ofrobots commented Mar 7, 2018

Opened upstream merge request bug (https://bugs.chromium.org/p/v8/issues/detail?id=7535) as per guide.

@Flarna
Copy link
Member Author

Flarna commented Mar 7, 2018

Grrr, seems I have read the wrong guideline...

@ofrobots Thanks for creating the upstream merge request!

@targos commit 15c0c3a8ba is included in 6.6. but 43e2fb1c3d not therefore V8 team should really merge 43e2fb1c3d at least to 6.6.
If I understand #19201 correct it's planned to use 6.6 (or newer) in Node v10. Therefore backport to 6.6. should be enough; no need to have this in master already now via 6.5 or a cherry pick.

nodejs/diagnostics#170 (comment) indicated to me that backports will be not done therefore I created this PR.

If I understood the v8 maintenance guide correct it's needed to create separate backport PRs for Node 9, 8, 6 and 4. But I assume this should wait until this PR is closed.

@ofrobots
Copy link
Contributor

ofrobots commented Mar 7, 2018

If I understood the v8 maintenance guide correct it's needed to create separate backport PRs for Node 9, 8, 6 and 4. But I assume this should wait until this PR is closed.

Let's wait to see what merges get approved upstream. This PR may be needed yet.

If the fixes are easy to cherry-pick for the previous node versions, then a separate backport may not even need to be opened, and the release team would take care of back-porting.

@ofrobots
Copy link
Contributor

ofrobots commented Mar 7, 2018

If I understand #19201 correct it's planned to use 6.6 (or newer) in Node v10. Therefore backport to 6.6. should be enough; no need to have this in master already now via 6.5 or a cherry pick.

Until #19201 lands, let's assume that we need the merges to V8 6.5 as well.

@Flarna
Copy link
Member Author

Flarna commented Mar 13, 2018

I have seen that above v8 fix is available via 6.5.254.37 (and 6.6.346.10) so it seems this cherry-pick PR is not needed.
But I'm not sure how to proceed to get new V8 into master + cherry-pick fix to older nodejs version.
Should I simply create PRs for this or is this done "automagically" somehow via some other mechanism I don't know?

@ofrobots
Copy link
Contributor

Hi @Flarna. You should retarget this PR to Node 9. The release team will take care of back-porting and releasing on older versions beyond that. If the cherry-pick does not land cleanly on older versions, then they will ping you to request a manual back-port, as necessary.

@Flarna
Copy link
Member Author

Flarna commented Mar 13, 2018

@ofrobots ok thanks. I will do a separate PR for Node 9.
V8 updates on master seem to happen anyway (e.g. #19303).

@Flarna Flarna closed this Mar 13, 2018
@Flarna Flarna deleted the deps_v8_nobusywaitonwin branch March 13, 2018 22:18
MylesBorins pushed a commit that referenced this pull request Mar 20, 2018
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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants