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: backport 525b396195 from upstream V8 #23827

Closed

Conversation

psmarshall
Copy link
Contributor

Original commit message:

[cpu-profiler] Fix a leak caused by re-logging existing functions.

Don't re-log all existing functions during StartProcessorIfNotStarted().
They will already be in the CodeMap attached to the ProfileGenerator and
re-logging them causes leaks. See the linked bug for more details.

Bug: v8:8253
Change-Id: Ibb1a1ab2431c588e8c3a3a9ff714767cdf61a88e
Reviewed-on: https://chromium-review.googlesource.com/1256763
Commit-Queue: Peter Marshall petermarshall@chromium.org
Reviewed-by: Yang Guo yangguo@chromium.org
Cr-Commit-Position: refs/heads/master@{#56336}

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

This has landed on V8 7.0 (https://chromium-review.googlesource.com/c/v8/v8/+/1280443)

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. v10.x v8 engine Issues and PRs related to the V8 dependency. labels Oct 23, 2018
@psmarshall
Copy link
Contributor Author

@Flarna FYI, here is the backport of the fix. I'll open a separate one for v8.x

@targos
Copy link
Member

targos commented Oct 23, 2018

Refs: v8/v8@525b396

@Flarna
Copy link
Member

Flarna commented Oct 23, 2018

Refs: #23070

@mmarchini
Copy link
Contributor

@mmarchini mmarchini added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 23, 2018
@refack refack mentioned this pull request Oct 23, 2018
3 tasks
@refack
Copy link
Contributor

refack commented Oct 23, 2018

Some failures might be fixed with #23733

@psmarshall
Copy link
Contributor Author

I've rebased, could we run the CI & V8 CI again now that #23733 has been backported to 10?

@refack
Copy link
Contributor

refack commented Oct 30, 2018

@Trott
Copy link
Member

Trott commented Nov 2, 2018

Are the V8 CI failures expected?

@refack
Copy link
Contributor

refack commented Nov 2, 2018

It's an escalated (-Werror) deprecation warning.
So half expected, as in expected some deprecated function use, not expected for it to fail the build.

10:24:07 ../../src/inspector/v8-console-message.cc:163:34: error: 'ToString' is deprecated [-Werror,-Wdeprecated-declarations]
10:24:07     bool result = append(bigint->ToString());
10:24:07                                  ^
10:24:07 ../../include/v8.h:2454:10: note: 'ToString' has been explicitly marked deprecated here
10:24:07   inline V8_DEPRECATED("Use maybe version", Local<String> ToString() const);
10:24:07          ^
10:24:07 ../../include/v8config.h:327:29: note: expanded from macro 'V8_DEPRECATED'

@psmarshall
Copy link
Contributor Author

This PR doesn't touch any of that code, so I guess the V8 CI would also fail without this PR?

@targos
Copy link
Member

targos commented Nov 2, 2018

V8 CI run on v10.x-staging to compare: https://ci.nodejs.org/job/node-test-commit-v8-linux/1806/

@psmarshall
Copy link
Contributor Author

I confirmed locally that it does fail without this PR

@psmarshall
Copy link
Contributor Author

psmarshall commented Nov 2, 2018

I'll bisect 👍

@richardlau
Copy link
Member

It's an escalated (-Werror) deprecation warning.
So half expected, as in expected some deprecated function use, not expected for it to fail the build.

10:24:07 ../../src/inspector/v8-console-message.cc:163:34: error: 'ToString' is deprecated [-Werror,-Wdeprecated-declarations]
10:24:07     bool result = append(bigint->ToString());
10:24:07                                  ^
10:24:07 ../../include/v8.h:2454:10: note: 'ToString' has been explicitly marked deprecated here
10:24:07   inline V8_DEPRECATED("Use maybe version", Local<String> ToString() const);
10:24:07          ^
10:24:07 ../../include/v8config.h:327:29: note: expanded from macro 'V8_DEPRECATED'

46c7d0d changed some V8_DEPRECATE_SOON to V8_DEPRECATED (including the one on line 2454 of v8.h that errored here).

@psmarshall
Copy link
Contributor Author

Yep looks like that's the one. We could partially revert that CL (just the ones that are still in use) or backport the usage of the new APIs from V8

@refack
Copy link
Contributor

refack commented Nov 2, 2018

I had a strong intuition that it was my fault... But our V8 CI builds with GN, so maybe it's not a GYP bug...

@Trott
Copy link
Member

Trott commented Nov 2, 2018

Should we remove author ready from this? Or is this OK to land in 10.x as-is?

@psmarshall
Copy link
Contributor Author

I'll submit another CL to fix the v8 CI on v10x-staging and then we can re-run it here and land this, so this is blocked at the moment

@psmarshall
Copy link
Contributor Author

Blocked on: #24195

@richardlau richardlau added the blocked PRs that are blocked by other issues or PRs. label Nov 6, 2018
@psmarshall psmarshall removed the blocked PRs that are blocked by other issues or PRs. label Nov 7, 2018
@psmarshall
Copy link
Contributor Author

Original commit message:

  [cpu-profiler] Fix a leak caused by re-logging existing functions.

  Don't re-log all existing functions during StartProcessorIfNotStarted().
  They will already be in the CodeMap attached to the ProfileGenerator and
  re-logging them causes leaks. See the linked bug for more details.

  Bug: v8:8253
  Change-Id: Ibb1a1ab2431c588e8c3a3a9ff714767cdf61a88e
  Reviewed-on: https://chromium-review.googlesource.com/1256763
  Commit-Queue: Peter Marshall <petermarshall@chromium.org>
  Reviewed-by: Yang Guo <yangguo@chromium.org>
  Cr-Commit-Position: refs/heads/master@{#56336}
@psmarshall
Copy link
Contributor Author

Fixed up v8_embedder_string and rebased, running CI again (there was a wasm test flake)

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

@psmarshall
Copy link
Contributor Author

V8 CI is green, CI is 'unstable' but looks ok. Can this be landed? @MylesBorins

@MylesBorins
Copy link
Contributor

landed in 1e9b4a2

MylesBorins pushed a commit that referenced this pull request Nov 12, 2018
Original commit message:

  [cpu-profiler] Fix a leak caused by re-logging existing functions.

  Don't re-log all existing functions during StartProcessorIfNotStarted().
  They will already be in the CodeMap attached to the ProfileGenerator and
  re-logging them causes leaks. See the linked bug for more details.

  Bug: v8:8253
  Change-Id: Ibb1a1ab2431c588e8c3a3a9ff714767cdf61a88e
  Reviewed-on: https://chromium-review.googlesource.com/1256763
  Commit-Queue: Peter Marshall <petermarshall@chromium.org>
  Reviewed-by: Yang Guo <yangguo@chromium.org>
  Cr-Commit-Position: refs/heads/master@{#56336}

PR-URL: #23827
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@codebytere codebytere mentioned this pull request Nov 27, 2018
rvagg pushed a commit that referenced this pull request Nov 28, 2018
Original commit message:

  [cpu-profiler] Fix a leak caused by re-logging existing functions.

  Don't re-log all existing functions during StartProcessorIfNotStarted().
  They will already be in the CodeMap attached to the ProfileGenerator and
  re-logging them causes leaks. See the linked bug for more details.

  Bug: v8:8253
  Change-Id: Ibb1a1ab2431c588e8c3a3a9ff714767cdf61a88e
  Reviewed-on: https://chromium-review.googlesource.com/1256763
  Commit-Queue: Peter Marshall <petermarshall@chromium.org>
  Reviewed-by: Yang Guo <yangguo@chromium.org>
  Cr-Commit-Position: refs/heads/master@{#56336}

PR-URL: #23827
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
Original commit message:

  [cpu-profiler] Fix a leak caused by re-logging existing functions.

  Don't re-log all existing functions during StartProcessorIfNotStarted().
  They will already be in the CodeMap attached to the ProfileGenerator and
  re-logging them causes leaks. See the linked bug for more details.

  Bug: v8:8253
  Change-Id: Ibb1a1ab2431c588e8c3a3a9ff714767cdf61a88e
  Reviewed-on: https://chromium-review.googlesource.com/1256763
  Commit-Queue: Peter Marshall <petermarshall@chromium.org>
  Reviewed-by: Yang Guo <yangguo@chromium.org>
  Cr-Commit-Position: refs/heads/master@{#56336}

PR-URL: #23827
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@codebytere codebytere mentioned this pull request Nov 29, 2018
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.

10 participants