Closed Bug 1457030 Opened 2 years ago Closed 2 years ago
Manager::Recycle Or Create Node is silly
Bug 1457030: Remove elements from the mNodesToDestroy array back to front, preventing expensive memmoves on large amounts of reuse.
59 bytes, text/x-review-board-request
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
Duplicate of this bug: 1352919
Pushed by email@example.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
You need to log in before you can comment on or make changes to this bug.