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

[v12.x] deps: V8: cherry-pick cb1c2b0fbfe7 #31939

Closed

Conversation

Flarna
Copy link
Member

@Flarna Flarna commented Feb 24, 2020

Original commit message:

Remove noscript_shared_function_infos

SharedFunctionInfos that do not belong to a script were tracked in
noscript_shared_function_infos. However this was only used in object-stats.
Remove this since it was actually leaking memory in some use cases.

Bug: v8:9674
Change-Id: I9482f7e5dedf975666a70684b3d2ea04c9a23518
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1798423
Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Reviewed-by: Michael Stanton <mvstanton@chromium.org>
Commit-Queue: Dominik Inführ <dinfuehr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#63685}

Refs: v8/v8@cb1c2b0
Refs: #31914

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • 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 Feb 24, 2020
@mmarchini mmarchini added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 26, 2020
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@mmarchini
Copy link
Contributor

17:44:05 #
17:44:05 # Fatal error in , line 0
17:44:05 # Check failed: CompareTexts(BuildActual(printer, snippets), LoadGolden("PrivateMethodAccess.golden")).
17:44:05 #

FWIW, this is a known failure (#32076)

@nodejs-github-bot

This comment has been minimized.

@Flarna
Copy link
Member Author

Flarna commented Mar 17, 2020

The CI failures looked strange. Rebased to get any relevant change in.

@Flarna
Copy link
Member Author

Flarna commented Mar 17, 2020

CI fails because No space left on device

@mmarchini
Copy link
Contributor

There are a lot of failures like this:

12:41:57 /home/iojs/build/workspace/node-test-commit-aix/nodes/aix71-ppc64/tools/doc/allhtml.js:87
12:41:57   if (!ids.has(match[1])) throw new Error(`link not found: ${match[1]}`);
12:41:57                           ^
12:41:57 
12:41:57 Error: link not found: repl_reverse_i_search
12:41:57     at Object.<anonymous> (/home/iojs/build/workspace/node-test-commit-aix/nodes/aix71-ppc64/tools/doc/allhtml.js:87:33)
12:41:57     at Module._compile (internal/modules/cjs/loader.js:1158:30)
12:41:57     at Object.Module._extensions..js (internal/modules/cjs/loader.js:1178:10)
12:41:57     at Module.load (internal/modules/cjs/loader.js:1002:32)
12:41:57     at Function.Module._load (internal/modules/cjs/loader.js:901:14)
12:41:57     at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:74:12)
12:41:57     at internal/main/run_main_module.js:18:47

Might be because of the rebase? I'm not sure

@richardlau
Copy link
Member

There are a lot of failures like this:

12:41:57 /home/iojs/build/workspace/node-test-commit-aix/nodes/aix71-ppc64/tools/doc/allhtml.js:87
12:41:57   if (!ids.has(match[1])) throw new Error(`link not found: ${match[1]}`);
12:41:57                           ^
12:41:57 
12:41:57 Error: link not found: repl_reverse_i_search
12:41:57     at Object.<anonymous> (/home/iojs/build/workspace/node-test-commit-aix/nodes/aix71-ppc64/tools/doc/allhtml.js:87:33)
12:41:57     at Module._compile (internal/modules/cjs/loader.js:1158:30)
12:41:57     at Object.Module._extensions..js (internal/modules/cjs/loader.js:1178:10)
12:41:57     at Module.load (internal/modules/cjs/loader.js:1002:32)
12:41:57     at Function.Module._load (internal/modules/cjs/loader.js:901:14)
12:41:57     at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:74:12)
12:41:57     at internal/main/run_main_module.js:18:47

Might be because of the rebase? I'm not sure

Should be fixed by rebasing: #32282 (comment)

@mmarchini
Copy link
Contributor

Ah, @Flarna rebased after the CI. I'll start it again

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@mmarchini
Copy link
Contributor

mmarchini commented Mar 18, 2020

I mean, this is probably good to land, the last failing on Jenkins was infra-related, but let's try again just to be sure. @Flarna were you expecting this to be included in the next release?

@Flarna
Copy link
Member Author

Flarna commented Mar 18, 2020

@mmarchini I was not targeting a specific release.
I created this PR simply because I saw the #31914 and found it's easy to fix - and a nice training for me to practice v8 back porting.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Mar 19, 2020

@Flarna
Copy link
Member Author

Flarna commented Mar 26, 2020

Rebased to get rid of merge conflicts.

@codebytere codebytere force-pushed the v12.x-staging branch 2 times, most recently from 63a03d2 to d577190 Compare March 31, 2020 23:57
@MylesBorins
Copy link
Contributor

@Flarna apologies but this requires another rebase. I've gone ahead and done the work but was unable to push to your branch.

https://github.com/MylesBorins/node/tree/backport-cb1c2b0

Original commit message:

    Remove noscript_shared_function_infos

    SharedFunctionInfos that do not belong to a script were tracked in
    noscript_shared_function_infos. However this was only used in object-stats.
    Remove this since it was actually leaking memory in some use cases.

    Bug: v8:9674
    Change-Id: I9482f7e5dedf975666a70684b3d2ea04c9a23518
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1798423
    Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
    Reviewed-by: Michael Stanton <mvstanton@chromium.org>
    Commit-Queue: Dominik Inführ <dinfuehr@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#63685}

Refs: v8/v8@cb1c2b0
@Flarna
Copy link
Member Author

Flarna commented Apr 1, 2020

@MylesBorins Rebased
Not sure why you can't push to my branch. Most likely because the repo is part of an organization and not a personal fork.

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 1, 2020

@MylesBorins
Copy link
Contributor

Looks like this might introduce new failures on both regular CI and V8-CI. Running CI against staging to confirm

12.x-staging CI: https://ci.nodejs.org/job/node-test-commit/37126/
12.x-staging V8: https://ci.nodejs.org/job/node-test-commit-v8-linux/2987/

@mmarchini
Copy link
Contributor

mmarchini commented Apr 1, 2020

@MylesBorins those are known, V8 CI on v12 is consistently failing, see comment above: #31939 (comment).

(I think I know how to fix it, will try later)

@nodejs-github-bot
Copy link
Collaborator

MylesBorins pushed a commit that referenced this pull request Apr 2, 2020
Original commit message:

    Remove noscript_shared_function_infos

    SharedFunctionInfos that do not belong to a script were tracked in
    noscript_shared_function_infos. However this was only used in object-stats.
    Remove this since it was actually leaking memory in some use cases.

    Bug: v8:9674
    Change-Id: I9482f7e5dedf975666a70684b3d2ea04c9a23518
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1798423
    Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
    Reviewed-by: Michael Stanton <mvstanton@chromium.org>
    Commit-Queue: Dominik Inführ <dinfuehr@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#63685}

PR-URL: #31939
Refs: v8/v8@cb1c2b0
Refs: #31914
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
@MylesBorins
Copy link
Contributor

landed in a83fc49

@MylesBorins MylesBorins closed this Apr 2, 2020
@Flarna Flarna deleted the backport-cb1c2b0 branch April 2, 2020 07:08
@codebytere codebytere mentioned this pull request Apr 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

7 participants