Skip to content

src: enable detailed source positions in V8 #24274

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.

Sign up for GitHub

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

Closed
hashseed wants to merge 2 commits into from
Closed

src: enable detailed source positions in V8 #24274

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
dc102e8
deps: backport 073073b4f1 from upstream V8
hashseed Nov 9, 2018
aa32194
src: enable detailed source positions in V8
hashseed Nov 9, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
src: enable detailed source positions in V8
  • Loading branch information
@hashseed
hashseed committed Nov 9, 2018
commit aa3219437647c2445bf00d30105b4bcb397e2c90
1 change: 1 addition & 0 deletions src/node.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2548,6 +2548,7 @@ Isolate* NewIsolate(ArrayBufferAllocator* allocator, uv_loop_t* event_loop) {
isolate->SetMicrotasksPolicy(MicrotasksPolicy::kExplicit);
isolate->SetFatalErrorHandler(OnFatalError);
isolate->SetAllowWasmCodeGenerationCallback(AllowWasmCodeGenerationCallback);
v8::CpuProfiler::UseDetailedSourcePositionsForProfiling(isolate);
Copy link
Copy Markdown
Contributor

@refack refack Nov 9, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: does this have a penalty? And even if not could this be conditional on inspector being attached or profiler run requested (I mean is it possible)?

Copy link
Copy Markdown
Contributor

@psmarshall psmarshall Nov 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem with only enabling this once the profiler run is requested is that functions that get optimized before profiling starts don't have the detailed line information. In the browser you can work around this by reloading the page with profiling enabled (devtools has a button for this) but for a running Node instance this is not really feasible, particularly if you want to debug some rarely-occurring thing. Unfortunately we see this in the wild quite a bit so having it on by default makes sense IMO.

The overhead is very low - on average it makes no difference, it the worst case a few percent increase in optimized compile time, but this is currently being moved off-thread anyway (plus the general performance boost of upgrading the V8 version is going to easily outweigh this).

Copy link
Copy Markdown
Contributor

@refack refack Nov 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation.

Copy link
Copy Markdown
Contributor

@refack refack Nov 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the browser you can work around this by reloading the page with profiling enabled

I'm thinking about this some more...
For node we have one main way to enable inspetor, via CLI args. There are two other ways which I don't have usage information about SIGUSR and inspector.open().

It might be a better balance to turn this on only for processes started with inspector listening, and save the performance cost for regular runs. IIUC the down side is less accurate code position information?

I'm ambivalent...

/CC @nodejs/performance

Copy link
Copy Markdown
Member

@Flarna Flarna Nov 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CpuProfiler can be also used without inspector, see e.g. NPM modules like v8-profiler. There are tools out in the wild which start profiling automatically for some time if they notice performance issues.

Copy link
Copy Markdown
Contributor

@psmarshall psmarshall Nov 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's also possible to use the CPU profiler directly through the V8 API without the inspector and there are some native modules that do this - that approach wouldn't work for them as far as I understand

Copy link
Copy Markdown
Member Author

@hashseed hashseed Nov 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The thoughts leading to this were:

  • It's hard to foresee whether the user is going to profile, both for Chrome and Node.
  • For Chrome, you can at least reload, with DevTools open.
  • Node.js does not have the reload option.
  • Adding more accurate data has a small one-time cost for the script. Chrome suffers a bit more for this as you pay more for optimizing compiles, possibly while starting up a page, and this occurs on every page navigation. Node.js does not encounter new scripts to compile and optimize as often.

Copy link
Copy Markdown
Contributor

@refack refack Nov 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that it's a tough call:
slightly slower compile time but for everyone ⚖️ improvement not available but only for profiling started out-of-band


return isolate;
}
Expand Down