[v4.x] deps: revert/re-apply 09db540 from upstream v8 - #14829
Closed
abernix wants to merge 2 commits into
Closed
Conversation
Copy link
Copy Markdown
Sorry, something went wrong.
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
bnoordhuis
approved these changes
Aug 15, 2017
bnoordhuis
left a comment
Copy link
Copy Markdown
Member
There was a problem hiding this comment.
Sorry, something went wrong.
Copy link
Copy Markdown
Member
|
ping @nodejs/lts |
Sorry, something went wrong.
Copy link
Copy Markdown
Contributor
|
test of v4.8.4 for comparison ci: https://ci.nodejs.org/job/node-test-commit/11812/ |
Sorry, something went wrong.
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>
Copy link
Copy Markdown
Contributor
|
Failures in CI are unrelated |
Sorry, something went wrong.
Copy link
Copy Markdown
|
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? |
Sorry, something went wrong.
Copy link
Copy Markdown
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 |
Sorry, something went wrong.
Closed
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>
Merged
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
Affected core subsystem(s)
V8 Engine
This is my first v8 commit, so happy to adjust accordingly, but this is what I've found!