Skip to content

[v12.x] deps: V8: backport fb26d0bb1835#33573

Closed
mmarchini wants to merge 1 commit intonodejs:v12.x-stagingfrom
mmarchini:v12.x-backport-gc-fix
Closed

[v12.x] deps: V8: backport fb26d0bb1835#33573
mmarchini wants to merge 1 commit intonodejs:v12.x-stagingfrom
mmarchini:v12.x-backport-gc-fix

Conversation

@mmarchini
Copy link
Contributor

@mmarchini mmarchini commented May 26, 2020

Original commit message:

[objects] Compact and shrink script_list

So far creating scripts always grew the script_list without ever
reusing cleared slots or shrinking. While this is probably not a
problem with script_list in practice, this is still a memory leak.

Fix this leak by using WeakArrayList::Append instead of AddToEnd.
Append adds to the end of the array, but potentially compacts and
shrinks the list as well. Other WeakArrayLists can use this method as
well, as long as they are not using indices into this array.

Bug: v8:10031
Change-Id: If743c4cc3f8d67ab735522f0ded038b2fb43e437
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1967385
Commit-Queue: Dominik Inführ <dinfuehr@chromium.org>
Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#65640}

Refs: v8/v8@fb26d0b

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. v12.x v8 engine Issues and PRs related to the V8 dependency. labels May 26, 2020
@mmarchini mmarchini linked an issue May 26, 2020 that may be closed by this pull request
Node v12.16.2 memory issues after upgrade from v8 (<--- Last few GCs --->) #33266
Closed
addaleax
addaleax approved these changes May 26, 2020
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

RSLGTM

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented May 26, 2020 •
edited by targos
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/31572/ ✔️ (yellow)
V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/3134/ ✔️

@BridgeAR BridgeAR changed the title deps: V8: backport fb26d0bb1835 [v12.x] deps: V8: backport fb26d0bb1835 May 27, 2020
BridgeAR
BridgeAR approved these changes May 27, 2020
View reviewed changes
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

RSLGTM

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 27, 2020
@lundibundi lundibundi mentioned this pull request Jun 3, 2020
Closed
@mmarchini
Copy link
Contributor Author

mmarchini commented Jun 4, 2020

@targos would it be better if V8 fixed this on 7.8, or should we just keep this as a floating patch? The patch is not that big (without tests), and since it's unlikely we bump to 7.9 before October anyway the patch shouldn't bring any headaches.

(also cc @codebytere, as I'm not sure how floating patches impact Electron)

@targos
Copy link
Member

targos commented Jun 4, 2020

I'm not sure what you mean? This V8 branch is no longer supported upstream

@mmarchini
Copy link
Contributor Author

mmarchini commented Jun 4, 2020

I see. In the issue it sounded like they were open to fixing it on 7.8, but maybe they meant if it is absolutely necessary.

@codebytere
Copy link
Member

codebytere commented Jun 6, 2020

@mmarchini it should be fine on our end!

@ex1st ex1st mentioned this pull request Jun 9, 2020
Merged
@codebytere
Copy link
Member

codebytere commented Jun 9, 2020

@mmarchini this needs to be rebased against latest v12.x-staging and then i can get it into the next RC for 12.x!

@mmarchini
Copy link
Contributor Author

mmarchini commented Jun 17, 2020

I missed the notification :(

We'll need to wait until the July release now, correct?

@mmarchini mmarchini force-pushed the v12.x-backport-gc-fix branch from 72d8e76 to e86be5c Compare June 17, 2020 19:26
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jun 17, 2020 •
edited by mmarchini
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/31914/
V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/3184/

@mmarchini
deps: V8: backport fb26d0bb1835
1f9566b 
Original commit message:

    [objects] Compact and shrink script_list

    So far creating scripts always grew the script_list without ever
    reusing cleared slots or shrinking. While this is probably not a
    problem with script_list in practice, this is still a memory leak.

    Fix this leak by using WeakArrayList::Append instead of AddToEnd.
    Append adds to the end of the array, but potentially compacts and
    shrinks the list as well. Other WeakArrayLists can use this method as
    well, as long as they are not using indices into this array.

    Bug: v8:10031
    Change-Id: If743c4cc3f8d67ab735522f0ded038b2fb43e437
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1967385
    Commit-Queue: Dominik Inführ <dinfuehr@chromium.org>
    Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#65640}

Refs: v8/v8@fb26d0b
@mmarchini mmarchini force-pushed the v12.x-backport-gc-fix branch from e86be5c to 1f9566b Compare June 17, 2020 19:31
@mmarchini
Copy link
Contributor Author

mmarchini commented Jun 17, 2020

@codebytere rebased :)

Did you get a conflict when using the release tools, or test errors? It rebased cleanly here, just had to bump the embedder string.

This was referenced Jun 18, 2020
Open
Open
Open
Open
Open
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jun 26, 2020

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jun 26, 2020 •
edited by BethGriggs
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/32109/ (Resume due to socket hang up on arm) ✅

BethGriggs pushed a commit that referenced this pull request Jun 26, 2020
@mmarchini @BethGriggs
deps: V8: backport fb26d0bb1835
97a3f7b 
Original commit message:

    [objects] Compact and shrink script_list

    So far creating scripts always grew the script_list without ever
    reusing cleared slots or shrinking. While this is probably not a
    problem with script_list in practice, this is still a memory leak.

    Fix this leak by using WeakArrayList::Append instead of AddToEnd.
    Append adds to the end of the array, but potentially compacts and
    shrinks the list as well. Other WeakArrayLists can use this method as
    well, as long as they are not using indices into this array.

    Bug: v8:10031
    Change-Id: If743c4cc3f8d67ab735522f0ded038b2fb43e437
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1967385
    Commit-Queue: Dominik Inführ <dinfuehr@chromium.org>
    Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#65640}

Refs: v8/v8@fb26d0b

PR-URL: #33573
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
@BethGriggs
Copy link
Member

BethGriggs commented Jun 26, 2020

Landed in 97a3f7b (v12.x-staging)

@BethGriggs BethGriggs closed this Jun 26, 2020
BethGriggs added a commit that referenced this pull request Jun 26, 2020
@BethGriggs
2020-06-30, Version 12.18.2 'Erbium' (LTS)
f5ca930 
Notable changes:

- deps: V8: backport fb26d0bb1835 (Matheus Marchini)
  [#33573](#33573)
- src: use symbol to store `AsyncWrap` resource (Anna Henningsen)
  [#31745](#31745)

PR-URL: TBD
@BethGriggs BethGriggs mentioned this pull request Jun 26, 2020
Merged
BethGriggs added a commit that referenced this pull request Jun 26, 2020
@BethGriggs
2020-06-30, Version 12.18.2 'Erbium' (LTS)
412c618 
Notable changes:

- deps: V8: backport fb26d0bb1835 (Matheus Marchini)
  [#33573](#33573)
- src: use symbol to store `AsyncWrap` resource (Anna Henningsen)
  [#31745](#31745)

PR-URL: #34077
This was referenced Jun 27, 2020
Open
Open
Open
Open
BethGriggs added a commit that referenced this pull request Jun 30, 2020
@BethGriggs
2020-06-30, Version 12.18.2 'Erbium' (LTS)
cb673e1 
Notable changes:

- deps: V8: backport fb26d0bb1835 (Matheus Marchini)
  [#33573](#33573)
- src: use symbol to store `AsyncWrap` resource (Anna Henningsen)
  [#31745](#31745)

PR-URL: #34077
This was referenced Jul 1, 2020
Open
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@addaleax addaleax addaleax approved these changes

@codebytere codebytere codebytere approved these changes

@BethGriggs BethGriggs BethGriggs approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. build Issues and PRs related to build files or the CI. 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.

Node v12.16.2 memory issues after upgrade from v8 (<--- Last few GCs --->)

7 participants

@mmarchini @nodejs-github-bot @targos @codebytere @BethGriggs @addaleax @BridgeAR