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

[v4.x] deps: revert/re-apply 09db540 from upstream v8 #14829

Closed
wants to merge 2 commits into from

Conversation

abernix
Copy link

@abernix abernix commented Aug 14, 2017

This PR aims to fix an improperly applied backport to the v8 deps on the v4.x series.

As discussed and discovered in #14228, the original intention of 3e8d7a7 was to apply v8/v8@e093a04 and v8/v8@09db540 but that backport commit failed to consider that the former commit (e093) was reverted in v8/v8@5f5a328. This led to inadvertent changes (which were never present on v8) to MarkCompactCollector::ClearWeakCollections being left in place on the Node.js 4.x series, in addition to the (incorrect) changes to the weakset and weakmap tests.

That MarkCompactCollector change is directly responsible for some errors, specifically noted in the stack traces of the post-mortems reported by myself and others in #14228 and meteor/meteor#8970 (comment).

Furthermore, while the correct fix to v8 was meant to apply the "Wipe deleted entries" logic to HashTable::Rehash it was also applied to CompilationCacheTable::Age as well. You'll note that an additional, seemingly arbitrary capacity variable needed to be declared in the backport which was not present in the original patch. Not too surprising as the patch applies cleanly to that class as well, albeit with a several thousand line patch offset. (This caught me off guard during debugging as well and it looks as if the v8 team was also bit by this temporarily in v8/v8@59c7657!)

In this process, I also took the liberty to backport the very relevant v8/v8@686558d which corrects the comment on the original commit.

It's my belief that this now matches up with the intended fix for #6180 and crbug.com/v8/4909 and also maintains relevant follow-ups, such as v8/v8@b93c80a6 and v8/v8@a76d133f.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

V8 Engine

This is my first v8 commit, so happy to adjust accordingly, but this is what I've found!

This reverts commit 2d07fd7, with the
exception of the `V8_PATCH_LEVEL` which will continue to increase.

This commit was intended to be a backport of v8's e093a04 and 09db540,
but it failed to consider the reversion of e093a04 in 5f5a328.
Original commit messages:
v8/v8@09db540
  Reland of Rehash and clear deleted entries in weak collections during GC

  BUG=v8:4909
  R=hpayer@chromium.org,ulan@chromium.org
  LOG=n

  Review URL: https://codereview.chromium.org/1890123002

  Cr-Commit-Position: refs/heads/master@{nodejs#35538}

v8/v8@686558d
  Fix comment about when we rehash ObjectHashTables before growing them

  R=ulan@chromium.org
  BUG=

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

Refs: https://crbug.com/v8/4909
Refs: nodejs#6180
Refs: nodejs#7689
Refs: nodejs#6398
Fixes: nodejs#14228
@nodejs-github-bot nodejs-github-bot added v4.x v8 engine Issues and PRs related to the V8 dependency. labels Aug 14, 2017
@addaleax addaleax changed the title deps: revert/re-apply 09db540 from upstream v8 [v4.x] deps: revert/re-apply 09db540 from upstream v8 Aug 14, 2017
@addaleax
Copy link
Member

@nodejs/v8 @MylesBorins

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

@jasnell
Copy link
Member

jasnell commented Aug 16, 2017

ping @nodejs/lts

@MylesBorins
Copy link
Contributor

MylesBorins pushed a commit that referenced this pull request Aug 16, 2017
This reverts commit 2d07fd7, with the
exception of the `V8_PATCH_LEVEL` which will continue to increase.

This commit was intended to be a backport of v8's e093a04 and 09db540,
but it failed to consider the reversion of e093a04 in 5f5a328.

PR-URL: #14829
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this pull request Aug 16, 2017
Original commit messages:
v8/v8@09db540
  Reland of Rehash and clear deleted entries in weak collections during GC

  BUG=v8:4909
  R=hpayer@chromium.org,ulan@chromium.org
  LOG=n

  Review URL: https://codereview.chromium.org/1890123002

  Cr-Commit-Position: refs/heads/master@{#35538}

v8/v8@686558d
  Fix comment about when we rehash ObjectHashTables before growing them

  R=ulan@chromium.org
  BUG=

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

Refs: https://crbug.com/v8/4909
Refs: #6180
Refs: #7689
Refs: #6398
Fixes: #14228

PR-URL: #14829
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@MylesBorins
Copy link
Contributor

Failures in CI are unrelated
fc6d118...5d7f68a

@abernix abernix deleted the abernix/reapply-backport branch August 16, 2017 10:20
@benjamn
Copy link
Contributor

benjamn commented Aug 18, 2017

Meteor is considering shipping a version of Node 4.8.4 with this patch, just until 4.8.5 is out. Do you (@MylesBorins, @nodejs/lts, et al.) have any guidance about (1) whether that's a terrible idea, and (2) when 4.8.5 might be released?

@MylesBorins
Copy link
Contributor

@benjamn floating the patch is not a terrible idea

next v4.x is likely in October, but perhaps we should ship it in september as the backlog is building

have an issue open about it nodejs/Release#241

@MylesBorins MylesBorins mentioned this pull request Sep 20, 2017
MylesBorins pushed a commit that referenced this pull request Oct 25, 2017
This reverts commit 2d07fd7, with the
exception of the `V8_PATCH_LEVEL` which will continue to increase.

This commit was intended to be a backport of v8's e093a04 and 09db540,
but it failed to consider the reversion of e093a04 in 5f5a328.

PR-URL: #14829
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this pull request Oct 25, 2017
Original commit messages:
v8/v8@09db540
  Reland of Rehash and clear deleted entries in weak collections during GC

  BUG=v8:4909
  R=hpayer@chromium.org,ulan@chromium.org
  LOG=n

  Review URL: https://codereview.chromium.org/1890123002

  Cr-Commit-Position: refs/heads/master@{#35538}

v8/v8@686558d
  Fix comment about when we rehash ObjectHashTables before growing them

  R=ulan@chromium.org
  BUG=

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

Refs: https://crbug.com/v8/4909
Refs: #6180
Refs: #7689
Refs: #6398
Fixes: #14228

PR-URL: #14829
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@MylesBorins MylesBorins mentioned this pull request Oct 25, 2017
gibfahn pushed a commit to ibmruntimes/node that referenced this pull request Nov 24, 2017
This reverts commit 2d07fd7, with the
exception of the `V8_PATCH_LEVEL` which will continue to increase.

This commit was intended to be a backport of v8's e093a04 and 09db540,
but it failed to consider the reversion of e093a04 in 5f5a328.

PR-URL: nodejs/node#14829
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
gibfahn pushed a commit to ibmruntimes/node that referenced this pull request Nov 24, 2017
Original commit messages:
v8/v8@09db540
  Reland of Rehash and clear deleted entries in weak collections during GC

  BUG=v8:4909
  R=hpayer@chromium.org,ulan@chromium.org
  LOG=n

  Review URL: https://codereview.chromium.org/1890123002

  Cr-Commit-Position: refs/heads/master@{#35538}

v8/v8@686558d
  Fix comment about when we rehash ObjectHashTables before growing them

  R=ulan@chromium.org
  BUG=

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

Refs: https://crbug.com/v8/4909
Refs: nodejs/node#6180
Refs: nodejs/node#7689
Refs: nodejs/node#6398
Fixes: nodejs/node#14228

PR-URL: nodejs/node#14829
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
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.

7 participants