FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

[v12.x] deps: V8: backport fb26d0bb1835 by mmarchini · Pull Request #33573 · nodejs/node · GitHub

/ node Public

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

Closed
mmarchini wants to merge 1 commit into
nodejs:v12.x-stagingfrom
mmarchini:v12.x-backport-gc-fix
ConversationCommits (1)ChecksFiles changed
Closed

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

Conversation

mmarchini commented May 26, 2020

Copy link
Copy Markdown
Contributor

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 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 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 approved these changes May 26, 2020
View reviewed changes

addaleax left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

RSLGTM

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

Copy link
Copy Markdown
Collaborator

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 changed the title deps: V8: backport fb26d0bb1835 [v12.x] deps: V8: backport fb26d0bb1835 May 27, 2020
BridgeAR approved these changes May 27, 2020
View reviewed changes

BridgeAR left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

RSLGTM

BridgeAR added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label May 27, 2020
lundibundi mentioned this pull request Jun 3, 2020
Closed

mmarchini commented Jun 4, 2020

Copy link
Copy Markdown
Contributor Author

@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 commented Jun 4, 2020

Copy link
Copy Markdown
Member

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

mmarchini commented Jun 4, 2020

Copy link
Copy Markdown
Contributor Author

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 commented Jun 6, 2020

Copy link
Copy Markdown
Member

@mmarchini it should be fine on our end!

ex1st mentioned this pull request Jun 9, 2020
Merged

codebytere commented Jun 9, 2020

Copy link
Copy Markdown
Member

@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 commented Jun 17, 2020

Copy link
Copy Markdown
Contributor Author

I missed the notification :(

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

mmarchini force-pushed the v12.x-backport-gc-fix branch from 72d8e76 to e86be5c Compare June 17, 2020 19:26

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

Copy link
Copy Markdown
Collaborator

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

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 force-pushed the v12.x-backport-gc-fix branch from e86be5c to 1f9566b Compare June 17, 2020 19:31

mmarchini commented Jun 17, 2020

Copy link
Copy Markdown
Contributor Author

@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

This comment has been minimized.

Sign in to view

nodejs-github-bot commented Jun 26, 2020

Copy link
Copy Markdown
Collaborator

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

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

Copy link
Copy Markdown
Collaborator

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
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 commented Jun 26, 2020

Copy link
Copy Markdown
Member

Landed in 97a3f7b (v12.x-staging)

BethGriggs closed this Jun 26, 2020
BethGriggs added a commit that referenced this pull request Jun 26, 2020
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 mentioned this pull request Jun 26, 2020
Merged
BethGriggs added a commit that referenced this pull request Jun 26, 2020
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
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

addaleax addaleax approved these changes
codebytere codebytere approved these changes
BethGriggs BethGriggs approved these changes
BridgeAR BridgeAR approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, 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

Back | FazBrowse Home | New Git URL