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

Add V8 cross-thread profiling patches #2227

Closed

Conversation

harrishancock
Copy link
Collaborator

V8's profiler does not expect isolates to be locked from different threads. Pull request #710 moved inspector protocol handling to a separate thread, meaning profiling was started and stopped on the inspector thread, not the Worker's request connection thread. If I remember right, the result of this is that the profiler ends up sampling the inspector thread, which doesn't normally run JavaScript, and causes a bunch of "(program)" placeholder spans to show up in the profile.

We previously discovered this issue with our internal runtime, which has always been heavily multi-threaded, and patched around the problem. This commit copies the two relevant patches from our internal repo.

Fixes #1754.

V8's profiler does not expect isolates to be locked from different threads. Pull request #710 moved inspector protocol handling to a separate thread, meaning profiling was started and stopped on the inspector thread, not the Worker's request connection thread. If I remember right, the result of this is that the profiler ends up sampling the inspector thread, which doesn't normally run JavaScript, and causes a bunch of "(program)" placeholder spans to show up in the profile.

We previously discovered this issue with our internal runtime, which has always been heavily multi-threaded, and patched around the problem. This commit copies the two relevant patches from our internal repo.

Fixes #1754.
@harrishancock harrishancock requested review from a team as code owners June 6, 2024 16:59
@kentonv
Copy link
Member

kentonv commented Jun 6, 2024

This seems a bit surprising -- the inspector protocol seems explicitly designed to run from a different thread, in order to support breakpoint debugging (which suspends the execution thread?).

(But hey if it works, ship it.)

@harrishancock
Copy link
Collaborator Author

I guess our patches are Linux-specific:

Mac:

external/v8/src/libsampler/sampler.cc:604:11: error: use of undeclared identifier 'SYS_tgkill'
  syscall(SYS_tgkill, platform_data()->tgid(), lockedTid, SIGPROF);
          ^

Windows failed for a separate, probably spuriuos reason about an unexpected clang version, which I think @jasnell mentioned in chat. Presumably it won't appreciate SYS_tgkill, too, though.

C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.40.33807\include\yvals_core.h(898,1): error: static assertion failed: error STL1000: Unexpected compiler version, expected Clang 17.0.0 or newer.

@harrishancock
Copy link
Collaborator Author

This seems a bit surprising -- the inspector protocol seems explicitly designed to run from a different thread, in order to support breakpoint debugging (which suspends the execution thread?).

It looks like it suspends execution by calling this runMessageLoopOnPause() function, which then explicitly pumps the inspector message loop until V8 tells it to stop:

// This method is called by v8 when a breakpoint or debugger statement is hit. This method
// processes debugger messages until `Debugger.resume()` is called, when v8 then calls
// `quitMessageLoopOnPause()`.
//
// This method is ultimately called from the `InspectorChannelImpl` and the isolate lock is
// held when this method is called.

@kentonv
Copy link
Member

kentonv commented Jun 10, 2024

Could we perhaps actually deliver the profiler start/stop messages over to the isolate thread, rather than trying to start the profiler cross-thread?

@harrishancock
Copy link
Collaborator Author

Could we perhaps actually deliver the profiler start/stop messages over to the isolate thread, rather than trying to start the profiler cross-thread?

This is done in #2497.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🐛 Bug Report — CPU profiling reports entire execution as (program)
3 participants