Compute the profiling start and end time for chrome tracing profiles #5187
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5187 +/- ##
=======================================
Coverage 88.56% 88.57%
=======================================
Files 308 308
Lines 28011 28025 +14
Branches 7578 7585 +7
=======================================
+ Hits 24809 24823 +14
Misses 2985 2985
Partials 217 217 ☔ View full report in Codecov by Sentry. |
|
Thanks for the reviews!
Good question, I completely forgot about this one. But it looks like this is changing the |
tenderlove
added a commit
to tenderlove/profiler
that referenced
this pull request
Nov 7, 2024
* main: (173 commits) Profile format changes for flows and stack-based markers Update all Yarn dependencies (2024-11-06) (firefox-devtools#5189) Add new tests for title changes for the startTime and public annotation Do not include the profile startTime in window title if it's zero Compute the profiling start and end time for chrome tracing profiles (firefox-devtools#5187) Do not clip the favicons in call tree (firefox-devtools#5185) Make sure that the symbolicator-cli builds properly Remove the unnecessary check after making favicon non-null Add default icons for tab selector pages and extensions that don't have favicons Update all Yarn dependencies (2024-10-30) (firefox-devtools#5181) Update the Firefox versions that introduces the webchannel Add a test for the favicons webchannel retrieval path Upgrade the webchannel mock to version 4 Add comment explaining webchannel version 4 Make the favicons fetching and symbolication run in parallel Update a comment to explain a possible path Use ArrayBuffer instead of using TypedArray directly Remove the unneeded image mock Get the binary favicon data directly and batch the icon loading for data urls Use a static icon counter for favicon classes ...
Merged
canova
added a commit
that referenced
this pull request
Nov 12, 2024
[Nazım Can Altınova] Change the test-build-coverage script to use 'yarn test' instead of jest directly (#5180) [Nazım Can Altınova] Retrieve the page favicons from the browser and display it on the tab selector (#5166) [Nazım Can Altınova] Add default favicons to extensions and pages that don't have nay favicons (#5182) [Nazım Can Altınova] Do not clip the favicons in call tree (#5185) [Nazım Can Altınova] Compute the profiling start and end time for chrome tracing profiles (#5187) [Nazım Can Altınova] Do not include the profile startTime in window title if it's zero (#5188) [Markus Stange] Profile format changes for flows and stack-based markers (#5186) [Nazım Can Altınova] Fix some svg images for Chrome by replacing context-fill with the black color (#5201) [Nazım Can Altınova] Enable the tab selector (#5197) [Nazım Can Altınova] Do not make the parent process visible by default when something is selected in the tab selector (#5198)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously we were adding the
profilingStartTimeandprofilingEndTimeonly when we were importing aCpuProfile:profiler/src/profile-logic/import/chrome.js
Lines 680 to 685 in e0ab0b6
But we weren't doing it for the other tracing profiles because it wasn't as straightforward. This proved to be a problem when I was looking at the profiles where I captured using our chrome extension, but it's also the same when I capture a profile in the Chrome's performance tab and then import it.
For example look at this profile here: https://share.firefox.dev/3AieK8R
Even though the profile is only like 1.5-2 seconds long, I'm getting a 1m 12s profile because of some rouge tracing events from before the profiler has started. It looks like a bug in how chrome streams these tracing values.
If you import the same profile to Chrome, it actually finds the correct time range. Here's the raw trace file, you can try it yourself on your Chrome:
Trace.json
So this PR changes the algorithm to look for the
TracingStartedInBrowserevent and use its start time as theprofilingStartTime. We don't have an event for theprofilingEndTime. So I'm using the same logic we use to determine that. It didn't have a problem before so it makes sense to keep that the same.Example profiles (of the trace I linked above): Before / After
You can also use the example trace.json profile to import it yourself
For testing it looks like we only have snapshot tests for chrome tracing. It updated some profiles, but also it kept the older
profilingStartTimes andprofilingEndTimes frmo CpuProfiles. So I think this is enough coverage for it as well.