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

TakeSnapshot takes too long #34826

Closed
hyj1991 opened this issue Aug 18, 2020 · 6 comments
Closed

TakeSnapshot takes too long #34826

hyj1991 opened this issue Aug 18, 2020 · 6 comments
Labels
v8 engine Issues and PRs related to the V8 dependency.

Comments

@hyj1991
Copy link

hyj1991 commented Aug 18, 2020

Reproduce:

It will run require('v8').getHeapSnapshot() automaticlly after 5s. And in this case, getHeapSnapshot takes too long with this empty application.

Node-version: v12.8.3
Platform: Darwin

@hyj1991
Copy link
Author

hyj1991 commented Aug 18, 2020

https://github.com/nodejs/node/blob/master/deps/v8/src/profiler/heap-snapshot-generator.cc#L575-L576

In fact, I think this is because when obtaining loactions during Takesnapshot, GetLineNumber / GetColumnNumber will trigger too many GetPositionInfoSlow in some cases.

@hyj1991
Copy link
Author

hyj1991 commented Aug 18, 2020

Could we change the ExtractLocationForJSFunction

void V8HeapExplorer::ExtractLocationForJSFunction(HeapEntry* entry,
                                                  JSFunction func) {
  if (!func.shared().script().IsScript()) return;
  Script script = Script::cast(func.shared().script());
  int scriptId = script.id();
  int start = func.shared().StartPosition();
  int line = script.GetLineNumber(start);
  int col = script.GetColumnNumber(start);
  snapshot_->AddLocation(entry, scriptId, line, col);
}

as

void V8HeapExplorer::ExtractLocationForJSFunction(HeapEntry* entry,
                                                  JSFunction func) {
  if (!func.shared().script().IsScript()) return;
  Script script = Script::cast(func.shared().script());
  int scriptId = script.id();
  int start = func.shared().StartPosition();

  // this is for setting script line ends before getting script position info to
  // prevent fallback to GetPositionInfoSlow.
  i::Handle<i::Script> script2(i::Script::cast(script), func.shared().GetIsolate());
  int line = script.GetLineNumber(script2, start);
  int col = script.GetColumnNumber(script2, start);

  snapshot_->AddLocation(entry, scriptId, line, col);
}

to speed up TakeSnapshot?

@bnoordhuis
Copy link
Member

@hyj1991 Fixes to V8 should be sent to the upstream project (see https://v8.dev/docs/contribute) but if it's strictly a performance improvement (no observable changes except speed), it'll almost certainly get accepted.

We can back-port the change to Node.js after it's merged upstream.

@bnoordhuis bnoordhuis added the v8 engine Issues and PRs related to the V8 dependency. label Aug 18, 2020
@hyj1991
Copy link
Author

hyj1991 commented Aug 19, 2020

https://chromium-review.googlesource.com/c/v8/v8/+/2362413

@gengjiawen
Copy link
Member

I get an graphql application with 300mb memory takes 50m to get the result ...

    CPU: (1) x64 AMD EPYC 7601 32-Core Processor
    Memory: 154.01 MB / 1.93 GB

@Trott
Copy link
Member

Trott commented Sep 20, 2021

I'm going to optimistically close this, but if that's wrong and there's still an issue here in Node.js, please comment or re-open. Thanks!

@Trott Trott closed this as completed Sep 20, 2021
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

No branches or pull requests

4 participants