Closed Bug 795674 Opened 7 years ago Closed 7 years ago

Fix DisplayItemData storage in FrameLayerBuilder

Categories

(Core :: Layout, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla19
Tracking Status
firefox18 + fixed

People

(Reporter: mattwoodrow, Assigned: mattwoodrow)

References

Details

Attachments

(5 files, 5 obsolete files)

The current code is fragile and complex, I'm sure we can do better. I haven't really thought through how yet, need to spend some time on this.

Currently each LayerManager stores a list of frames (and associated display items) that were drawn into it. Each of those frames keeps a reference to this LayerManager data in a frame property.

Inactive layers are stored with the data for the display item that created it, giving us a full tree structure of all items/layers/managers involved in the paint.

In the case where a frame has multiple layer managers (inactive ContainerLayers cause this), then the frame stores a reference to the outermost LayerManager. We require that all references to a frame live within a single subtree so that when a frame is destroyed, we can prune the subtree and remove all dangling pointers to the frame.

This breaks down when we have merged frames because this is trying to keep references to a subtree from multiple frames.
Blocks: 795591
I think it's important we reinstate the scrolllayer underlying frame swap until  we have code that deals with merged frames properly. Patch incoming.
Assignee: chrislord.net → matt.woodrow
Sorry about the size of this, ended up being a little more complicated than I expected.

I also added as much validation code as I could to try catch issues early.

I can't reproduce any crashes with this, probably missed a few test cases though. Would appreciate it if anyone else could test this out and see if anything shows up.

It's more or less what was proposed. I didn't make the frame data store nsRefPtr's, since that seemed like it would just hide bugs by leaking memory instead. Better to crash early and fix it, I think.
Attachment #669325 - Flags: review?(roc)
Comment on attachment 669325 [details] [diff] [review]
Implement the new DisplayItemData storage

This looks like it has regressed invalidation really badly, looking in to it.
Attachment #669325 - Flags: review?(roc)
Fixed a bug in FrameListMatches.
Attachment #669325 - Attachment is obsolete: true
Attachment #669356 - Flags: review?(roc)
We might also want to attempt to retain content when the frame list for a merged display item changes.

This might be something you want to look at Chris? I don't really understand the display item merging code, and am not sure under what circumstances we can retain content.

You'd want to look at all callers of FrameListMatches(). Happy to help out with this.
Comment on attachment 669356 [details] [diff] [review]
Implement the new DisplayItemData storage v2

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

My high-level comment here is, why did you decide to have LayerManagerData have a hashtable mapping (frame x key) -> (DisplayItemData x mContainerLayerGeneration)? I thought LayerManagerData could just have a hash-set of DisplayItemData pointers, and I still think so. Everywhere we have a frame + key, we can look up the frame's list of DisplayItemDatas and find the DisplayItemData that way. Can't we?

::: layout/base/FrameLayerBuilder.cpp
@@ +29,5 @@
>  #include "nsTransitionManager.h"
>  
>  #ifdef DEBUG
>  #include <stdio.h>
> +#define DEBUG_INVALIDATIONS

comment out DEBUG_INVALIDATIONS

@@ +101,5 @@
> +    if (mFrameList[i] == sDestroyedFrame) {
> +      continue;
> +    }
> +    nsTArray<DisplayItemData*> *array = 
> +      reinterpret_cast<nsTArray<DisplayItemData*>*>(mFrameList[i]->Properties().Get(GetDescriptorForManager(nullptr)));

Seems like we always pass nullptr to GetDescriptorForManager. Why do we even need separate properties for the different layer managers now? We should be able to store all the DisplayItemDatas in a single array even if they're for different layer managers, and GetDescriptorForManager can just go away.

@@ +106,5 @@
> +    if (!array) {
> +      continue;
> +    }
> +
> +    NS_ABORT_IF_FALSE(!array->Contains(this), "Must have removed ourselves from the frames!");

This entire function body should be #ifdef DEBUG, right?

@@ +120,5 @@
> +  nsTArray<DisplayItemData*> *array = 
> +    reinterpret_cast<nsTArray<DisplayItemData*>*>(GetKey().mFrame->Properties().Get(GetDescriptorForManager(nullptr)));
> +  if (array) {
> +    NS_ABORT_IF_FALSE(!array->Contains(mData), "Must have removed ourselves from the frames!");
> +  }

The entire body of this function can be #ifdef DEBUG too, looks like.

@@ +196,5 @@
>     * Tracks which frames have layers associated with them.
>     */
> +  LayerManager *mLayerManager;
> +  LayerManagerData *mParent;
> +  nsTHashtable<FrameLayerBuilder::DisplayItemHashData> mDisplayItems;

Why do we need to hash these by frame/display item key? I was hoping we wouldn't have to.

@@ +787,5 @@
>      return nullptr;
>    }
>  
> +  DisplayItemKey key(aFrame, aKey);
> +  DisplayItemHashData *hash = data->mDisplayItems.GetEntry(key);

This could use the frame's property and search for the item with the given key and the right layer manager, instead of looking up mDisplayItems.

::: layout/base/FrameLayerBuilder.h
@@ +482,5 @@
>  protected:
>    /**
>     * We store an array of these for each frame that is associated with
>     * one or more retained layers. Each DisplayItemData records the layer
>     * used to render one of the frame's display items.

This comment needs to be fixed.

@@ +484,5 @@
>     * We store an array of these for each frame that is associated with
>     * one or more retained layers. Each DisplayItemData records the layer
>     * used to render one of the frame's display items.
>     */
> +  class DisplayItemKey {

This needs a comment to describe what it's used for.

@@ +500,5 @@
> +    nsIFrame* mFrame;
> +    uint32_t mKey;
> +  };
> +
> +  class DisplayItemData : public PLDHashEntryHdr {

This needs a comment. In fact somewhere we need a big comment explaining how the storage works.

@@ +520,2 @@
>  
> +    void CopyInto(DisplayItemData* aDest);

Comment that this doesn't really copy, aDest steals and leaves this object invalid.

@@ +524,4 @@
>      nsRefPtr<Layer> mLayer;
>      nsRefPtr<Layer> mOptLayer;
>      nsRefPtr<LayerManager> mInactiveManager;
> +    uint32_t mDisplayItemKey;

move this down to be just above mContainerLayerGeneration

@@ +528,1 @@
>      nsAutoTArray<nsIFrame*, 2> mFrameList;

Why isn't this an nsAutoTArray of length 1, by the way?

@@ +541,3 @@
>    };
>  
> +  class DisplayItemHashData : public PLDHashEntryHdr {

This needs a comment too, to describe how it is used.

@@ +572,5 @@
> +                                       
> +    enum { ALLOW_MEMMOVE = false };
> +
> +    DisplayItemKey mKey;
> +    nsRefPtr<DisplayItemData> mData;

We don't need to store DisplayItemKey here. We can get away with just storing an nsIFrame*, since mData contains the display item type key.

::: layout/base/nsDisplayItemTypes.h
@@ +122,5 @@
> +  switch (aType) {
> +    case TYPE_ALT_FEEDBACK: return "TYPE_ALT_FEEDBACK";
> +    case TYPE_BACKGROUND: return "TYPE_BACKGROUND";
> +    case TYPE_BORDER: return "TYPE_BORDER";
> +    case TYPE_BOX_SHADOW_OUTER: return "TYPE_BOX_SHADOW_OUTER";

Put this code in its own patch.

Also I think it's time to do the nsDisplayItemTypeList.h trick with macros. So create nsDisplayItemTypeList.h containing
DECLARE_DISPLAY_ITEM_TYPE(ALT_FEEDBACK) etc and include it twice from nsDisplayItemTypes, once to create the enum and once to implement DisplayItemTypeName.

::: layout/base/nsLayoutUtils.cpp
@@ +4063,5 @@
>      if (!f->HasAnyStateBits(NS_FRAME_IN_POPUP)) {
> +      nsIFrame* temp = f->PresContext()->FrameManager()->GetRootFrame();
> +      if (!temp) {
> +        return f;
> +      }

Remove, as discussed.

::: xpcom/glue/nsHashKeys.h
@@ +202,5 @@
>    typedef const uint64_t& KeyType;
>    typedef const uint64_t* KeyTypePointer;
>    
>    nsUint64HashKey(KeyTypePointer aKey) : mValue(*aKey) { }
> +  nsUint64HashKey(KeyType aKey) : mValue(aKey) { }

Remove, as discussed.
Attachment #669356 - Flags: review?(roc) → review-
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #10)
> 
> My high-level comment here is, why did you decide to have LayerManagerData
> have a hashtable mapping (frame x key) -> (DisplayItemData x
> mContainerLayerGeneration)? I thought LayerManagerData could just have a
> hash-set of DisplayItemData pointers, and I still think so. Everywhere we
> have a frame + key, we can look up the frame's list of DisplayItemDatas and
> find the DisplayItemData that way. Can't we?

I misread hash-set as hash-table here, and that seemed like the logical key choice.

I'm not sure how the hash set would work for the purposes of StoreNewDisplayItemData/UpdateDisplayItemData, since all the new DisplayItemData's will be different pointers.

Unless we remove mNewDisplayItemData entirely and just modify a single copy. Not entirely sure if that would work though, especially for the component-alpha flattening unwinding process.
(In reply to Matt Woodrow (:mattwoodrow) from comment #11)
> I'm not sure how the hash set would work for the purposes of
> StoreNewDisplayItemData/UpdateDisplayItemData, since all the new
> DisplayItemData's will be different pointers.

Right. Maybe mNewDisplayItemData should contain DisplayItemHashDatas but 
LayerManagerData should just be the DisplayItemData pointers?
(In reply to Matt Woodrow (:mattwoodrow) from comment #9)
> We might also want to attempt to retain content when the frame list for a
> merged display item changes.
> 
> This might be something you want to look at Chris? I don't really understand
> the display item merging code, and am not sure under what circumstances we
> can retain content.
> 
> You'd want to look at all callers of FrameListMatches(). Happy to help out
> with this.

I think this is going to depend somewhat on the items being merged - do we lose layers any time the frame list changes now, or just when the underlying frame changes? I need to find the time to go over this code again - probably before the impending work week :)
Attachment #669356 - Attachment is obsolete: true
Attachment #670177 - Flags: review?(roc)
(In reply to Chris Lord [:cwiiis] from comment #13)
> I think this is going to depend somewhat on the items being merged - do we
> lose layers any time the frame list changes now, or just when the underlying
> frame changes?

Any time the frame list changes, with this patch.
Comment on attachment 670177 [details] [diff] [review]
Implement the new DisplayItemData storage v3

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

::: layout/base/FrameLayerBuilder.cpp
@@ +879,5 @@
> +      key.mFrame = data->mFrameList[j];
> +
> +      if (key.mFrame != aFrame) {
> +        nsTArray<DisplayItemData*> *array = 
> +          reinterpret_cast<nsTArray<DisplayItemData*>*>(key.mFrame->Properties().Get(LayerManagerDataProperty()));

This declaration of 'array' shadows the outer one, which is a bad idea. Rename it.

@@ +1001,5 @@
> +    for (uint32_t i = 0; i < data->mFrameList.Length(); i++) {
> +      nsIFrame* frame = data->mFrameList[i];
> +      nsTArray<DisplayItemData*> *array = 
> +        reinterpret_cast<nsTArray<DisplayItemData*>*>(frame->Properties().Get(LayerManagerDataProperty()));
> +      array->RemoveElement(data);

Let's have a helper function, perhaps a member of DisplayItemData, that walks through the frame list and removes the DisplayItemData from each frame's array (optionally skipping one frame passed in as a parameter). We can call it here and also above in RemoveFrameFromLayerManager.

@@ +1034,5 @@
> +    nsTArray<DisplayItemData*> *array = 
> +      reinterpret_cast<nsTArray<DisplayItemData*>*>(frame->Properties().Get(LayerManagerDataProperty()));
> +    // TODO: This is wasteful for merged display items since they will come up again in this iterator, and we've
> +    // already removed the data from the frames.
> +    array->RemoveElement(data);

We could call that function here too.

@@ +1058,5 @@
> +    array = new nsTArray<DisplayItemData*>();
> +    key.mFrame->Properties().Set(LayerManagerDataProperty(), array);
> +  }
> +  array->AppendElement(aEntry->mData);
> +  aEntry->mData = nullptr;

This assignment is not needed since we destroy aEntry immediately, right?

@@ +2230,5 @@
>    if (layerManager) {
> +    DisplayItemData* data = GetDisplayItemDataForManager(aItem, layerManager);
> +    if (data) {
> +      Layer* layer = data->mLayer;
> +      if (layer->Manager()->GetUserData(&gLayerManagerUserData)) {

How can layer->Manager() be different from layerManager?

@@ +2331,5 @@
>    }
>  }
>  
>  void
> +FrameLayerBuilder::StoreDataForFrame(nsDisplayItem* aItem, DisplayItemData* aData)

Make aData an already_AddRefed

@@ +2744,2 @@
>      if (entry) {
> +      entry->mData = did;

did.forget()

@@ +2752,5 @@
> +    }
> +    for (uint32_t i = 0; i < mergedFrames.Length(); ++i) {
> +      nsIFrame* mergedFrame = mergedFrames[i];
> +      DisplayItemKey key(mergedFrame, containerDisplayItemKey);
> +      entry = mNewDisplayItemData.PutEntry(key);

Declare a new variable to hold this

@@ +2754,5 @@
> +      nsIFrame* mergedFrame = mergedFrames[i];
> +      DisplayItemKey key(mergedFrame, containerDisplayItemKey);
> +      entry = mNewDisplayItemData.PutEntry(key);
> +      if (entry) {
> +        entry->mData = did;

reference entry->mData

::: layout/base/nsDisplayList.cpp
@@ +1018,5 @@
>      } else {
>        layerManager->BeginTransaction();
>      }
>    }
> +  if (widgetTransaction) {

Why did you change this?
Comment on attachment 670178 [details] [diff] [review]
Add DisplayItemData debugging code

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

::: layout/base/FrameLayerBuilder.cpp
@@ +30,5 @@
>  
>  #ifdef DEBUG
>  #include <stdio.h>
>  //#define DEBUG_INVALIDATIONS
> +//+#define DEBUG_DISPLAY_ITEM_DATA

Remove +

::: layout/base/nsDisplayItemTypes.h
@@ +17,2 @@
>  enum Type {
> +  TYPE_ZERO = 0, /** Spacer so that the first item starts at 0 */

"starts at 1"
Attachment #670178 - Flags: review?(roc) → review+
> 
> @@ +2744,2 @@
> >      if (entry) {
> > +      entry->mData = did;
> 
> did.forget()

Won't that leak?

> 
> 
> @@ +2754,5 @@
> > +      nsIFrame* mergedFrame = mergedFrames[i];
> > +      DisplayItemKey key(mergedFrame, containerDisplayItemKey);
> > +      entry = mNewDisplayItemData.PutEntry(key);
> > +      if (entry) {
> > +        entry->mData = did;
> 
> reference entry->mData

Not sure what you mean, entry->mData is an nsRefPtr, it should reference itself.

> 
> ::: layout/base/nsDisplayList.cpp
> @@ +1018,5 @@
> >      } else {
> >        layerManager->BeginTransaction();
> >      }
> >    }
> > +  if (widgetTransaction) {
> 
> Why did you change this?

I folded in the 'fix secondary layer manager code' patch. This was the only change that actually remained.
Fixed all comments except for the few that I replied to.
Attachment #670177 - Attachment is obsolete: true
Attachment #670177 - Flags: review?(roc)
Attachment #670249 - Flags: review?(roc)
Attached patch Remove mNewDisplayItemData (obsolete) — Splinter Review
Haven't tested this all that much yet, but it improves perf a lot and makes the code a lot cleaner too.
Attachment #670296 - Flags: feedback?(roc)
Comment on attachment 670249 [details] [diff] [review]
Implement the new DisplayItemData storage v4

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

::: layout/base/FrameLayerBuilder.cpp
@@ +889,5 @@
> +
> +    DisplayItemKey key(aFrame, data->mDisplayItemKey);
> +    uint32_t length = data->mFrameList.Length();
> +    for (uint32_t j = 0; j < length; j++) {
> +      key.mFrame = data->mFrameList[j];

Call data->RemoveFrameData(aFrame)? We certainly don't seem to need "key" here since we only use key.mFrame.

::: layout/base/nsDisplayList.cpp
@@ +1018,5 @@
>      } else {
>        layerManager->BeginTransaction();
>      }
>    }
> +  if (widgetTransaction) {

We don't use allowRetaining anymore. Please do a followup patch to remove it. (In which case nsIWidget::GetLayerManager can probably have that parameter removed.)
Attachment #670249 - Flags: review?(roc) → review+
(In reply to Matt Woodrow (:mattwoodrow) from comment #19)
> > @@ +2744,2 @@
> > >      if (entry) {
> > > +      entry->mData = did;
> > 
> > did.forget()
> 
> Won't that leak?

I don't see why it would. entry->mData is an nsRefPtr, so assigning did.forget() to it will just shift the pointer over without changing the refcount. The reference added in the assignment to 'did' would eventually be released by the destructor of entry->mData.

> > @@ +2754,5 @@
> > > +      nsIFrame* mergedFrame = mergedFrames[i];
> > > +      DisplayItemKey key(mergedFrame, containerDisplayItemKey);
> > > +      entry = mNewDisplayItemData.PutEntry(key);
> > > +      if (entry) {
> > > +        entry->mData = did;
> > 
> > reference entry->mData
> 
> Not sure what you mean, entry->mData is an nsRefPtr, it should reference
> itself.

Sorry, what I meant was, here you would create a new variable for the result of PutEntry, say mergedEntry, and write "mergedEntry->mData = entry->mData", which would addref.

This is a microoptimization, so not very important. You can do it in a separate patch if you want to, or not do it at all.
Comment on attachment 670296 [details] [diff] [review]
Remove mNewDisplayItemData

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

Looks fabulous!

::: layout/base/FrameLayerBuilder.cpp
@@ +2620,5 @@
> +    for (uint32_t i = 0; i < data->mFrameList.Length(); i++) {
> +      nsIFrame* frame = data->mFrameList[i];
> +      nsTArray<DisplayItemData*> *array = 
> +        reinterpret_cast<nsTArray<DisplayItemData*>*>(frame->Properties().Get(LayerManagerDataProperty()));
> +      array->RemoveElement(data);

This will happen automatically now, won't it?
It looks like this caused a tcheckerboard regression:

Regression :( Robocop Checkerboarding No Snapshot Benchmark increase 49.2% on Android 2.2 (Native) Mozilla-Inbound
------------------------------------------------------------------------------------------------------------------
   Previous: avg 2426.603 stddev 328.538 of 30 runs up to revision 41f527aaa8ba
   New     : avg 3620.954 stddev 225.195 of 5 runs since revision 0bf4db8c014c
   Change  : +1194.351 (49.2% / z=3.635)
   Graph   : http://mzl.la/Rk5jnQ
(In reply to Jeff Muizelaar [:jrmuizel] from comment #27)
> It looks like this caused a tcheckerboard regression:
> 
> Regression :( Robocop Checkerboarding No Snapshot Benchmark increase 49.2%
> on Android 2.2 (Native) Mozilla-Inbound
> -----------------------------------------------------------------------------
> -------------------------------------
>    Previous: avg 2426.603 stddev 328.538 of 30 runs up to revision
> 41f527aaa8ba
>    New     : avg 3620.954 stddev 225.195 of 5 runs since revision
> 0bf4db8c014c
>    Change  : +1194.351 (49.2% / z=3.635)
>    Graph   : http://mzl.la/Rk5jnQ

I guess this is somewhat expected, as this swap: http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.cpp#2902 will no longer have an effect?

We probably need to add exceptions to the frame list changing causing a whole-layer invalidation, but I'm not exactly sure of all the circumstances. If a frame is removed from the merged list, I'd have guessed that you'd want to invalidate its bounds (if they're still visible), but do we store bounds of old frames? Should we?

Without having read the code, that's my naive suggestion - store the display item bounds of merged items in the merged frames list and instead of invalidating the layer when a frame disappears, invalidate those bounds on the layer instead.

I'll be looking at invalidation again when I've finished with the current tiling stuff in gfx, but I think Matt or roc could comment more intelligently here.
(In reply to Chris Lord [:cwiiis] from comment #28)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #27)
> > It looks like this caused a tcheckerboard regression:
> > 
> > Regression :( Robocop Checkerboarding No Snapshot Benchmark increase 49.2%
> > on Android 2.2 (Native) Mozilla-Inbound
> > -----------------------------------------------------------------------------
> > -------------------------------------
> >    Previous: avg 2426.603 stddev 328.538 of 30 runs up to revision
> > 41f527aaa8ba
> >    New     : avg 3620.954 stddev 225.195 of 5 runs since revision
> > 0bf4db8c014c
> >    Change  : +1194.351 (49.2% / z=3.635)
> >    Graph   : http://mzl.la/Rk5jnQ
> 
> I guess this is somewhat expected, as this swap:
> http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.
> cpp#2902 will no longer have an effect?

To clarify, this isn't quite what I meant. This swap was there to mitigate the effect of the underlying frame of a scroll layer changing causing a full layer invalidation - This swap will still have that effect (stopping the underlying frame changing), but the merged frame list changing now forces an invalidation anyway, so it's pointless (as the underlying frame of a scroll layer is (almost?) always accompanied by a merged frame list change).
That doesn't sound unreasonable to me, but I don't know much about display item merging.

Do we have any desktop test cases that show this problem?

I can probably write a patch fairly quickly, I just don't know how to test it easily.
The test-case on bug 781516 used to trigger this case, I think. I'm not sure if (for whatever reason) it only triggered on mobile though.

If you don't mind a live site, my own web-site used to trigger it (chrislord.net) - when the Last.FM music panel on the right disappears, the underlying frame of the scroll layer changes (I suppose because it's an iframe or some such).
Depends on: 801350
> Do we have any desktop test cases that show this problem?

The "Runfield" demo - https://developer.mozilla.org/en-US/demos/detail/runfield - perhaps? It used to run quite smoothly for me under Linux, but in recent nightly builds I usually get about a frame every second or so. The CPU usage isn't unreasonable so I assume the page just isn't updated when it should be. I could be wrong (and if I am, I should file a new bug report), but I think it was the changes here that introduced the regression.

Using the pre-built mozilla-center snapshots, I got this regression range:

good: http://hg.mozilla.org/mozilla-central/rev/1301a72b1c39
bad:  http://hg.mozilla.org/mozilla-central/rev/90857937b601

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=1301a72b1c39&tochange=90857937b601

Compiling it myself, I got this:

good: http://hg.mozilla.org/mozilla-central/rev/9f28c28e988f
bad:  http://hg.mozilla.org/mozilla-central/rev/0586d2a97c52

(I don't know how to show that with a single URL.)
Hopefully fixed all the issues, including the problem with runfield.

https://tbpl.mozilla.org/?tree=Try&rev=ba5b54e55f51
Attachment #670296 - Attachment is obsolete: true
Attachment #670296 - Flags: feedback?(roc)
Attachment #671277 - Flags: review?(roc)
Comment on attachment 671277 [details] [diff] [review]
Remove mNewDisplayItemData

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

::: layout/base/FrameLayerBuilder.cpp
@@ +2636,1 @@
>      return PL_DHASH_REMOVE;

Add a comment explaining why it's OK to not restore old items that could have been overwritten.
Attachment #671277 - Flags: review?(roc) → review+
Unfortunately I can't reproduce the problem on either of those two pages Chris.

I've attached a WIP patch that should help a fair bit. It might crash randomly though, the RemoveFrameFromLayerManager code shouldn't be accessing the frame pointer.

I think we really want to store separate geometry objects for each of the merged sub-lists so that we can determine the changed area correctly, even without a frame pointer.
Depends on: 801568
Blocks: 801217
(In reply to Matt Woodrow (:mattwoodrow) from comment #36)
> Created attachment 671285 [details] [diff] [review]
> WIP patch for handling merged frame changes
> 
> Unfortunately I can't reproduce the problem on either of those two pages
> Chris.
> 
> I've attached a WIP patch that should help a fair bit. It might crash
> randomly though, the RemoveFrameFromLayerManager code shouldn't be accessing
> the frame pointer.
> 
> I think we really want to store separate geometry objects for each of the
> merged sub-lists so that we can determine the changed area correctly, even
> without a frame pointer.

Early to say, but I'll comment anyway - we're getting layers recreated on each scroll at the top of http://taskjs.org - I haven't debugged what the reason is yet, but it could well be this.
Confirmed that the patch fixes the repainting on taskjs.org.

I wouldn't say this is perfect, since we're not actually using the geometries of the merged display items.

It would probably be more correct to keep the merged display lists separately under nsDisplayOpacity, compute and retain separate geometries for each one and then use those to determine the invalidation region. But I think this should work fine for most cases.

Note that we still invalidate the entire combined area if one of the merged frames is deleted, but that seems like it would be a relatively rare case.

https://tbpl.mozilla.org/?tree=Try&rev=dcd4506c35d6
Attachment #671285 - Attachment is obsolete: true
Attachment #671705 - Flags: review?(roc)
Comment on attachment 671705 [details] [diff] [review]
Handle merged frame changes better

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

::: layout/base/FrameLayerBuilder.cpp
@@ +1088,5 @@
>    if (array) {
>      for (uint32_t i = 0; i < array->Length(); i++) {
>        DisplayItemData* item = array->ElementAt(i);
>        if (item->mDisplayItemKey == aItem->GetPerFrameKey() &&
> +          item->mLayer->Manager() == aManager) {

So what happens if a DisplayItemData with key K has two frames A and B and in the next paint, A and B have two non-merged display items both with key K? Seems like we could have a problem because they'd both hit the same DisplayItemData and we'd try to share it when we shouldn't?

::: layout/base/FrameLayerBuilder.h
@@ +502,5 @@
>       * to the LayerManagerDataProperty list on the frame.
>       */
>      void AddFrame(nsIFrame* aFrame);
> +    void RemoveFrame(nsIFrame* aFrame);
> +    void GetFrameListChanges(nsDisplayItem* aOther, nsTArray<nsIFrame*>& aOut);

Needs a comment. I guess "Sets aOut to the list of frames that have been added to or removed from the display item"?
(In reply to Matt Woodrow (:mattwoodrow) from comment #39)
> Note that we still invalidate the entire combined area if one of the merged
> frames is deleted, but that seems like it would be a relatively rare case.

Where does that happen?
> So what happens if a DisplayItemData with key K has two frames A and B and
> in the next paint, A and B have two non-merged display items both with key
> K? Seems like we could have a problem because they'd both hit the same
> DisplayItemData and we'd try to share it when we shouldn't?

Because we no longer have mNewDisplayItemData, I think we're fine here. On the second paint, frame A will pick up the old DisplayItemData in InvalidateForLayerChange, notice that B has been removed and invalidate the area for B. Then in AddLayerDisplayItem we will update the stored data, strip all references to B (and remove references from B -> data).

Once we get up to processing B, we won't be able to find the old display item data because of this.

I guess this still isn't ideal because we'd invalidate the area of B when we didn't need to, but I don't think this is a common case. These bugs seem to be happening when we scroll and a nsDisplayOpacity either appears or disappears and is added/remove to a merged group.


> 
> Needs a comment. I guess "Sets aOut to the list of frames that have been
> added to or removed from the display item"?

Sure.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #41)
> (In reply to Matt Woodrow (:mattwoodrow) from comment #39)
> > Note that we still invalidate the entire combined area if one of the merged
> > frames is deleted, but that seems like it would be a relatively rare case.
> 
> Where does that happen?

RemoveFrameFromLayerManager
Blocks: 796115
Blocks: 798761
Blocks: 771407
The bug 798761 crash is in the top 15 on Aurora 18, so nominating this for tracking 18.
(In reply to Jeff Muizelaar [:jrmuizel] from comment #27)
> It looks like this caused a tcheckerboard regression:
> 
> Regression :( Robocop Checkerboarding No Snapshot Benchmark increase 49.2%
> on Android 2.2 (Native) Mozilla-Inbound
> -----------------------------------------------------------------------------
> -------------------------------------
>    Previous: avg 2426.603 stddev 328.538 of 30 runs up to revision
> 41f527aaa8ba
>    New     : avg 3620.954 stddev 225.195 of 5 runs since revision
> 0bf4db8c014c
>    Change  : +1194.351 (49.2% / z=3.635)
>    Graph   : http://mzl.la/Rk5jnQ

This is fixed on inbound now. Looks like we're back to ~2100.
https://hg.mozilla.org/mozilla-central/rev/b6cd3108731b
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Blocks: 796117
Depends on: 803194
Comment on attachment 670249 [details] [diff] [review]
Implement the new DisplayItemData storage v4

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 539356
User impact if declined: Various crashes and performance regressions.
Testing completed (on m-c, etc.): Been on m-c for around a week.
Risk to taking this patch (and alternatives if risky): Fairly large code change, but it fixes a large set of crashes and performance issues. Please consider this request to be for all patches in this bug.
String or UUID changes made by this patch: None
Attachment #670249 - Flags: approval-mozilla-aurora?
Matt, On checking the DLBI Blocks buglist(796117 771407 795591 795826 796115 798761 801217 ) on this bug, it appears this patch only fixes one DLBI bug tracked for FF18 (Bug 796115) . So would it possible to get a low risk patch to fix that particular issue ? Also this patch seems to be causing regressions already (Bug 803194) , so any other alternatives here to land a low risk patch in aurora may be helpful .
Why do you think it didn't fix bug 798761 (for example)?
This also fixed bug 797254, and I think some other bugs. I'm confident we really do need this.
Blocks: 797254
Duplicate of this bug: 802408
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #50)
> Why do you think it didn't fix bug 798761 (for example)?

bug 798761 is not being tracked for 18, may be because its not critical. I saw it was #35 in aurora and that could be the reason . Although happy that it gets fixed by this patch :)
Comment on attachment 670249 [details] [diff] [review]
Implement the new DisplayItemData storage v4

Approving for aurora as it fixes various crashes & performance regressions due to dlbi landing and considering comment 51 .
Attachment #670249 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Blocks: 799579
Depends on: 1030370
You need to log in before you can comment on or make changes to this bug.