Closed Bug 1462584 Opened Last year Closed Last year

Avoid dereferencing DisplayItemData in GetOldLayerForFrame

Categories

(Core :: Web Painting, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox62 --- wontfix
firefox63 --- fixed

People

(Reporter: jnicol, Assigned: jnicol)

References

Details

Attachments

(1 file)

It's causing cache misses which are showing up in profiles of the displaylist mutate test case.

Rather than calling item->Disconnected(), we can make DisplayItemData::Disconnect responsible for ensuring the item no longer points to the data.

Rather than calling data->mLayer->Manager() != mRetainingManager, we can store a pointer to the manager along with the DisplayItemData on the nsDisplayItem. The item should be in the cache so grabbing this pointer to perform this check shouldn't cost us anything.
Comment on attachment 8976956 [details]
Bug 1462584 - Avoid dereferencing DisplayItemData in GetOldLayerForFrame.

https://reviewboard.mozilla.org/r/245116/#review251024

::: layout/painting/FrameLayerBuilder.cpp
(Diff revision 1)
>      (mRetainingManager->GetUserData(&gLayerManagerUserData));
>  
>    RefPtr<DisplayItemData> data =
>      new (aItem->Frame()->PresContext()) DisplayItemData(lmd, aItem->GetPerFrameKey(), aLayer);
>  
> -  if (!data->HasMergedFrames()) {

I'm not sure we want to remove this, but it was causing test failures. (layout/reftests/invalidation/inactive-layertree-visible-region-*)

By calling this, one of the DisplayItemDatas would overwrite the other's mItem pointer to null. Which would then cause ComputeGeometryChangeForItem to invalidate it a frame too late. Or something like that, I should have written it down when I had my rr session open.
Comment on attachment 8976956 [details]
Bug 1462584 - Avoid dereferencing DisplayItemData in GetOldLayerForFrame.

https://reviewboard.mozilla.org/r/245116/#review252416

::: layout/painting/FrameLayerBuilder.cpp:5086
(Diff revision 1)
>      data->mInactiveManager = tempManager;
>      // We optimized this PaintedLayer into a ColorLayer/ImageLayer. Store the optimized
>      // layer here.
>      if (aLayer != layer) {
>        data->mOptLayer = aLayer;
> -      data->mItem = nullptr;
> +      if (data->mItem) {

It's not obvious why we need to do this at all. Seems like we're clearing our cache on the item for no reason.

Should be relatively rare though, so maybe not worth breaking a green try run.

::: layout/painting/FrameLayerBuilder.cpp:5194
(Diff revision 1)
>  {
>    if (aData) {
>      if (!aData->mUsed) {
> +      if (!aData->HasMergedFrames() && aState != LayerState::LAYER_INACTIVE &&
> +          aState != LayerState::LAYER_SVG_EFFECTS && !GetContainingPaintedLayerData()) {
> +        aItem->SetDisplayItemData(aData, aLayer->Manager());

Do you remember why we only store on the item for the outermost DIDs?

Oh, I guess this is blocking us from storing DID when there are two (one for the inactive item in the active PaintedLayer, and one for 'active' item within the inactive layer).

I think we should be ok storing the DID for the actual inner leaf items, but detecting which those are might be hard.

Can we drop the 'GetContainingPaintedLayerData' check, so that we store the inner DID when there are two, and store the only DID for leaf items within the inactive layer?

That'd make the cache work for the inners, and the layer manager validity check should stop us using the wrong one for the paired DIDs.

Then again, inactive layers are on their way out, so maybe it doesn't matter.

Please add a comment at least :)

::: layout/painting/FrameLayerBuilder.cpp
(Diff revision 1)
>      (mRetainingManager->GetUserData(&gLayerManagerUserData));
>  
>    RefPtr<DisplayItemData> data =
>      new (aItem->Frame()->PresContext()) DisplayItemData(lmd, aItem->GetPerFrameKey(), aLayer);
>  
> -  if (!data->HasMergedFrames()) {

Hmm, this one seems a bit bad.

When we first create items, the initial attempt to call SetDisplayItemData (from MakeDisplayItem) will fail, since they haven't been through FLB yet, and it doesn't exist.

When we recreate the item on a future paint then it should work.

For items that don't get recreated (are retained via retained-dl), then nothing will ever call SetDisplayItemData on them, and we won't get the cache benefits.

I don't quite understand your comment, you say 'one of the DisplayItemDatas', why are there multiple? Is this the same as the above branch where we can be called twice for an inactive item (inner and outer), and we need to make sure we're only handling it once (or neither, as long as it's only for inactives).
Comment on attachment 8976956 [details]
Bug 1462584 - Avoid dereferencing DisplayItemData in GetOldLayerForFrame.

https://reviewboard.mozilla.org/r/245116/#review252416

> Hmm, this one seems a bit bad.
> 
> When we first create items, the initial attempt to call SetDisplayItemData (from MakeDisplayItem) will fail, since they haven't been through FLB yet, and it doesn't exist.
> 
> When we recreate the item on a future paint then it should work.
> 
> For items that don't get recreated (are retained via retained-dl), then nothing will ever call SetDisplayItemData on them, and we won't get the cache benefits.
> 
> I don't quite understand your comment, you say 'one of the DisplayItemDatas', why are there multiple? Is this the same as the above branch where we can be called twice for an inactive item (inner and outer), and we need to make sure we're only handling it once (or neither, as long as it's only for inactives).

This one was supposed to be marked as an issue, oops. This is the main one I'm worried about, since caching behaviour might be worse under some retained-dl workloads.
Priority: -- → P2
Blocks: 1468426
Comment on attachment 8976956 [details]
Bug 1462584 - Avoid dereferencing DisplayItemData in GetOldLayerForFrame.

https://reviewboard.mozilla.org/r/245116/#review257076
Attachment #8976956 - Flags: review?(matt.woodrow) → review+
Pushed by jnicol@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2ea7e45aaaaf
Avoid dereferencing DisplayItemData in GetOldLayerForFrame. r=mattwoodrow
Pushed by jnicol@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6179611c6ced
Avoid dereferencing DisplayItemData in GetOldLayerForFrame. r=mattwoodrow
https://hg.mozilla.org/mozilla-central/rev/6179611c6ced
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Flags: needinfo?(jnicol)
You need to log in before you can comment on or make changes to this bug.