Closed Bug 1457030 Opened 2 years ago Closed 2 years ago

APZCTreeManager::RecycleOrCreateNode is silly

Categories

(Core :: Graphics: Layers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: bas.schouten, Assigned: bas.schouten)

References

Details

Attachments

(1 file)

It stores recyclable nodes in an array.. and then grabs them from the front first.. causing the whole contents of the array to be moved every time one is removed.. To make things worse it then uses RemoveElement instead of RemoveElementAt which does a lookup, even though it already knows the index.. this silliness should be fixed. The comment says in the 'common case' this loop picks from the front each time, this means the lookup will be fast, but the removal will be expensive. This is easily fixed by iterating back to front which means the minimum amount of nodes will be copied.

Patch upcoming. This causes a markable improvement on the DisplayList mutate active test case.
Comment on attachment 8971091 [details]
Bug 1457030: Remove elements from the mNodesToDestroy array back to front, preventing expensive memmoves on large amounts of reuse.

https://reviewboard.mozilla.org/r/239872/#review245834

This is fine, and will help in the majority of pages with many layers.

Pages that have a lot of primary hit-test nodes (lots of APZC instances) won't be affected by this patch, and we might have a similar problem at https://searchfox.org/mozilla-central/rev/78dbe34925f04975f16cb9a5d4938be714d41897/gfx/layers/apz/src/APZCTreeManager.cpp#968. :dvander had previously attacked this problem as well in bug 1352919 with a different approach which I thought was a little unwieldy. There's a suggestion in bug 1352919 comment 6 for a more comprehensive change that would take of the lots-of-primary-nodes scenario as well.

But I'm happy to just land this and defer doing the rest until we know that it's actually needed. Totally possible that won't happen.
Attachment #8971091 - Flags: review?(bugmail) → review+
Summary: APZCTreeManager::RecycleOrCreateDrawing is silly → APZCTreeManager::RecycleOrCreateNode is silly
Pushed by bschouten@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5644d7852eb8
Remove elements from the mNodesToDestroy array back to front, preventing expensive memmoves on large amounts of reuse. r=kats
https://hg.mozilla.org/mozilla-central/rev/5644d7852eb8
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.