-
Notifications
You must be signed in to change notification settings - Fork 240
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
Support multithreaded profiling #353
base: main
Are you sure you want to change the base?
Conversation
Hey 👋 Good work! It seems that you still need to insert the |
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 sending this over. It's certainly ambitious! But it would be a good improvement to get this working. I'm still a bit confused about how this works. Do you have a sample script that illustrates how it's supposed to work?
@dataclass | ||
class StackSamplerSubscriberTarget: | ||
call_stack: SubscriberCallstackFn | ||
event: SubscriberEventFn |
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 wonder if we can combine this event and async_state change concepts into one interface. It seems that they're pretty similar to me.
def record_thread_start(self, thread_id: str, time: float) -> None: | ||
if not self.thread_start_times: | ||
self.first_start_time = time | ||
self.thread_start_times[thread_id] = time - self.first_start_time |
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.
Why not store the raw start time in each, rather than the offset from the first_start_time
? Generally I find it better to store truth over computed values.
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 initially tried raw time but it had some complexity when trying to view the traces on the same view, i'd be happy to look at that again.
) | ||
if event == 'thread_start': | ||
self._active_session.record_thread_start(thread_id, time) | ||
|
||
# pylint: disable=W0613 | ||
def _sampler_saw_call_stack( |
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'm confused why this method _sampler_saw_call_stack
doesn't need to change? Don't we need to separate out the storage of the different threads?
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 frame records get a root thread id added which allows us to reconstruct what is separate - this is done in build_call_stack
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.
(different threads will never clash in the pythons code due to the GIL)
Done!
Done! |
As per #352 I have modified pyinstrument to support multiple threads.
It does this by separating out both timing and frames for the different threads - as such it modifies a substantial amount of the data model (usually in the form of
List[frames]
->Dict[str, List[Frames]]
and the equivalents in typescript.I don't do much typescript so it's possible there are prettier ways of doing the modifications on that side.
I'd be interested in discussing how this looks to you