Skip to content

Fix various performance problems with power graphs and counters in general#4428

Merged
julienw merged 5 commits into
firefox-devtools:mainfrom
julienw:fix-rerendering-problem-with-counters
Jan 18, 2023
Merged

Fix various performance problems with power graphs and counters in general#4428
julienw merged 5 commits into
firefox-devtools:mainfrom
julienw:fix-rerendering-problem-with-counters

Conversation

@julienw
Copy link
Copy Markdown
Contributor

@julienw julienw commented Jan 18, 2023

STR:

  1. Open this profile
  2. Just hover with the mouse in the timeline

=> This is painfully slow.

There are many problems:

  • (unfixed) the power graphs in this profile are very costly, I believe this is because the profile itself is very long. It probably isn't a good idea to draw every counter values when we're zoomed out, but I didn't want to tackle this. The same problem probably exists in some other graphs such as the memory graph.
  • commit 1: since Fix the incorrect first counters on the graphs when zoomed in #4114, the counter object was renewed at each state change, which triggered a full rerender of many of our counter-based graphs. This is even more visible since Using mouseTimePosition in Selection.js and added tests for that #3000 landed. I fixed this by simply using createSelector to memoize the result.
  • commit 3: the power graphs didn't use the intersection observer, so even if out of view they would costly render. Note: other default tracks still don't have it, such as the network track. Non-default tracks such as the process CPU or event delay tracks also don't have it. Ideally the InView should be moved above, maybe at the LocalTrack or GlobalTrack location, I'd like to look at it eventually.
  • commit 4: not a problem by itself but I took this opportunity to remove the requestAnimationFrame calls. My understanding is that this is not needed in recent versions of React as the render mechanism is throttled by React already, so this introduces an additional unwanted delay. I already removed it in some other places in the past.

The commit 2 is a simple tool I used to discover which props were changed. I think it's useful to keep this in the codebase for whenever we need to debug react rerender issues.

production
deploy preview

Tell me what you think!

@codecov
Copy link
Copy Markdown

codecov Bot commented Jan 18, 2023

Codecov Report

Base: 88.56% // Head: 88.52% // Decreases project coverage by -0.04% ⚠️

Coverage data is based on head (73cd01b) compared to base (a2bb2b0).
Patch coverage: 60.00% of modified lines in pull request are covered.

❗ Current head 73cd01b differs from pull request most recent head 5f03759. Consider uploading reports for the commit 5f03759 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4428      +/-   ##
==========================================
- Coverage   88.56%   88.52%   -0.04%     
==========================================
  Files         282      283       +1     
  Lines       25494    25508      +14     
  Branches     6866     6869       +3     
==========================================
+ Hits        22578    22582       +4     
- Misses       2709     2717       +8     
- Partials      207      209       +2     
Impacted Files Coverage Δ
src/utils/react.js 0.00% <0.00%> (ø)
src/components/timeline/TrackPowerGraph.js 93.00% <93.33%> (-0.48%) ⬇️
src/selectors/profile.js 95.66% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@julienw julienw requested a review from canova January 18, 2023 10:33
Copy link
Copy Markdown
Contributor

@mstange mstange left a comment

Choose a reason for hiding this comment

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

This all looks good to me.

Copy link
Copy Markdown
Member

@canova canova left a comment

Choose a reason for hiding this comment

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

Thanks for the fix!

Comment thread src/utils/react.js
// render() {
// logPropChanges(this);
// }
export function logPropChanges(component: any) {
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.

Oh nice, this will come in handy in the future!

@julienw julienw enabled auto-merge January 18, 2023 17:25
@julienw julienw merged commit b6bba99 into firefox-devtools:main Jan 18, 2023
Sign up for free to 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.

3 participants