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

deps: backport e093a04, 09db540 from upstream V8 #6398

Closed
wants to merge 2 commits into from
Closed

deps: backport e093a04, 09db540 from upstream V8 #6398

wants to merge 2 commits into from

Conversation

ofrobots
Copy link
Contributor

@ofrobots ofrobots commented Apr 26, 2016 •
edited
Loading

Checklist
  • tests and code linting passes
  • the commit message follows commit guidelines
Affected core subsystem(s)

deps

Description of change

Original commit messages:
v8/v8@e093a04

Rehash and clear deleted entries in weak collections during GC
Otherwise, they'll just keep growing until we run out of memory or hit the FixedArray's maximum capacity.

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

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

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

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-Bug: https://crbug.com/v8/4909
Fixes: #6180
Ref: #6375
R=@nodejs/v8
CI: https://ci.nodejs.org/job/node-test-pull-request/2399/

@ofrobots
deps: backport e093a04, 09db540 from upstream V8
3e8d7a7 
Original commit messages:
v8/v8@e093a04
  Rehash and clear deleted entries in weak collections during GC
  Otherwise, they'll just keep growing until we run out of memory or hit the FixedArray's maximum capacity.

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

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

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

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-Bug: https://crbug.com/v8/4909
Fixes: nodejs#6180
@ofrobots
[SQUASH] fixup needed for backport
5b72b47
@ofrobots ofrobots added v8 engine Issues and PRs related to the V8 dependency. lts-watch-v4.x labels Apr 26, 2016
@indutny
Copy link
Member

indutny commented Apr 26, 2016

Rubber-stamp LGTM

@Fishrock123 Fishrock123 modified the milestone: 6.0.0 Apr 26, 2016
bnoordhuis
bnoordhuis reviewed Apr 26, 2016
View reviewed changes
deps/v8/src/heap/mark-compact.cc
@@ -2422,6 +2422,13 @@ void MarkCompactCollector::ClearWeakCollections() {
table->RemoveEntry(i);
}
}
// Rehash if more than 25% of the entries are deleted entries.
Copy link
Member

Choose a reason for hiding this comment

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

More than 50%, I think?

Copy link
Contributor

Choose a reason for hiding this comment

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

actually, it's 1/3, but in any case the comment is wrong

@bnoordhuis
Copy link
Member

LGTM

@ChALkeR
Copy link
Member

ChALkeR commented Apr 26, 2016 •
edited
Loading

Does this solve the testcase at #6375 (comment)?
I am not able to test that atm.
Note: this is just a question, it should not hold this PR.

@jasnell
Copy link
Member

jasnell commented Apr 26, 2016

Should this go into v6?

@ChALkeR
Copy link
Member

ChALkeR commented Apr 26, 2016

@jasnell I think that's the reason for backporting, isn't it?

@ChALkeR ChALkeR added the memory Issues and PRs related to the memory management or memory footprint. label Apr 26, 2016
@ofrobots
Copy link
Contributor Author

This can go in a patch level, no need to hold the release for this.
On Apr 26, 2016 8:52 AM, "Сковорода Никита Андреевич" <
notifications@github.com> wrote:

@jasnell https://github.com/jasnell I think that's the reason for
backporting, isn't it?

—
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#6398 (comment)

@jasnell
Copy link
Member

jasnell commented Apr 26, 2016

Ok. LGTM btw

jeisinger
jeisinger reviewed Apr 27, 2016
View reviewed changes
deps/v8/src/heap/mark-compact.cc
if ((table->NumberOfDeletedElements() << kJSWeakCollectionLoadFactorExp) >
table->NumberOfElements()) {
HandleScope scope(heap()->isolate());
table->Rehash(heap()->isolate()->factory()->undefined_value());
Copy link
Contributor

Choose a reason for hiding this comment

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

@ofrobots this is the wrong CL, you should only merge the "Reland" CL.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jeisinger: Applying the two patches from the 'Reland CL' (https://codereview.chromium.org/1890123002/#ps20001) produces the same patch as 3e8d7a7. Maybe I am missing something?

/cc @matthewloring re: #7883

@ChALkeR
Copy link
Member

ChALkeR commented Apr 27, 2016

Btw, @jeisinger, @natorion, what about back-porting this to v8 5.0.x?

@natorion
Copy link

Could you please use the process outlined in https://github.com/v8/v8/wiki/Merging%20&%20Patching ?

@ofrobots
Copy link
Contributor Author

ofrobots commented May 4, 2016

Superceded by #6572. I will open a separate PR to backport the relevant commits to v4.x.

@ofrobots
Copy link
Contributor Author

Upon verification, the upstream fixes are not sufficient for solving the original issue in #6180. Closing this PR. Let's wait until https://crbug.com/v8/4909 gets fully resolved.

@ofrobots ofrobots closed this May 10, 2016
@ChALkeR ChALkeR mentioned this pull request May 10, 2016
Closed
@matthewloring matthewloring mentioned this pull request Jul 12, 2016
Closed
2 tasks
@matthewloring matthewloring mentioned this pull request Jul 27, 2016
Closed
2 tasks
fhinkel added a commit to v8/node that referenced this pull request Jun 28, 2017
@fhinkel
[HACK] test: deactivate failing tests
d9ffec7 
See Build nodejs#6398.
@ofrobots ofrobots deleted the fix-6180-master branch July 21, 2017 14:27
abernix added a commit to abernix/node that referenced this pull request Aug 14, 2017
@abernix
deps: cherry-pick 09db540,686558d from V8 upstream
5f18fb2 
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
MylesBorins pushed a commit that referenced this pull request Aug 16, 2017
@abernix @MylesBorins
deps: cherry-pick 09db540,686558d from V8 upstream
5d7f68a 
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 pushed a commit that referenced this pull request Oct 25, 2017
@abernix @MylesBorins
deps: cherry-pick 09db540,686558d from V8 upstream
71f9cdf 
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>
gibfahn pushed a commit to ibmruntimes/node that referenced this pull request Nov 24, 2017
@abernix @gibfahn
deps: cherry-pick 09db540,686558d from V8 upstream
b77def2 
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
Reviewers
No reviews
Assignees
No one assigned
Labels
memory Issues and PRs related to the memory management or memory footprint. 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.

Memory leak in simple test case involving WeakSet
9 participants
@ofrobots @indutny @bnoordhuis @ChALkeR @jasnell @natorion @jeisinger @MylesBorins @Fishrock123