Closed Bug 1449608 Opened 6 years ago Closed 6 years ago

FrameLayerBuilder spends too much time computing ScrollMetadata

Categories

(Core :: Web Painting, enhancement, P3)

enhancement

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.
Priority: -- → P3
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 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 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
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.
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
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
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)
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
https://hg.mozilla.org/mozilla-central/rev/e9b1d02e24f0
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.