Skip to content

Fix the compare view when using the same profile in both entries - #5469

Merged
julienw merged 2 commits into
firefox-devtools:mainfrom
julienw:fix-compare-view-with-identical-profiles
May 22, 2025
Merged

Fix the compare view when using the same profile in both entries#5469
julienw merged 2 commits into
firefox-devtools:mainfrom
julienw:fix-compare-view-with-identical-profiles

Conversation

@julienw

Copy link
Copy Markdown
Contributor

I use this often to test the compare view, because it's easier. So I'd like this edge case to work.

@julienw
julienw requested a review from canovaMay 21, 2025 17:13
@julienw
julienwforce-pushed the fix-compare-view-with-identical-profiles branch from 91af327 to 63fbde3CompareMay 21, 2025 17:15
@codecov

codecovBot commented May 21, 2025

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 86.03%. Comparing base (ea9787b) to head (63fbde3).
Report is 2 commits behind head on main.

Files with missing linesPatch %Lines
src/profile-logic/merge-compare.js50.00%1 Missing ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #5469 +/- ##
==========================================
- Coverage 86.04% 86.03% -0.01% 
==========================================
Files 312 312 Lines 30356 30358 +2 Branches 8294 8295 +1 ==========================================
+ Hits 26119 26120 +1 - Misses 3642 3643 +1 
Partials 595 595 

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@canovacanova left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good to me overall, but I'm surprised to see how it can cause an issue in the comparison view. Don't we usually use the committed range for the compare profiles? Which part fails when we have these values?

@julienw

Copy link
Copy Markdown
ContributorAuthor

The compare function adjusts all timestamps so that the start of the profile is effectively "0". All samples and markers have now a value that is relative to "0". But without this patch we were copying the existing value for these meta properties.

Then we use this information from the meta object when it's present in

export function getTimeRangeIncludingAllThreads(
profile: Profile
): StartEndRange {
constcompleteRange={start: Infinity,end: -Infinity};
if(
profile.meta.profilingStartTime!==undefined&&
profile.meta.profilingEndTime
){
return{
start: profile.meta.profilingStartTime,
end: profile.meta.profilingEndTime,
};
}

instead of computing the range from the threads data.

And that's where it breaks!

@julienw
julienw enabled auto-merge (squash) May 22, 2025 17:14
@julienw
julienw merged commit 09b1e75 into firefox-devtools:mainMay 22, 2025
@julienwjulienw mentioned this pull request Jun 10, 2025
julienw added a commit that referenced this pull request Jun 10, 2025
Here are the changes:
[Julien Wajsberg] Fix the compare view when using the same profile in both entries (#5469)
[Nazım Can Altınova] Change the tests that check co2 values so they don't fail on every co2 dependency update (#5475)
[Julien Wajsberg] Ignore the Symbol Store database cache when the user presses the resymbolication button (#5479)
[Markus Stange] In the profile format, store a shared stringArray for all threads (#5481)
Sign up for freeto 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.

2 participants

@julienw@canova