1330570 - Allocate DisplayItemData in the presshell arena

Status: RESOLVED FIXED

(Core :: Web Painting, defect)

Description

[Markus Stange [:mstange]]

We access these a lot during layer building. Moving them to the presshell arena might improve cache locality.

DisplayItemData objects are reference counted. They are stored in an nsTArray which is stored as a frame property. For best results, we also need to allocate the nsTArray and its internal buffers in the arena, as well as the FramePropertyTable (bug 1330558).

Comment 1

[Bas Schouten (:bas.schouten)]

Attachment: Move DisplayItemData into the PresShell arena

Here's a first stab at this, I still need to do some solid profiling to provide evidence for its efficacy. Let me know what you think.

Comment 2

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8827218 [details] [diff] [review]
Move DisplayItemData into the PresShell arena

Review of attachment 8827218 [details] [diff] [review]:
-----------------------------------------------------------------

Seems totally reasonable. Looking forward to perf data!

Comment 3

[Markus Stange [:mstange]]

Comment on attachment 8827218 [details] [diff] [review]
Move DisplayItemData into the PresShell arena

Review of attachment 8827218 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good! This won't help with display list building times (because display list building doesn't use DisplayItemData), but it should help with layer building times.

The AssertDisplayItemData stuff is instrumentation that was supposed to help me debug a crash (bug 1141089) but I never figured out what was happening. Maybe you have ideas, now that you've looked at this code a bit? In any case, we have enough crash reports to tell us that sAliveDisplayItemDatas->Contains(aData) is the check that's failing, so we could just remove the instrumentation. That should help get the cache misses down.
In fact, I'd be surprised if this patch helps perf on its own as long as AssertDisplayItemData still exists.

Comment 4

[Markus Stange [:mstange]]

Comment on attachment 8827218 [details] [diff] [review]
Move DisplayItemData into the PresShell arena

Review of attachment 8827218 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/painting/FrameLayerBuilder.h
@@ +72,5 @@
> +    return aPresContext->PresShell()->
> +      AllocateByObjectID(eArenaObjectID_DisplayItemData, sz);
> +  }
> +
> +  // These two methods are for use by ArenaRefPtr.

It looks like the rest of this patch just uses regular RefPtrs, and not ArenaRefPtr. Have you found out what ArenaRefPtr is for?

Comment 5

[Bas Schouten (:bas.schouten)]

(In reply to Markus Stange [:mstange] from comment #4)
> Comment on attachment 8827218 [details] [diff] [review]
> Move DisplayItemData into the PresShell arena
> 
> Review of attachment 8827218 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/painting/FrameLayerBuilder.h
> @@ +72,5 @@
> > +    return aPresContext->PresShell()->
> > +      AllocateByObjectID(eArenaObjectID_DisplayItemData, sz);
> > +  }
> > +
> > +  // These two methods are for use by ArenaRefPtr.
> 
> It looks like the rest of this patch just uses regular RefPtrs, and not
> ArenaRefPtr. Have you found out what ArenaRefPtr is for?

I think I did but I no longer remember.. I think it had something to do with allocation and deallocation policy :s.

Comment 6

[Bas Schouten (:bas.schouten)]

Attachment: Bug 1330570: Allocate DisplayItemData into the PresShell Arena.

Review commit: https://reviewboard.mozilla.org/r/137108/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/137108/

Comment 7

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8865438 [details]
Bug 1330570: Allocate DisplayItemData into the PresShell Arena.

https://reviewboard.mozilla.org/r/137108/#review140338

r=me with either using ArenaRefPtr, or dropping support for it.

::: layout/painting/FrameLayerBuilder.h:73
(Diff revision 1)
> +  void Invalidate() { mIsInvalid = true; }
> +  void ClearAnimationCompositorState();
> +
> +  static DisplayItemData* AssertDisplayItemData(DisplayItemData* aData);
> +
> +  void* operator new(size_t sz, nsPresContext* aPresContext)

Seems like most of this new code is basically identical boilterplate to what's in nsStyleContext. Might be worth moving it to a #define so it can be shared?

::: layout/painting/FrameLayerBuilder.cpp:137
(Diff revision 1)
>    GetMaskLayerImageCache()->Sweep();
>    MOZ_COUNT_DTOR(FrameLayerBuilder);
>  }
>  
> -FrameLayerBuilder::DisplayItemData::DisplayItemData(LayerManagerData* aParent, uint32_t aKey,
> +DisplayItemData::DisplayItemData(LayerManagerData* aParent, uint32_t aKey,
>                                                      Layer* aLayer, nsIFrame* aFrame)

Alignment

::: layout/painting/FrameLayerBuilder.cpp:219
(Diff revision 1)
>  }
>  
>  void
> -FrameLayerBuilder::DisplayItemData::BeginUpdate(Layer* aLayer, LayerState aState,
> +DisplayItemData::BeginUpdate(Layer* aLayer, LayerState aState,
>                                                  uint32_t aContainerLayerGeneration,
>                                                  nsDisplayItem* aItem /* = nullptr */)

Alignment here too.

::: layout/painting/FrameLayerBuilder.cpp:396
(Diff revision 1)
>     */
>    LayerManager *mLayerManager;
>  #ifdef DEBUG_DISPLAY_ITEM_DATA
>    LayerManagerData *mParent;
>  #endif
> -  nsTHashtable<nsRefPtrHashKey<FrameLayerBuilder::DisplayItemData> > mDisplayItems;
> +  nsTHashtable<nsRefPtrHashKey<DisplayItemData> > mDisplayItems;

We could use ArenaRefPtr here instead of normal RefPtr.

ArenaRefPtr is a 'weak' pointer, that clears its reference if the arena gets destroyed while there are still outstanding references to items within it.

That should never actually happen though, since we should always destroy all frames first, and the frame dtor should clean up any DisplayItemData objects.

If we're not going to use ArenaRefPtr though, we should probably switch to PRES_ARENA_OBJECT_WITHOUT_ARENAREFPTR_SUPPORT and go back to normal refcounting and not pay for the extra tracking.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9483bb6d8ce6
Allocate DisplayItemData into the PresShell Arena. r=mattwoodrow

Comment 9

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/df9358a0469a
Backed out changeset 9483bb6d8ce6 for test failures in test_selection_move_commands.html | cmd_scrollBottom - -300 should equal -300

Comment 10

[Carsten Book [:Tomcat]]

backed out in https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=df9358a0469ad540a31aedbd4307ed9f5c3cd532 for causing https://treeherder.mozilla.org/logviewer.html#?job_id=97980542&repo=mozilla-inbound

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/129ab771d0ad
Allocate DisplayItemData into the PresShell Arena. r=mattwoodrow

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/129ab771d0ad