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: backport d727680 from V8 upstream #14947

Closed
wants to merge 2 commits into from
Closed

deps: backport d727680 from V8 upstream #14947

wants to merge 2 commits into from

Conversation

matthewloring
Copy link

Original commit message:

[d8] Bring PredictablePlatform in line with default platform
This removes a lot of special handling for the predictable platform.
Instead of executing spawned foreground and background tasks
immediately (i.e. inside the scope that spawns the tasks), just add
both to the foreground task queue.

This avoids existing special handling for predictable mode in wasm
async compilation, and should fix current failures on the predictable
bot.

BUG=v8:6427

Change-Id: Idbaa764a3dc8c230c29f3937d885e12174691ac4
Reviewed-on: https://chromium-review.googlesource.com/509694
Reviewed-by: Jochen Eisinger jochen@chromium.org
Commit-Queue: Clemens Hammacher clemensh@chromium.org
Cr-Commit-Position: refs/heads/master@{#45538}

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

@nodejs-github-bot nodejs-github-bot added the v8 engine Issues and PRs related to the V8 dependency. label Aug 19, 2017
@matthewloring
Copy link
Author

@matthewloring
Copy link
Author

It looks like there are still compilation failures on the nodes=benchmark configuration but the others are green.

@matthewloring
Copy link
Author

@matthewloring
Copy link
Author

@refack
Copy link
Contributor

refack commented Aug 19, 2017

Some "procedural" questions:

  1. I tried to compare to https://chromium-review.googlesource.com/509694 but saw significant diff - https://www.diffchecker.com/qchO3Yt3.
    Can you give some comments on that? (Obviously if the diff is the same we could just rubberstamp).
    If the cherrypick required manual massaging it would be great if you could separate it into 2 commit for easy review. Otherwise even if you simply tl;dr the required changes, that would help me review.

  2. I see this landed in 6.1.1 and the others from src: Node implementation of v8::Platform #14001 other patch version of 6.1 could you add that to the commit message, so when we bump to 6.1 we know there's no need to re-float this.

/cc @nodejs/v8

@MylesBorins
Copy link
Contributor

@refack fwiw I went through with the tool and double checked all floated patches before landing them to see whether or not they were in the latest release

@seishun
Copy link
Contributor

seishun commented Aug 20, 2017

Is it even possibly to re-float a patch that's already in the release? I would assume git would skip it as an empty commit.

@refack
Copy link
Contributor

refack commented Aug 20, 2017

Is it even possibly to re-float a patch that's already in the release? I would assume git would skip it as an empty commit.

Or conflict, and then you need to figure out what's the story. But if there's a tool and a procedure, than my point is moot.

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

rubber stamp lgtm

@targos
Copy link
Member

targos commented Aug 21, 2017

Please s/cherry-pick/backport/ because this is not a clean cherry-pick.

@matthewloring
Copy link
Author

@refack The TL;DR is that I applied v8/v8@d727680 and then resolved the merge conflicts in PredictablePlatform. These were mostly caused by the fact that the GetTracingController portion of the change had already landed in node. I don't recall making any other changes.

The procedural question I had was how to handle the second commit in this PR. It is not related to the backport and is not implemented in V8 since the affected file no longer existed in V8 when CancelableTaskManager::Id was added. Should I open this as a separate PR?

Matt Loring added 2 commits August 21, 2017 13:37
Original commit message:

[d8] Bring PredictablePlatform in line with default platform
This removes a lot of special handling for the predictable platform.
Instead of executing spawned foreground and background tasks
immediately (i.e. inside the scope that spawns the tasks), just add
both to the foreground task queue.

This avoids existing special handling for predictable mode in wasm
async compilation, and should fix current failures on the predictable
bot.

BUG=v8:6427

Change-Id: Idbaa764a3dc8c230c29f3937d885e12174691ac4
Reviewed-on: https://chromium-review.googlesource.com/509694
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Commit-Queue: Clemens Hammacher <clemensh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#45538}
@matthewloring matthewloring changed the title deps: cherry-pick d727680 from V8 upstream deps: backport d727680 from V8 upstream Aug 21, 2017
@matthewloring
Copy link
Author

@targos Updated the name to backport.

Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

50% Rubberstamp
40% git procedures
10% Review

@MylesBorins
Copy link
Contributor

LGMT if CI is green (V8 CI :P)

@matthewloring
Copy link
Author

@MylesBorins @targos Any thoughts on the second commit? Should it be squashed or left as its own commit?

@ofrobots
Copy link
Contributor

Squash, given the understanding that the first commit won't compile just by itself.

@matthewloring
Copy link
Author

Ok, I'll plan to squash and land this tomorrow morning if nobody else has concerns.

@matthewloring
Copy link
Author

Landed in d348512. V8 CI should be happy again 😸

@matthewloring matthewloring deleted the backport branch August 22, 2017 16:55
addaleax pushed a commit to addaleax/ayo that referenced this pull request Aug 25, 2017
Original commit message:

[d8] Bring PredictablePlatform in line with default platform
This removes a lot of special handling for the predictable platform.
Instead of executing spawned foreground and background tasks
immediately (i.e. inside the scope that spawns the tasks), just add
both to the foreground task queue.

This avoids existing special handling for predictable mode in wasm
async compilation, and should fix current failures on the predictable
bot.

BUG=v8:6427

Change-Id: Idbaa764a3dc8c230c29f3937d885e12174691ac4
Reviewed-on: https://chromium-review.googlesource.com/509694
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Commit-Queue: Clemens Hammacher <clemensh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#45538}

PR-URL: nodejs/node#14947
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Nikolai Vavilov <vvnicholas@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Aug 28, 2017
Original commit message:

[d8] Bring PredictablePlatform in line with default platform
This removes a lot of special handling for the predictable platform.
Instead of executing spawned foreground and background tasks
immediately (i.e. inside the scope that spawns the tasks), just add
both to the foreground task queue.

This avoids existing special handling for predictable mode in wasm
async compilation, and should fix current failures on the predictable
bot.

BUG=v8:6427

Change-Id: Idbaa764a3dc8c230c29f3937d885e12174691ac4
Reviewed-on: https://chromium-review.googlesource.com/509694
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Commit-Queue: Clemens Hammacher <clemensh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#45538}

PR-URL: nodejs/node#14947
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Nikolai Vavilov <vvnicholas@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
MylesBorins pushed a commit that referenced this pull request Sep 10, 2017
Original commit message:

[d8] Bring PredictablePlatform in line with default platform
This removes a lot of special handling for the predictable platform.
Instead of executing spawned foreground and background tasks
immediately (i.e. inside the scope that spawns the tasks), just add
both to the foreground task queue.

This avoids existing special handling for predictable mode in wasm
async compilation, and should fix current failures on the predictable
bot.

BUG=v8:6427

Change-Id: Idbaa764a3dc8c230c29f3937d885e12174691ac4
Reviewed-on: https://chromium-review.googlesource.com/509694
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Commit-Queue: Clemens Hammacher <clemensh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#45538}

PR-URL: #14947
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Nikolai Vavilov <vvnicholas@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@MylesBorins MylesBorins mentioned this pull request Sep 10, 2017
MylesBorins pushed a commit that referenced this pull request Sep 12, 2017
Original commit message:

[d8] Bring PredictablePlatform in line with default platform
This removes a lot of special handling for the predictable platform.
Instead of executing spawned foreground and background tasks
immediately (i.e. inside the scope that spawns the tasks), just add
both to the foreground task queue.

This avoids existing special handling for predictable mode in wasm
async compilation, and should fix current failures on the predictable
bot.

BUG=v8:6427

Change-Id: Idbaa764a3dc8c230c29f3937d885e12174691ac4
Reviewed-on: https://chromium-review.googlesource.com/509694
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Commit-Queue: Clemens Hammacher <clemensh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#45538}

PR-URL: #14947
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Nikolai Vavilov <vvnicholas@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@MylesBorins
Copy link
Contributor

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants