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

[v9.x backport] deps: cherry-pick 15c0c3a8ba and 43e2fb1c3d from upstream V8 #19333

Closed
wants to merge 1 commit into from
Closed

[v9.x backport] deps: cherry-pick 15c0c3a8ba and 43e2fb1c3d from upstream V8 #19333

wants to merge 1 commit into from

Conversation

Flarna
Copy link
Member

@Flarna Flarna commented Mar 13, 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 CpuProfiler 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: #19200
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. v9.x labels Mar 13, 2018
@MylesBorins
Copy link
Contributor

generally we will cherry pick each of the commits individually, which makes it easier for us to revert if there are issues. I'm not sure if that is set in stone though

/cc @ofrobots @fhinkel

@MylesBorins
Copy link
Contributor

@Flarna
Copy link
Member Author

Flarna commented Mar 14, 2018

Splitting this PR into two PRs doesn't make sense as the second commit fixes a bug introduced with the first. We should never end up in having just one of them.
But I can split this PR into two separate commits if needed.

@MylesBorins
Copy link
Contributor

@Flarna that makes sense

/cc @nodejs/platform-aix re: failure

@richardlau
Copy link
Member

/cc @nodejs/platform-aix re: failure

test-performance was recently moved on master from parallel to sequential (#19228) as it was flaky.

Copy link
Contributor

@MylesBorins MylesBorins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@ofrobots ofrobots left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM w/ nit: the commit message abstract is too long. I would recommend something like:

deps: v8: cherry-pick fixes for v8:7535

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: #19200
Refs: v8/v8@15c0c3a
Refs: v8/v8@43e2fb1
@Flarna
Copy link
Member Author

Flarna commented Mar 16, 2018

I have updated commit messages as suggested by @ofrobots

@MylesBorins MylesBorins force-pushed the v9.x-staging branch 4 times, most recently from d457b9d to 03c321a Compare March 20, 2018 11:56
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]>
@MylesBorins
Copy link
Contributor

landed in 5966b8c

@targos targos mentioned this pull request Mar 20, 2018
@Flarna Flarna deleted the backport-19200-to-v9.x branch March 22, 2018 19:53
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.

5 participants