-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
src: enable detailed source positions in V8 #24274
Conversation
Original commit message: [profiler] introduce API to enable detailed source positions This allows Node.js to enable detailed source positions for optimized code early on, without having to pass a flag string. [email protected] Change-Id: Ie74ea41f600cf6e31acbe802116df4976ccf1c75 Reviewed-on: https://chromium-review.googlesource.com/c/1319757 Commit-Queue: Yang Guo <[email protected]> Reviewed-by: Peter Marshall <[email protected]> Cr-Commit-Position: refs/heads/master@{#57380}
@@ -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); |
There was a problem hiding this comment.
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)?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
Seems like we got a few timeouts on SmartOS. How should we proceed with this PR? |
They might be flakes https://ci.nodejs.org/job/node-test-pull-request/18574/ |
Looks like some other tests failed this time. |
Original commit message: [profiler] introduce API to enable detailed source positions This allows Node.js to enable detailed source positions for optimized code early on, without having to pass a flag string. [email protected] Change-Id: Ie74ea41f600cf6e31acbe802116df4976ccf1c75 Reviewed-on: https://chromium-review.googlesource.com/c/1319757 Commit-Queue: Yang Guo <[email protected]> Reviewed-by: Peter Marshall <[email protected]> Cr-Commit-Position: refs/heads/master@{#57380} PR-URL: nodejs#24274 Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Peter Marshall <[email protected]> Reviewed-By: Ali Ijaz Sheikh <[email protected]> Reviewed-By: Matheus Marchini <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
PR-URL: nodejs#24274 Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Peter Marshall <[email protected]> Reviewed-By: Ali Ijaz Sheikh <[email protected]> Reviewed-By: Matheus Marchini <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
Landed in c1d18a0...e2a8e32 |
Original commit message: [profiler] introduce API to enable detailed source positions This allows Node.js to enable detailed source positions for optimized code early on, without having to pass a flag string. [email protected] Change-Id: Ie74ea41f600cf6e31acbe802116df4976ccf1c75 Reviewed-on: https://chromium-review.googlesource.com/c/1319757 Commit-Queue: Yang Guo <[email protected]> Reviewed-by: Peter Marshall <[email protected]> Cr-Commit-Position: refs/heads/master@{#57380} PR-URL: #24274 Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Peter Marshall <[email protected]> Reviewed-By: Ali Ijaz Sheikh <[email protected]> Reviewed-By: Matheus Marchini <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
PR-URL: #24274 Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Peter Marshall <[email protected]> Reviewed-By: Ali Ijaz Sheikh <[email protected]> Reviewed-By: Matheus Marchini <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
Original commit message: [profiler] introduce API to enable detailed source positions This allows Node.js to enable detailed source positions for optimized code early on, without having to pass a flag string. [email protected] Change-Id: Ie74ea41f600cf6e31acbe802116df4976ccf1c75 Reviewed-on: https://chromium-review.googlesource.com/c/1319757 Commit-Queue: Yang Guo <[email protected]> Reviewed-by: Peter Marshall <[email protected]> Cr-Commit-Position: refs/heads/master@{#57380} PR-URL: nodejs#24274 Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Peter Marshall <[email protected]> Reviewed-By: Ali Ijaz Sheikh <[email protected]> Reviewed-By: Matheus Marchini <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
PR-URL: nodejs#24274 Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Peter Marshall <[email protected]> Reviewed-By: Ali Ijaz Sheikh <[email protected]> Reviewed-By: Matheus Marchini <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
Original commit message: [profiler] introduce API to enable detailed source positions This allows Node.js to enable detailed source positions for optimized code early on, without having to pass a flag string. [email protected] Change-Id: Ie74ea41f600cf6e31acbe802116df4976ccf1c75 Reviewed-on: https://chromium-review.googlesource.com/c/1319757 Commit-Queue: Yang Guo <[email protected]> Reviewed-by: Peter Marshall <[email protected]> Cr-Commit-Position: refs/heads/master@{#57380} PR-URL: #24274 Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Peter Marshall <[email protected]> Reviewed-By: Ali Ijaz Sheikh <[email protected]> Reviewed-By: Matheus Marchini <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
PR-URL: #24274 Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Peter Marshall <[email protected]> Reviewed-By: Ali Ijaz Sheikh <[email protected]> Reviewed-By: Matheus Marchini <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
The commits from this PR were reverted in #24394 (V8 tests were broken). Please open a new pull request to get them back in. N.B.: I didn't add the |
Original commit message: [profiler] introduce API to enable detailed source positions This allows Node.js to enable detailed source positions for optimized code early on, without having to pass a flag string. [email protected] Change-Id: Ie74ea41f600cf6e31acbe802116df4976ccf1c75 Reviewed-on: https://chromium-review.googlesource.com/c/1319757 Commit-Queue: Yang Guo <[email protected]> Reviewed-by: Peter Marshall <[email protected]> Cr-Commit-Position: refs/heads/master@{#57380} Refs: v8/v8@073073b PR-URL: #24515 Refs: #24274 Refs: #24394 Refs: #24393 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Peter Marshall <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>
PR-URL: #24515 Refs: #24274 Refs: #24394 Refs: #24393 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Peter Marshall <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>
Original commit message: [profiler] introduce API to enable detailed source positions This allows Node.js to enable detailed source positions for optimized code early on, without having to pass a flag string. [email protected] Change-Id: Ie74ea41f600cf6e31acbe802116df4976ccf1c75 Reviewed-on: https://chromium-review.googlesource.com/c/1319757 Commit-Queue: Yang Guo <[email protected]> Reviewed-by: Peter Marshall <[email protected]> Cr-Commit-Position: refs/heads/master@{#57380} Refs: v8/v8@073073b PR-URL: #24515 Refs: #24274 Refs: #24394 Refs: #24393 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Peter Marshall <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>
PR-URL: #24515 Refs: #24274 Refs: #24394 Refs: #24393 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Peter Marshall <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>
Original commit message: [profiler] introduce API to enable detailed source positions This allows Node.js to enable detailed source positions for optimized code early on, without having to pass a flag string. [email protected] Change-Id: Ie74ea41f600cf6e31acbe802116df4976ccf1c75 Reviewed-on: https://chromium-review.googlesource.com/c/1319757 Commit-Queue: Yang Guo <[email protected]> Reviewed-by: Peter Marshall <[email protected]> Cr-Commit-Position: refs/heads/master@{#57380} Refs: v8/v8@073073b PR-URL: #24515 Refs: #24274 Refs: #24394 Refs: #24393 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Peter Marshall <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>
PR-URL: #24515 Refs: #24274 Refs: #24394 Refs: #24393 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Peter Marshall <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>
Original commit message: [profiler] introduce API to enable detailed source positions This allows Node.js to enable detailed source positions for optimized code early on, without having to pass a flag string. [email protected] Change-Id: Ie74ea41f600cf6e31acbe802116df4976ccf1c75 Reviewed-on: https://chromium-review.googlesource.com/c/1319757 Commit-Queue: Yang Guo <[email protected]> Reviewed-by: Peter Marshall <[email protected]> Cr-Commit-Position: refs/heads/master@{#57380} Refs: v8/v8@073073b PR-URL: nodejs#24515 Refs: nodejs#24274 Refs: nodejs#24394 Refs: nodejs#24393 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Peter Marshall <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>
Original commit message: [profiler] introduce API to enable detailed source positions This allows Node.js to enable detailed source positions for optimized code early on, without having to pass a flag string. [email protected] Change-Id: Ie74ea41f600cf6e31acbe802116df4976ccf1c75 Reviewed-on: https://chromium-review.googlesource.com/c/1319757 Commit-Queue: Yang Guo <[email protected]> Reviewed-by: Peter Marshall <[email protected]> Cr-Commit-Position: refs/heads/master@{#57380} Refs: v8/v8@073073b PR-URL: nodejs#24515 Refs: nodejs#24274 Refs: nodejs#24394 Refs: nodejs#24393 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Peter Marshall <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>
Original commit message: [profiler] introduce API to enable detailed source positions This allows Node.js to enable detailed source positions for optimized code early on, without having to pass a flag string. [email protected] Change-Id: Ie74ea41f600cf6e31acbe802116df4976ccf1c75 Reviewed-on: https://chromium-review.googlesource.com/c/1319757 Commit-Queue: Yang Guo <[email protected]> Reviewed-by: Peter Marshall <[email protected]> Cr-Commit-Position: refs/heads/master@{#57380} Refs: v8/v8@073073b PR-URL: nodejs#24515 Refs: nodejs#24274 Refs: nodejs#24394 Refs: nodejs#24393 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Peter Marshall <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>
PR-URL: nodejs#24515 Refs: nodejs#24274 Refs: nodejs#24394 Refs: nodejs#24393 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Peter Marshall <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>
Original commit message: [profiler] introduce API to enable detailed source positions This allows Node.js to enable detailed source positions for optimized code early on, without having to pass a flag string. [email protected] Change-Id: Ie74ea41f600cf6e31acbe802116df4976ccf1c75 Reviewed-on: https://chromium-review.googlesource.com/c/1319757 Commit-Queue: Yang Guo <[email protected]> Reviewed-by: Peter Marshall <[email protected]> Cr-Commit-Position: refs/heads/master@{#57380} Refs: v8/v8@073073b PR-URL: nodejs#24515 Refs: nodejs#24274 Refs: nodejs#24394 Refs: nodejs#24393 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Peter Marshall <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>
Enable detailed source positions in generated code. When profiling, this gives a better mapping between profile samples to source text.
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes