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

deps: cherry-pick b93c80a from v8 upstream #7689

Closed
wants to merge 2 commits into from
Closed

deps: cherry-pick b93c80a from v8 upstream #7689

wants to merge 2 commits into from

Conversation

matthewloring
Copy link

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

deps

Description of change

Original commit message:

If we can't rehash the backing store for weak sets & maps, do a last
resort GC

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

Committed: https://crrev.com/b93c80a6039c757723e70420ae73375b5d277814
Cr-Commit-Position: refs/heads/master@{#37591}

Fixes: #6180

@nodejs-github-bot nodejs-github-bot added the v8 engine Issues and PRs related to the V8 dependency. label Jul 12, 2016
@matthewloring
Copy link
Author

Resolution of #6398.
\cc @nodejs/v8

@ofrobots
Copy link
Contributor

/cc @nodejs/v8.

@MylesBorins
Copy link
Contributor

@ofrobots would you be willing to put together a PR against v4.x-staging that includes all of those changes?

@ofrobots
Copy link
Contributor

@thealphanerd Sure. Once this has baked for a couple of weeks on master and v6.x?

@MylesBorins
Copy link
Contributor

sounds reasonable to me

// If we're out of luck, we didn't get a GC recently, and so rehashing
// isn't enough to avoid a crash.
int nof = table->NumberOfElements() + 1;
if (!table->HasSufficientCapacity(nof)) {
Copy link
Member

Choose a reason for hiding this comment

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

The call to HasSufficientCapacity() with n == NumberOfElements() + 1 doesn't look obviously correct to me. Here is its implementation:

template <typename Derived, typename Shape, typename Key>
bool HashTable<Derived, Shape, Key>::HasSufficientCapacity(int n) {
  int capacity = Capacity();
  int nof = NumberOfElements() + n;
  int nod = NumberOfDeletedElements();
  // Return true if:
  //   50% is still free after adding n elements and
  //   at most 50% of the free elements are deleted elements.
  if (nod <= (capacity - nof) >> 1) {
    int needed_free = nof >> 1;
    if (nof + needed_free <= capacity) return true;
  }
  return false;
}

capacity - nof can be < 0 if capacity < NumberOfElements() * 2 + 1 and right-shifting a negative number has implementation dependent behavior. I think compilers all implement it as an arithmetic shift but I'd say it's better to avoid it altogether.

As well, because it uses int, there is a potential for integer overflow when NumberOfElements() is large.

Copy link
Contributor

Choose a reason for hiding this comment

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

The maximum size of the backing store is 128MB so that this can never happen.

Copy link
Member

Choose a reason for hiding this comment

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

You mean integer overflow? What about the capacity < NumberOfElements() * 2 + 1 case?

Copy link
Contributor

Choose a reason for hiding this comment

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

JSWeakCollection uses an ObjectHashTable as backing store. ObjectHashTableShape::kEntrySize is 2, ObjectHashTable::kMaxCapacity (FixedArray::kMaxSize(128 MB * kPointerSize) - FixArray::kHeaderSize) / kPointerSize / kEntrySize which is a bit less than 64MB, and NumberOfElements() * 2 + 1 can be at most 134217729 which fits nicely into a signed 32bit integer.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I understand that the integer overflow case is covered. I mean the right shift when capacity < nof.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, sorry, I didn't read carefully enough what you actually wrote. https://codereview.chromium.org/2162333002 should address this.

Copy link
Author

Choose a reason for hiding this comment

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

I can bring in 2162333002 as part of this PR. Does the fix there look good to you @bnoordhuis?

Copy link
Member

Choose a reason for hiding this comment

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

Looks good at a quick glance.

Copy link
Author

Choose a reason for hiding this comment

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

Uploaded.

@matthewloring
Copy link
Author

/cc @jeisinger who did the original PR.

@bnoordhuis
Copy link
Member

LGTM

@matthewloring
Copy link
Author

@bnoordhuis
Copy link
Member

Can you also run the V8 test suite? That's the https://ci.nodejs.org/job/node-test-commit-v8-linux/ job.

@ofrobots
Copy link
Contributor

@matthewloring
Copy link
Author

@mhdawson @joransiu Any thoughts on the s390x failures?

@joransiu
Copy link
Contributor

@matthewloring The s390x failures in StackAlignment in test-platform.cc (https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel72-s390x,v8test=v8test/231/console) should be fixed via #7771, which should have landed already.

@matthewloring
Copy link
Author

matthewloring commented Jul 25, 2016

@Trott
Copy link
Member

Trott commented Jul 25, 2016

Will probably want to re-run CI after #7873 lands (hopefully within the hour). That will fix the bad build on four of the Linux hosts.

Matt Loring added 2 commits July 25, 2016 14:19
Original commit message:

If we can't rehash the backing store for weak sets & maps, do a last
resort GC

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

Committed: https://crrev.com/b93c80a6039c757723e70420ae73375b5d277814
Cr-Commit-Position: refs/heads/master@{#37591}

Fixes: #6180
Original commit message:

Fix incorrect parameter to HasSufficientCapacity

It takes the number of additional elements, not the total target
capacity.

Also, avoid right-shifting a negative integer as this is undefined in
general

BUG=v8:4909
R=verwaest@chromium.org

Review-Url: https://codereview.chromium.org/2162333002
Cr-Commit-Position: refs/heads/master@{#37901}
@matthewloring
Copy link
Author

Only the windows CI is failing and it seems to be failing similarly in other builds. I'll land this tomorrow if nobody objects.

@Trott
Copy link
Member

Trott commented Jul 26, 2016

Looks like the Windows issue has been fixed. Fingers-crossed re-run of CI: https://ci.nodejs.org/job/node-test-pull-request/3415/

@matthewloring
Copy link
Author

Landed in a3d62bd, e22ffef.

@matthewloring matthewloring deleted the v8-4909 branch July 26, 2016 17:47
@ofrobots
Copy link
Contributor

ofrobots commented Jul 26, 2016

@matthewloring do you also want to take care of this:

@thealphanerd

would you be willing to put together a PR against v4.x-staging that includes all of those changes?

@matthewloring matthewloring restored the v8-4909 branch July 26, 2016 22:14
@matthewloring matthewloring deleted the v8-4909 branch July 26, 2016 22:35
@matthewloring
Copy link
Author

@ofrobots @thealphanerd Opened here: #7883.

ofrobots pushed a commit to ofrobots/node that referenced this pull request Aug 25, 2016
Original commit message:

If we can't rehash the backing store for weak sets & maps, do a last
resort GC

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

Committed: https://crrev.com/b93c80a6039c757723e70420ae73375b5d277814
Cr-Commit-Position: refs/heads/master@{#37591}

Fixes: nodejs#6180
PR-URL: nodejs#7689
Reviewed-By: Matt Loring <mattloring@google.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
ofrobots pushed a commit to ofrobots/node that referenced this pull request Aug 25, 2016
Original commit message:

Fix incorrect parameter to HasSufficientCapacity

It takes the number of additional elements, not the total target
capacity.

Also, avoid right-shifting a negative integer as this is undefined in
general

BUG=v8:4909
R=verwaest@chromium.org

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

Fixes: nodejs#6180
PR-URL: nodejs#7689
Reviewed-By: Matt Loring <mattloring@google.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this pull request Oct 10, 2016
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}

Ref: https://crbug.com/v8/4909
Ref: #7883
Fixes: #6180
PR-URL: #7689
Reviewed-By: Matt Loring <mattloring@google.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this pull request Oct 10, 2016
Original commit message:

If we can't rehash the backing store for weak sets & maps, do a last
resort GC

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

Committed: https://crrev.com/b93c80a6039c757723e70420ae73375b5d277814
Cr-Commit-Position: refs/heads/master@{#37591}

Fixes: #6180
Ref: #7883
PR-URL: #7689
Reviewed-By: Matt Loring <mattloring@google.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this pull request Oct 10, 2016
Original commit message:

  Fix incorrect parameter to HasSufficientCapacity

  It takes the number of additional elements, not the total target
  capacity.

  Also, avoid right-shifting a negative integer as this is undefined in
  general

  BUG=v8:4909
  R=verwaest@chromium.org

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

Fixes: #6180
Ref: #7883
PR-URL: #7689
Reviewed-By: Matt Loring <mattloring@google.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this pull request Oct 10, 2016
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}

Ref: https://crbug.com/v8/4909
Ref: #7883
Fixes: #6180
PR-URL: #7689
Reviewed-By: Matt Loring <mattloring@google.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this pull request Oct 10, 2016
Original commit message:

If we can't rehash the backing store for weak sets & maps, do a last
resort GC

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

Committed: https://crrev.com/b93c80a6039c757723e70420ae73375b5d277814
Cr-Commit-Position: refs/heads/master@{#37591}

Fixes: #6180
Ref: #7883
PR-URL: #7689
Reviewed-By: Matt Loring <mattloring@google.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this pull request Oct 10, 2016
Original commit message:

  Fix incorrect parameter to HasSufficientCapacity

  It takes the number of additional elements, not the total target
  capacity.

  Also, avoid right-shifting a negative integer as this is undefined in
  general

  BUG=v8:4909
  R=verwaest@chromium.org

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

Fixes: #6180
Ref: #7883
PR-URL: #7689
Reviewed-By: Matt Loring <mattloring@google.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
rvagg pushed a commit that referenced this pull request Oct 18, 2016
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}

Ref: https://crbug.com/v8/4909
Ref: #7883
Fixes: #6180
PR-URL: #7689
Reviewed-By: Matt Loring <mattloring@google.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
rvagg pushed a commit that referenced this pull request Oct 18, 2016
Original commit message:

If we can't rehash the backing store for weak sets & maps, do a last
resort GC

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

Committed: https://crrev.com/b93c80a6039c757723e70420ae73375b5d277814
Cr-Commit-Position: refs/heads/master@{#37591}

Fixes: #6180
Ref: #7883
PR-URL: #7689
Reviewed-By: Matt Loring <mattloring@google.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
rvagg pushed a commit that referenced this pull request Oct 18, 2016
Original commit message:

  Fix incorrect parameter to HasSufficientCapacity

  It takes the number of additional elements, not the total target
  capacity.

  Also, avoid right-shifting a negative integer as this is undefined in
  general

  BUG=v8:4909
  R=verwaest@chromium.org

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

Fixes: #6180
Ref: #7883
PR-URL: #7689
Reviewed-By: Matt Loring <mattloring@google.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this pull request Oct 26, 2016
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}

Ref: https://crbug.com/v8/4909
Ref: #7883
Fixes: #6180
PR-URL: #7689
Reviewed-By: Matt Loring <mattloring@google.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this pull request Oct 26, 2016
Original commit message:

If we can't rehash the backing store for weak sets & maps, do a last
resort GC

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

Committed: https://crrev.com/b93c80a6039c757723e70420ae73375b5d277814
Cr-Commit-Position: refs/heads/master@{#37591}

Fixes: #6180
Ref: #7883
PR-URL: #7689
Reviewed-By: Matt Loring <mattloring@google.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this pull request Oct 26, 2016
Original commit message:

  Fix incorrect parameter to HasSufficientCapacity

  It takes the number of additional elements, not the total target
  capacity.

  Also, avoid right-shifting a negative integer as this is undefined in
  general

  BUG=v8:4909
  R=verwaest@chromium.org

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

Fixes: #6180
Ref: #7883
PR-URL: #7689
Reviewed-By: Matt Loring <mattloring@google.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@MylesBorins MylesBorins mentioned this pull request Oct 26, 2016
BethGriggs pushed a commit to ibmruntimes/node that referenced this pull request Nov 9, 2016
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}

Ref: https://crbug.com/v8/4909
Ref: nodejs/node#7883
Fixes: nodejs/node#6180
PR-URL: nodejs/node#7689
Reviewed-By: Matt Loring <mattloring@google.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
BethGriggs pushed a commit to ibmruntimes/node that referenced this pull request Nov 9, 2016
Original commit message:

If we can't rehash the backing store for weak sets & maps, do a last
resort GC

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

Committed: https://crrev.com/b93c80a6039c757723e70420ae73375b5d277814
Cr-Commit-Position: refs/heads/master@{#37591}

Fixes: nodejs/node#6180
Ref: nodejs/node#7883
PR-URL: nodejs/node#7689
Reviewed-By: Matt Loring <mattloring@google.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
BethGriggs pushed a commit to ibmruntimes/node that referenced this pull request Nov 9, 2016
Original commit message:

  Fix incorrect parameter to HasSufficientCapacity

  It takes the number of additional elements, not the total target
  capacity.

  Also, avoid right-shifting a negative integer as this is undefined in
  general

  BUG=v8:4909
  R=verwaest@chromium.org

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

Fixes: nodejs/node#6180
Ref: nodejs/node#7883
PR-URL: nodejs/node#7689
Reviewed-By: Matt Loring <mattloring@google.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
abernix added a commit to abernix/node that referenced this pull request Aug 14, 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@{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
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
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
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.

Memory leak in simple test case involving WeakSet
9 participants