TakeSnapshot takes too long

[hyj1991]

Reproduce:

• git clone https://github.com/hyj1991/midway-showcase.git
• npm install
• npm run dev

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]

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]

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]

@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.

[hyj1991]

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

[gengjiawen]

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]

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!