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.

Etherpad with notes: https://etherpad.mozilla.org/mH1IzyXMSb

I think it's important we reinstate the scrolllayer underlying frame swap until we have code that deals with merged frames properly. Patch incoming.

Pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/036b983640be

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.

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.

Fixed a bug in FrameListMatches.

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.

(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 :)

(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"

> > @@ +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.

Haven't tested this all that much yet, but it improves perf a lot and makes the code a lot cleaner too.

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.)

(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?

https://hg.mozilla.org/integration/mozilla-inbound/rev/0586d2a97c52 https://hg.mozilla.org/integration/mozilla-inbound/rev/b1c3b7ae3571

https://hg.mozilla.org/mozilla-central/rev/0586d2a97c52 https://hg.mozilla.org/mozilla-central/rev/b1c3b7ae3571

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).

> 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.)

Torbjörn, this works: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=9f28c28e988f&tochange=0586d2a97c52

Hopefully fixed all the issues, including the problem with runfield. https://tbpl.mozilla.org/?tree=Try&rev=ba5b54e55f51

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.

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.

(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

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

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.

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

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.

(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 .

https://hg.mozilla.org/releases/mozilla-aurora/rev/699983c37342 https://hg.mozilla.org/releases/mozilla-aurora/rev/7f162c1da646 https://hg.mozilla.org/releases/mozilla-aurora/rev/54f0db3c0197 https://hg.mozilla.org/releases/mozilla-aurora/rev/97533c2c2010