deps: cherry-pick b93c80a from v8 upstream by matthewloring · Pull Request #7689 · nodejs/node · GitHub

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

Resolution of #6398.
\cc @nodejs/v8

/cc @nodejs/v8.

Added lts-watch-4.x label. v4.x would also need the fixes in deps: backport e093a04, 09db540 from upstream V8 #6398, but are not needed on master.
@matthewloring: for V8 5.2 and 5.3 can you request a merge in the upstream repository.

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

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

sounds reasonable to me

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.

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

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

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.

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

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

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

Looks good at a quick glance.

Uploaded.

/cc @jeisinger who did the original PR.

LGTM

CI: https://ci.nodejs.org/job/node-test-pull-request/3390/

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

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/231/

@mhdawson @joransiu Any thoughts on the s390x failures?

@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.

Running after rebase.
CI: https://ci.nodejs.org/job/node-test-pull-request/3407/
V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/238/

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.

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}

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

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

Landed in a3d62bd, e22ffef.

@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?

@ofrobots @thealphanerd Opened here: #7883.

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>

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>

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>

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>

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>