Compute profiling time range from samples and markers#715
Compute profiling time range from samples and markers#715nordzilla wants to merge 1 commit intomstange:mainfrom
Conversation
6241cfe to
a06eab1
Compare
This commit adds `profilingStartTime` and `profilingEndTime` fields to the profile metadata, computed from all sample timestamps and marker start/end times across all threads.
a06eab1 to
a16f715
Compare
mstange
left a comment
There was a problem hiding this comment.
Oh, I actually had a different change in mind: I think Profile should have set_profiling_start/end_time methods, and then samply can just set the right timestamps that way, and nothing has to be computed in an extra pass.
|
Thanks for the feedback! I should have time to look into it tomorrow morning. EDIT: Didn't have as much time this morning as I thought. I'll get to this as soon as I can. |
|
Thanks! Here are some more specifics on how I think we should get the timestamps:
|
UpdateI've been looking into this a bit. I'm onboarding onto a lot more samply-internal context than, for example, emitting marker files in #710, or doing a min/max through the processed data (as was done in my first implementation attempt for this PR). Even when I get this PR up, it will probably be macOS and Linux only. I don't have a Windows dev machine to look into and test those internals. |
Overview
This PR adds
profilingStartTimeandprofilingEndTimefields to the profile metadata, computed from all sample timestamps and marker start/end times across all threads.Tests
I've updated all of the existing tests to check these values, but it doesn't have full coverage of the edge cases I've encountered, where marker times may be before/after sample times, or the converse, on multiple threads, etc.
I did manually verify that it fixes the issues I was seeing in #710 related to the profiling time range.
Example Before: https://share.firefox.dev/4oSRnGW
Example After: https://share.firefox.dev/4p5vUux
However, I'm not entirely sure if the current behavior is correct for every scenario.
For example, I'm not sure if the following start and end time of
10.0, which is the computed result for one of the current test cases, makes sense:I didn't add any additional test cases because I'm still very much a novice when it comes to the internals of the profiles. I bet an LLM could generate more test cases pretty well, given the context, but I wouldn't be able to accurately review whether it represents a realistic profile.
I'm not sure how extensive you want the test coverage, since these cases are pretty long, but I'm happy to try to add more cases if desired.
Additional Thoughts
I considered utilizing rayon for the
compute_profiling_time_range()function, because it seems like a natural use case, and probably would drastically reduce the time to process large profiles. However, I didn't want to introduce a new dependency to this crate without asking.Related Work
This PR was implemented as an alternative to firefox-devtools/profiler#5682.