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 #7883

Closed
wants to merge 4 commits into from
Closed

deps: cherry-pick b93c80a from v8 upstream #7883

wants to merge 4 commits into from

Conversation

matthewloring
Copy link

@matthewloring matthewloring commented Jul 26, 2016

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}

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

matthewloring commented Jul 26, 2016

/cc @thealphanerd @bnoordhuis @ofrobots

@mscdex mscdex added the v4.x label Jul 26, 2016
// Rehash if more than 25% of the entries are deleted entries.
// TODO(jochen): Consider to shrink the fixed array in place.
if ((table->NumberOfDeletedElements() << kJSWeakCollectionLoadFactorExp) >
table->NumberOfElements()) {
Copy link
Member

Choose a reason for hiding this comment

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

The 25% doesn't seem right. If I read the code right, it's 50%.

(Mild sense of deja vu, maybe I've commented on it before.)

Copy link
Author

Choose a reason for hiding this comment

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

Original review: #6398 (comment)

@bnoordhuis
Copy link
Member

LGTM with a question. Can you run both the node and the V8 CI?

@MylesBorins
Copy link
Contributor

is this important enough to roll into v4.5.0 or can it wait for the next patch release?

@matthewloring
Copy link
Author

Waiting for the next patch release should be fine.

@MylesBorins
Copy link
Contributor

@matthewloring would you be able to rebase this? We should be able to get it landed soon!

@MylesBorins
Copy link
Contributor

/cc @ofrobots @nodejs/v8 does this still need to be backported?

@bnoordhuis
Copy link
Member

Yes, this should still be back-ported.

@MylesBorins
Copy link
Contributor

ping @matthewloring

ofrobots and others added 4 commits October 5, 2016 14:06
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
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
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
PR-URL: #7689
Reviewed-By: Matt Loring <mattloring@google.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@matthewloring
Copy link
Author

@thealphanerd rebased

@MylesBorins
Copy link
Contributor

@MylesBorins
Copy link
Contributor

re running BSD: https://ci.nodejs.org/job/node-test-commit-freebsd/4719/
the windows failures are unrelated

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
Copy link
Contributor

failures are all unrelated to this PR, landing

d71dfdd...32b2fb8

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>
@matthewloring matthewloring deleted the staging branch October 20, 2016 21:15
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>
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>
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.

6 participants