Skip to content

Remove the getBoundingClientRect call from SampleGraph#3604

Merged
canova merged 2 commits into
firefox-devtools:mainfrom
canova:canvas-getboundingclientrect
Oct 12, 2021
Merged

Remove the getBoundingClientRect call from SampleGraph#3604
canova merged 2 commits into
firefox-devtools:mainfrom
canova:canvas-getboundingclientrect

Conversation

@canova

@canova canova commented Oct 11, 2021

Copy link
Copy Markdown
Member

Partially fixes #3345

In #3345, @mstange was suggesting that we were doing a lot of layout thrashing. It turned out that we were doing this thrashing because of the getBoundingClientRect in the SampleGraph component. So I removed this call by wrapping that component with a withSize.

There are some snapshot changes, I believe they are changed because we do the layout thrashing differently now and the order got changed. But I don't see anything out of the ordinary in the snapshots.

Here are two profiles with and without this patch, so we can see the difference in the layout thrashing:

Before this PR
After this PR

Note that we still have some getBoundingClientRect calls in the "after". But this comes from the OverflowEdgeIndicator component. I think we should also remove the calls from that component, but that's more complex, because we have two getBoundingClientRect calls in there for different divs.

@canova canova requested a review from julienw October 11, 2021 11:21
@codecov

codecov Bot commented Oct 11, 2021

Copy link
Copy Markdown

Codecov Report

Merging #3604 (d315721) into main (e75ee8a) will not change coverage.
The diff coverage is 100.00%.

❗ Current head d315721 differs from pull request most recent head 91e523b. Consider uploading reports for the commit 91e523b to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##             main    #3604   +/-   ##
=======================================
  Coverage   88.95%   88.95%           
=======================================
  Files         257      257           
  Lines       21352    21352           
  Branches     5451     5451           
=======================================
  Hits        18994    18994           
  Misses       2182     2182           
  Partials      176      176           
Impacted Files Coverage Δ
src/components/shared/thread/SampleGraph.js 95.74% <100.00%> (ø)
src/actions/receive-profile.js 89.42% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e75ee8a...91e523b. Read the comment docs.

@julienw julienw left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks, this looks good to me!
From profiles I gathered, this saves some time.

I think the remaining issue isn't about the remaining getBoundingClientRect but that WithSize calls offsetWidth... We need to find a way to compute the widths before setting something in the DOM, instead of interleaving writes and reads...

@canova

canova commented Oct 12, 2021

Copy link
Copy Markdown
Member Author

Thanks for the review!

I think the remaining issue isn't about the remaining getBoundingClientRect but that WithSize calls offsetWidth... We need to find a way to compute the widths before setting something in the DOM, instead of interleaving writes and reads...

Yeah, we can think about how to do it.

@canova canova force-pushed the canvas-getboundingclientrect branch from d315721 to 91e523b Compare October 12, 2021 08:15
@canova canova enabled auto-merge October 12, 2021 08:16
@canova canova merged commit c736a1e into firefox-devtools:main Oct 12, 2021
@canova canova deleted the canvas-getboundingclientrect branch October 12, 2021 08:35
@canova canova added this to the Timeline with many threads milestone Oct 12, 2021
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.

With a large number of tracks, the initial load is very slow

2 participants