Use the meta.profilingStartTime and meta.profilingEndTime fields#4648
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #4648 +/- ##
==========================================
+ Coverage 88.48% 88.62% +0.14%
==========================================
Files 295 294 -1
Lines 26266 26116 -150
Branches 7082 7050 -32
==========================================
- Hits 23241 23145 -96
+ Misses 2814 2766 -48
+ Partials 211 205 -6
☔ View full report in Codecov by Sentry. |
| thread, | ||
| jsTracer, | ||
| meta.categories | ||
| meta.categories || [] |
There was a problem hiding this comment.
This looks spurious (and will still fail in this function), can you please revert this change?
There was a problem hiding this comment.
This was just to silence flow errors. If I don't add the ProfileMeta type annotation on const meta: ProfileMeta, flow fails when I assign values to meta.profilingStartTime = because the profilingStartTime field doesn't exist on the literal object 'meta'. With the meta object annotated to 'ProfileMeta', flow is unhappy on this line because the categories field is optional in ProfileMeta, but convertJsTracerToThread won't take undefined.
Given that I'm not actually changing the behavior and that afaik jsTracer is dead code at this point, I thought this way of silencing the error was fine. Do you see another nicer way?
There was a problem hiding this comment.
I see :)
You can use geckoProfile.meta.categories directly here, and Flow will be happy.
| // When the recording started (in milliseconds after startTime). | ||
| profilingStartTime?: Milliseconds, | ||
| // When the recording ended (in milliseconds after startTime). | ||
| profilingEndTime?: Milliseconds, |
There was a problem hiding this comment.
It could be good to repeat these comments in the gecko-profile type file too
| expect(processedMeta.sampleUnits).toEqual(geckoMeta.sampleUnits); | ||
| }); | ||
|
|
||
| it('keeps the profilingStartTime and profilingEndTime', function () { |
| title += _formatDateTime(meta.startTime); | ||
| title += _formatDateTime( | ||
| meta.startTime + (meta.profilingStartTime || 0) | ||
| ); |
fqueze
force-pushed
the
useProfilingStartTime
branch
from
3f3b2c3 to
f8963ef
Compare
…ecide the profile root range.
fqueze
force-pushed
the
useProfilingStartTime
branch
from
f8963ef to
2e5e256
Compare
3 participants
They can be used to decide what the profile root range should be. This fixes #4425 and handles most of #3458 (which can be refocused on what remains, ie. using
meta.contentEarliestTimeto show if/when the buffer overflowed).Additionally, I tweaked the Profile Info panel to show when the profiler started and for how long it recorded, instead of showing when the parent process started. I'm also showing the uptime of the application when the profiler started (I think that information is more useful than the process start time).
Requesting review from Julien as I already put multiple PRs in Nazim's review queue, but I'm equally happy if Nazim wants to take it.