Closed
Bug 1449608
Opened 6 years ago
Closed 6 years ago
FrameLayerBuilder spends too much time computing ScrollMetadata
Categories
(Core :: Web Painting, enhancement, P3)
Core
Web Painting
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: jnicol, Assigned: jnicol)
References
(Blocks 1 open bug, )
Details
Attachments
(1 file)
On the displaylist mutate test, with active layers, I'm seeing about 20% of layer building time in ContainerState::SetupScrollingMetadata and ScrollFrameHelper::ComputeScrollMetadata. In this test it's calculating the exact same thing for each layer, so we should be able to avoid a lot of this. We call ContainerState::SetupScrollingMetadata for each NewLayerEntry, which calls ScrollFrameHelper::ComputeScrollMetadata for each ActiveScrolledRoot in the entry's chain. If we look at the arguments to ComputeScrollMetadata: * The scroll frame itself depends on the ASR so will vary. It definitely affects the calculation. * aLayer is different for each NewLayerEntry. But it is only used for a cheap calculation when async scrolling is disabled. * aLayerManager I think might be constant for each ContainerState, but in any case is only used for logging. * aContainerReferenceFrame and aParameters are constant for each ContainerState. * aClip will also vary and affects the calculation. So with that in mind a think we can 1) Refactor the cheap non-async-scrolling portion of ScrollFrameHelper::ComputeScrollMetadata in to a different function which still gets called as frequently as before. 2) Within ContainerState, cache the result of ScrollFrameHelper::ComputeScrollMetadata for each ASR and DisplayItemClip pair.
Updated•6 years ago
|
Priority: -- → P3
Comment hidden (mozreview-request) |
Assignee | ||
Comment 2•6 years ago
|
||
I've gone for a conservative approach of only caching the most recent result from ComputeScrollMetadata. So if there is more than one ASR in the clip chain, or the layers have different clips, then this will not give us any benefit. I considered using a map or vector to cache results for every ASR and DisplayItemClip combination. But worried that there might be more overhead from doing that. I'm not sure about the name for the function I've split out, so I based it on what the comment said. Very open to better suggestions. I also don't really understand the conditions in that function so just copied them straight over, I don't know if they can be simplified or need changed.
Comment 3•6 years ago
|
||
mozreview-review |
Comment on attachment 8969581 [details] Bug 1449608 - Avoid calling ComputeScrollMetadata repeatedly for same scroll frame and clip. https://reviewboard.mozilla.org/r/238344/#review244486 ::: layout/painting/FrameLayerBuilder.cpp:5468 (Diff revision 1) > + mCachedScrollMetadata = Some(CachedScrollMetadata()); > + mCachedScrollMetadata->mASR = asr; > + mCachedScrollMetadata->mClip = clip; > + mCachedScrollMetadata->mMetadata = metadata; Does this work? mCachedScrollMetadata.emplace({ asr, clip, metadata });
Attachment #8969581 -
Flags: review?(mstange) → review+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 5•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8969581 [details] Bug 1449608 - Avoid calling ComputeScrollMetadata repeatedly for same scroll frame and clip. https://reviewboard.mozilla.org/r/238344/#review244486 > Does this work? > mCachedScrollMetadata.emplace({ asr, clip, metadata }); The compiler didn't seem to like that, but I've added a constructor and replaced this code with an emplace call.
Pushed by jnicol@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b443e56d2f62 Avoid calling ComputeScrollMetadata repeatedly for same scroll frame and clip. r=mstange
Comment 7•6 years ago
|
||
Backed out for build bustages and automation.py mass failures Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=b443e56d2f623eee3834c7fafe7c58d7d24405d1 Failure log: Build bustage: https://treeherder.mozilla.org/logviewer.html#?job_id=175179496&repo=autoland&lineNumber=41067 and https://treeherder.mozilla.org/logviewer.html#?job_id=175179505&repo=autoland&lineNumber=32464 Automation.py failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=175185163&repo=autoland&lineNumber=1974 Backout: https://hg.mozilla.org/integration/autoland/rev/ee291d63749722cfc51215f24476fc1d6783f8ea
Flags: needinfo?(jnicol)
Assignee | ||
Comment 8•6 years ago
|
||
Ah, unlike with std::optional, mozilla::Maybe::emplace should not be called if isSome. Sorry about the bustage. I think I'll just remove the Maybe, anyway. mCachedScrollMetadata::mASR == nullptr can be used to indicate when there is no cached metadata.
Comment hidden (mozreview-request) |
Comment 10•6 years ago
|
||
Pushed by jnicol@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/544e2832e783 Avoid calling ComputeScrollMetadata repeatedly for same scroll frame and clip. r=mstange
Comment 11•6 years ago
|
||
Backed out for bustages "xul.dll : fatal error LNK1120: 1 unresolved externals" Push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=544e2832e78306fc04c7cbaea81a40b0189ae155 Log: https://treeherder.mozilla.org/logviewer.html#?job_id=175375780&repo=autoland&lineNumber=40006
Comment 12•6 years ago
|
||
Backout by nbeleuzu@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/7b4cbe358d21 Backed out changeset 544e2832e783 for bustages "xul.dll : fatal error LNK112". CLOSED TREE
Assignee | ||
Comment 13•6 years ago
|
||
Really not having much luck with this one! I believe there is a missing " = 0" after the declaration of the new function nsIScrollableFrame::ClipLayerToDisplayPort. Which strangely only causes an issue on our windows builds. Will reland tomorrow (after triple checking it actually works!!)
Flags: needinfo?(jnicol)
Comment hidden (mozreview-request) |
Comment 15•6 years ago
|
||
Pushed by jnicol@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e9b1d02e24f0 Avoid calling ComputeScrollMetadata repeatedly for same scroll frame and clip. r=mstange
Comment 16•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e9b1d02e24f0
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in
before you can comment on or make changes to this bug.
Description
•