TakeSnapshot takes too long #34826

Activity

1. hyj1991 commented on Aug 18, 2020

   

   hyj1991

   on Aug 18, 2020

   Author

   More actions

   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.
2. hyj1991 commented on Aug 18, 2020

   

   hyj1991

   on Aug 18, 2020

   Last edited by hyj1991

   Author

   More actions

   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?
3. bnoordhuis commented on Aug 18, 2020

   

   bnoordhuis

   on Aug 18, 2020

   Member

   More actions

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

   🎉React with 🎉2Reacted by 易容 (hyj1991) and Juan José
4. bnoordhuis

   added

   v8 engineIssues and PRs related to the V8 dependency.Issues and PRs related to the V8 dependency.

   on Aug 18, 2020
5. hyj1991 commented on Aug 19, 2020

   

   hyj1991

   on Aug 19, 2020

   Author

   More actions

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

   🎉React with 🎉3Reacted by mary marchini, zhangsir and Juan José
6. gengjiawen commented on Sep 7, 2021

   

   gengjiawen

   on Sep 7, 2021

   Member

   More actions

   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
7. Trott commented on Sep 20, 2021

   

   Trott

   on Sep 20, 2021

   Member

   More actions

   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!
8. Trott

   closed this as completedon Sep 20, 2021