Skip to content

Filter left trimmer v4.x - #10668

Merged
MylesBorins merged 4 commits into
nodejs:v4.x-stagingfrom
MylesBorins:filter-left-trimmer-v4.x
Jan 12, 2017
Merged

Filter left trimmer v4.x#10668
MylesBorins merged 4 commits into
nodejs:v4.x-stagingfrom
MylesBorins:filter-left-trimmer-v4.x

Conversation

@MylesBorins

@MylesBorinsMylesBorins commented Jan 6, 2017

Copy link
Copy Markdown
Contributor

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-botnodejs-github-bot added v4.x v8 engine Issues and PRs related to the V8 dependency. labels Jan 6, 2017
@MylesBorins

Copy link
Copy Markdown
ContributorAuthor

@indutnyindutny left a comment

Copy link
Copy Markdown
Member

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
Copy Markdown
ContributorAuthor

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

@ofrobotsofrobots left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for backporting!

Comment threaddeps/v8/include/v8-version.h Outdated

Copy link
Copy Markdown
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
Copy Markdown
ContributorAuthor

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
Copy Markdown
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

MylesBorins commented Jan 10, 2017

Copy link
Copy Markdown
ContributorAuthor

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
MylesBorinsforce-pushed the filter-left-trimmer-v4.x branch from 81618aa to cb75225CompareJanuary 12, 2017 22:33
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>
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>
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>
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
MylesBorinsforce-pushed the filter-left-trimmer-v4.x branch from cb75225 to 20bee0fCompareJanuary 12, 2017 22:34
@MylesBorins
MylesBorins merged commit 20bee0f into nodejs:v4.x-stagingJan 12, 2017
@MylesBorins

Copy link
Copy Markdown
ContributorAuthor

landed in 2677b9b...20bee0f

MylesBorins added a commit that referenced this pull request Jan 24, 2017
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
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
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
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>
@MylesBorinsMylesBorins mentioned this pull request Jan 24, 2017
MylesBorins added a commit that referenced this pull request Feb 1, 2017
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
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
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
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
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
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
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
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 freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

v8 engineIssues and PRs related to the V8 dependency.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@MylesBorins@ofrobots@indutny@jasnell@gibfahn@nodejs-github-bot