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

Filter left trimmer v4.x #10668

Merged
merged 4 commits into from
Jan 12, 2017
Merged

Filter left trimmer v4.x #10668

merged 4 commits into from
Jan 12, 2017

Conversation

MylesBorins
Copy link
Contributor

@MylesBorins MylesBorins commented Jan 6, 2017 •
edited
Loading

Backport of d800a65, 7a88ff3, and a715957 from V8 upstream to v4.x

These commits would not land cleanly on v4.x and needed to be manually reproduced. Due to the commits needing to be manually recreated I would appreciate a slightly more thorough review of this PR.

Conflicts:

  • 6403bb2:
    • Does not include changes to SLOW_DCHECK
  • d91c6ae:
    • Does not include changes to src/heap/scavenger.cc
    • These changes would be reverted in 477fb96 so they can be considered a no-op
  • 477fb96:
    • Does not include changes to src/heap/scavenger.cc
    • this negates the changes not included in d91c6ae

/cc @ofrobots @nodejs/v8

@nodejs-github-bot nodejs-github-bot added v4.x v8 engine Issues and PRs related to the V8 dependency. labels Jan 6, 2017
@MylesBorins
Copy link
Contributor Author

CI: https://ci.nodejs.org/job/node-test-pull-request/5737/
V8-CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/500/

indutny
indutny approved these changes Jan 6, 2017
View reviewed changes
Copy link
Member

@indutny indutny 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

@MylesBorins
Copy link
Contributor Author

looks like there are failures in CI @nodejs/v8 can you please take a look

ofrobots
ofrobots approved these changes Jan 7, 2017
View reviewed changes
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.

Thanks for backporting!

gibfahn
gibfahn reviewed Jan 7, 2017
View reviewed changes
deps/v8/include/v8-version.h
@@ -11,7 +11,7 @@
#define V8_MAJOR_VERSION 4
#define V8_MINOR_VERSION 5
#define V8_BUILD_NUMBER 103
#define V8_PATCH_LEVEL 43
#define V8_PATCH_LEVEL 44
Copy link
Member

Choose a reason for hiding this comment

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

If this is 3 commits, should the patch level be bumped 3 times?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do not believe so /cc @ofrobots

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO it is not necessary to bump the patch level on each commit – although an argument could be made that the commits be squashed. Upstream does squash when doing back-merges. In this case, each of the commit ended being slightly different from the upstream version, but as long as that is documented in the PR here, it might be okay to squash? I don't have strong feelings on this however.

@MylesBorins
Copy link
Contributor Author

MylesBorins commented Jan 10, 2017 •
edited
Loading

one more attempt at ci: https://ci.nodejs.org/job/node-test-commit-v8-linux/509/

edit: still getting failures so here is a build using v4.x-staging to see if they are expected

https://ci.nodejs.org/job/node-test-commit-v8-linux/510/

/cc @nodejs/v8 can you take a look

@MylesBorins
deps: backport d800a65 from V8 upstream
47d18d4 
This backport does not include the original changes to SLOW_DCHECK
as it does not exist in the V8 in node v4.x

Original commit message:
  Filter out stale left-trimmed handles

  BUG=chromium:620553
  LOG=N
  R=jochen@chromium.org

  Review-Url: https://codereview.chromium.org/2078403002
  Cr-Commit-Position: refs/heads/master@{nodejs#37108}

PR-URL: nodejs#10668
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
@MylesBorins
deps: backport 7a88ff3 from V8 upstream
83144af 
This backport does not include the changes to `src/heap/scavenger.cc`
as it does not exist in the V8 included in the v4.x stream.

Original commit message:
  Filter out stale left-trimmed handles for scavenges

  The missing part from
    https://codereview.chromium.org/2078403002/

  R=jochen@chromium.org
  BUG=chromium:621869
  LOG=N

  Review-Url: https://codereview.chromium.org/2077353004
  Cr-Commit-Position: refs/heads/master@{nodejs#37184}

PR-URL: nodejs#10668
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
@MylesBorins
deps: backport a715957 from V8 upstream
e0db108 
This commit does not include the changes to `src/heap/scavenger.cc`.

These changes would revert the changes that should have come in
086bd5aede, meaning that there is no issue with that change missing
in the previous commit.

Original commit message:
  Iterate handles with special left-trim visitor

  BUG=chromium:620553
  LOG=N
  R=hpayer@chromium.org

  Review-Url: https://codereview.chromium.org/2102243002
  Cr-Commit-Position: refs/heads/master@{nodejs#37366}

PR-URL: nodejs#10668
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
@MylesBorins
deps: update patch level in V8
20bee0f 
PR-URL: nodejs#10668
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
@MylesBorins MylesBorins merged commit 20bee0f into nodejs:v4.x-staging Jan 12, 2017
@MylesBorins
Copy link
Contributor Author

landed in 2677b9b...20bee0f

MylesBorins added a commit that referenced this pull request Jan 24, 2017
@MylesBorins
deps: backport d800a65 from V8 upstream
97d1c6b 
This backport does not include the original changes to SLOW_DCHECK
as it does not exist in the V8 in node v4.x

Original commit message:
  Filter out stale left-trimmed handles

  BUG=chromium:620553
  LOG=N
  R=jochen@chromium.org

  Review-Url: https://codereview.chromium.org/2078403002
  Cr-Commit-Position: refs/heads/master@{#37108}

PR-URL: #10668
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
MylesBorins added a commit that referenced this pull request Jan 24, 2017
@MylesBorins
deps: backport 7a88ff3 from V8 upstream
ba4a20a 
This backport does not include the changes to `src/heap/scavenger.cc`
as it does not exist in the V8 included in the v4.x stream.

Original commit message:
  Filter out stale left-trimmed handles for scavenges

  The missing part from
    https://codereview.chromium.org/2078403002/

  R=jochen@chromium.org
  BUG=chromium:621869
  LOG=N

  Review-Url: https://codereview.chromium.org/2077353004
  Cr-Commit-Position: refs/heads/master@{#37184}

PR-URL: #10668
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
MylesBorins added a commit that referenced this pull request Jan 24, 2017
@MylesBorins
deps: backport a715957 from V8 upstream
18be292 
This commit does not include the changes to `src/heap/scavenger.cc`.

These changes would revert the changes that should have come in
086bd5aede, meaning that there is no issue with that change missing
in the previous commit.

Original commit message:
  Iterate handles with special left-trim visitor

  BUG=chromium:620553
  LOG=N
  R=hpayer@chromium.org

  Review-Url: https://codereview.chromium.org/2102243002
  Cr-Commit-Position: refs/heads/master@{#37366}

PR-URL: #10668
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
MylesBorins added a commit that referenced this pull request Jan 24, 2017
@MylesBorins
deps: update patch level in V8
3f05d95 
PR-URL: #10668
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
@MylesBorins MylesBorins mentioned this pull request Jan 24, 2017
Merged
MylesBorins added a commit that referenced this pull request Feb 1, 2017
@MylesBorins
deps: backport d800a65 from V8 upstream
8bd3d83 
This backport does not include the original changes to SLOW_DCHECK
as it does not exist in the V8 in node v4.x

Original commit message:
  Filter out stale left-trimmed handles

  BUG=chromium:620553
  LOG=N
  R=jochen@chromium.org

  Review-Url: https://codereview.chromium.org/2078403002
  Cr-Commit-Position: refs/heads/master@{#37108}

PR-URL: #10668
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
MylesBorins added a commit that referenced this pull request Feb 1, 2017
@MylesBorins
deps: backport 7a88ff3 from V8 upstream
ce66c8e 
This backport does not include the changes to `src/heap/scavenger.cc`
as it does not exist in the V8 included in the v4.x stream.

Original commit message:
  Filter out stale left-trimmed handles for scavenges

  The missing part from
    https://codereview.chromium.org/2078403002/

  R=jochen@chromium.org
  BUG=chromium:621869
  LOG=N

  Review-Url: https://codereview.chromium.org/2077353004
  Cr-Commit-Position: refs/heads/master@{#37184}

PR-URL: #10668
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
MylesBorins added a commit that referenced this pull request Feb 1, 2017
@MylesBorins
deps: backport a715957 from V8 upstream
a234d44 
This commit does not include the changes to `src/heap/scavenger.cc`.

These changes would revert the changes that should have come in
086bd5aede, meaning that there is no issue with that change missing
in the previous commit.

Original commit message:
  Iterate handles with special left-trim visitor

  BUG=chromium:620553
  LOG=N
  R=hpayer@chromium.org

  Review-Url: https://codereview.chromium.org/2102243002
  Cr-Commit-Position: refs/heads/master@{#37366}

PR-URL: #10668
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
MylesBorins added a commit that referenced this pull request Feb 1, 2017
@MylesBorins
deps: update patch level in V8
4e07bd4 
PR-URL: #10668
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
gibfahn pushed a commit to ibmruntimes/node that referenced this pull request Feb 22, 2017
@MylesBorins @gibfahn
deps: backport d800a65 from V8 upstream
785d66f 
This backport does not include the original changes to SLOW_DCHECK
as it does not exist in the V8 in node v4.x

Original commit message:
  Filter out stale left-trimmed handles

  BUG=chromium:620553
  LOG=N
  R=jochen@chromium.org

  Review-Url: https://codereview.chromium.org/2078403002
  Cr-Commit-Position: refs/heads/master@{#37108}

PR-URL: nodejs/node#10668
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
gibfahn pushed a commit to ibmruntimes/node that referenced this pull request Feb 22, 2017
@MylesBorins @gibfahn
deps: backport 7a88ff3 from V8 upstream
10398cc 
This backport does not include the changes to `src/heap/scavenger.cc`
as it does not exist in the V8 included in the v4.x stream.

Original commit message:
  Filter out stale left-trimmed handles for scavenges

  The missing part from
    https://codereview.chromium.org/2078403002/

  R=jochen@chromium.org
  BUG=chromium:621869
  LOG=N

  Review-Url: https://codereview.chromium.org/2077353004
  Cr-Commit-Position: refs/heads/master@{#37184}

PR-URL: nodejs/node#10668
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
gibfahn pushed a commit to ibmruntimes/node that referenced this pull request Feb 22, 2017
@MylesBorins @gibfahn
deps: backport a715957 from V8 upstream
2386ddc 
This commit does not include the changes to `src/heap/scavenger.cc`.

These changes would revert the changes that should have come in
086bd5aede, meaning that there is no issue with that change missing
in the previous commit.

Original commit message:
  Iterate handles with special left-trim visitor

  BUG=chromium:620553
  LOG=N
  R=hpayer@chromium.org

  Review-Url: https://codereview.chromium.org/2102243002
  Cr-Commit-Position: refs/heads/master@{#37366}

PR-URL: nodejs/node#10668
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
gibfahn pushed a commit to ibmruntimes/node that referenced this pull request Feb 22, 2017
@MylesBorins @gibfahn
deps: update patch level in V8
e9a59d1 
PR-URL: nodejs/node#10668
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
@MylesBorins MylesBorins deleted the filter-left-trimmer-v4.x branch November 14, 2017 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gibfahn gibfahn gibfahn left review comments

@ofrobots ofrobots ofrobots approved these changes

@jasnell jasnell jasnell approved these changes

@indutny indutny indutny approved these changes

Assignees
No one assigned
Labels
v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@MylesBorins @ofrobots @indutny @jasnell @gibfahn @nodejs-github-bot