Profile of the site http://www.keithclark.co.uk/labs/css-fps/nojs/ shows that memory allocations during BSP tree creation/deletion seem slow. During composite they can, at least according to the profile, take up to 20-40% of the total time. Performance profile: https://perfht.ml/2nJ3xGr I think it might be a really good idea to try something like arena here.

These patches fix various performance issues with BSPTree and gfx::Polygon implementations, and they seem to improve compositing performance by up to 20-25% in the demo above. Changelog: Polygon: * Avoid converting from 3D points to 4D points when clipping with Rect. * Avoid allocating new Polygons during clipping. BSPTree / Layers: * Added some missed uses of move semantics. * libc++ implementation of std::deque uses 4096 byte block allocations. This makes it very inefficient for shorter lists, which are common in BSPTree code. Replaced it with std::list. * Allocate BSPTree nodes using a memory pool. Misc: * Code cleanup and comments. * Pre-allocated nsTArrays if a there is a hint of a possible size.

Comment on attachment 8854473 [details] Bug 1351426 - Part 2: Only use 4D points in gfx::Polygon https://reviewboard.mozilla.org/r/126404/#review129052 Looks good. r=me with tiny indendation nit ::: gfx/2d/Polygon.h:92 (Diff revision 1) > > - explicit PolygonTyped(const nsTArray<Point3DType>& aPoints) > - : mNormal(DefaultNormal()), mPoints(ToPoints4D(aPoints)) > + explicit PolygonTyped(const std::initializer_list<Point4DType>& aPoints, > + const Point4DType& aNormal = DefaultNormal()) > + : mNormal(aNormal), mPoints(aPoints) > { > -#ifdef DEBUG > + #ifdef DEBUG nit: Did you mean to indent the #ifdef and #endif? In this case, I think they should be slammed to the left.

Comment on attachment 8854474 [details] Bug 1351426 - Part 3: Refactor BSPTree to use list instead of deque and use arena for memory allocations https://reviewboard.mozilla.org/r/126406/#review129072 I like it! r=me with a small nit on adding a comment. ::: gfx/layers/BSPTree.h:92 (Diff revision 1) > + explicit BSPTree(std::list<LayerPolygon>& aLayers) > { > MOZ_ASSERT(!aLayers.empty()); > - mRoot.reset(new BSPTreeNode(PopFront(aLayers))); > > + PL_InitArenaPool(&mPool, "BSPTreeArena", 4096, 0); nit: Perhaps it would be useful here to add a comment to describe how a size of 4096 was selected. What would happen if it were too big or too small?

Comment on attachment 8854475 [details] Bug 1351426 - Part 4: Refactor gfx::Polygon to avoid unnecessary work and memory allocations https://reviewboard.mozilla.org/r/126408/#review129078 LGTM

Comment on attachment 8854476 [details] Bug 1351426 - Part 5: Cleanup style and comments https://reviewboard.mozilla.org/r/126410/#review129082 Excellent, thanks for the cleanup and comments!

Comment on attachment 8854472 [details] Bug 1351426 - Part 1: Move the layer geometry instead of copying https://reviewboard.mozilla.org/r/126402/#review129114

Thank you for the reviews Kip and Matt. I have fixed the nits. Regarding the memory pool size, 4096 bytes was chosen as a reasonably sized power of two. With OSX/libc++ sizeof(BSPTreeNode) is 40 bytes, so one pool allocation is enough for roughly 100 BSPTreeNodes. For reference, the css-fps demo allocates around 900 BSPTreeNodes per frame, and https://threejs.org/examples/#css3d_panorama around 20. I also benchmarked the demo with a pool size of 2048 bytes, and the resulting compositing time compared to 4096 bytes was within profile sampling interval of 0.1ms.

I noticed that recently (4 days ago) the layout code moved from PLArena to ArenaAllocator (bug 1351904). Similarly, I changed the part 3 of this patch to use ArenaAllocator.

(In reply to Eric Rahm [:erahm] from comment #22) > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=63c94f6844f492153c99e677adbefe3e3632b58a Awesome! Thank you for this Eric.

Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b8598fc84f34 Part 1: Move the layer geometry instead of copying r=mattwoodrow https://hg.mozilla.org/integration/autoland/rev/8e36dfa4061e Part 2: Only use 4D points in gfx::Polygon r=kip https://hg.mozilla.org/integration/autoland/rev/a6c0e4789330 Part 3: Refactor BSPTree to use list instead of deque r=kip https://hg.mozilla.org/integration/autoland/rev/aeca2ef150b8 Part 4: Refactor gfx::Polygon to avoid unnecessary work and memory allocations r=kip https://hg.mozilla.org/integration/autoland/rev/efdb4e01ad4e Part 5: Cleanup style and comments r=kip

sorry had to back this out for memory leaks like https://treeherder.mozilla.org/logviewer.html#?job_id=90103019&repo=autoland that started with this push

Backout by cbook@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/99cab65c998b Backed out changeset efdb4e01ad4e https://hg.mozilla.org/integration/autoland/rev/59fe8d9f572f Backed out changeset aeca2ef150b8 https://hg.mozilla.org/integration/autoland/rev/3cc843a5c109 Backed out changeset a6c0e4789330 https://hg.mozilla.org/integration/autoland/rev/e27bd932966d Backed out changeset 8e36dfa4061e https://hg.mozilla.org/integration/autoland/rev/1b0e449993d4 Backed out changeset b8598fc84f34 for memory leaks

Another try. Fixed a bug where layer lists were not released during BSPTree destruction. https://treeherder.mozilla.org/#/jobs?repo=try&revision=517aebb6fd205e6573296c4429c0adfb454ce319

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/8657d0055dab Part 1: Move the layer geometry instead of copying r=mattwoodrow https://hg.mozilla.org/integration/autoland/rev/889ccddba31a Part 2: Only use 4D points in gfx::Polygon r=kip https://hg.mozilla.org/integration/autoland/rev/2c56897d9ed1 Part 3: Refactor BSPTree to use list instead of deque and use arena for memory allocations r=kip https://hg.mozilla.org/integration/autoland/rev/347a0fff68ba Part 4: Refactor gfx::Polygon to avoid unnecessary work and memory allocations r=kip https://hg.mozilla.org/integration/autoland/rev/00c0bff27644 Part 5: Cleanup style and comments r=kip

Backed out for bustage (implicit conversion at BSPTree.h:64): https://hg.mozilla.org/integration/autoland/rev/4706b59fb91102c52b1311c4878d616009932ad6 https://hg.mozilla.org/integration/autoland/rev/4f4ad271989803480975ba6a24e330fbee135f04 https://hg.mozilla.org/integration/autoland/rev/c31cc549dc7cf68f5a25a27171e3be69fa336cb9 https://hg.mozilla.org/integration/autoland/rev/809becdaddafd3b32201b9f5694b2f8b7eb56150 https://hg.mozilla.org/integration/autoland/rev/9538baf70db58f2e7ad7db1494482afca8d2febf Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=00c0bff27644aeba8eb96452874e21fc5c7cce65&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=90554079&repo=autoland [task 2017-04-11T17:52:07.322949Z] 17:52:07 INFO - In file included from /home/worker/workspace/build/src/obj-firefox/gfx/tests/gtest/Unified_cpp_gfx_tests_gtest0.cpp:83: [task 2017-04-11T17:52:07.323060Z] 17:52:07 INFO - In file included from /home/worker/workspace/build/src/gfx/tests/gtest/TestBSPTree.cpp:8: [task 2017-04-11T17:52:07.323150Z] 17:52:07 INFO - /home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/layers/BSPTree.h:64:3: error: bad implicit conversion constructor for 'BSPTreeNode' [task 2017-04-11T17:52:07.323200Z] 17:52:07 INFO - BSPTreeNode(nsTArray<LayerList*>& aListPointers) [task 2017-04-11T17:52:07.323229Z] 17:52:07 INFO - ^ [task 2017-04-11T17:52:07.323307Z] 17:52:07 INFO - /home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/layers/BSPTree.h:64:3: note: consider adding the explicit keyword to the constructor

Third time is the charm. Rebased the patches and added the missing explicit keyword to constructor. I hope the try run is reliable despite the issues with AWS today. Sorry for the inconvenience. https://treeherder.mozilla.org/#/jobs?repo=try&revision=a0783e0a38c69f741524091cc937946cbf1dd7d0

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/a8c0d0c65ce6 Part 1: Move the layer geometry instead of copying r=mattwoodrow https://hg.mozilla.org/integration/autoland/rev/dc963e6fa7f9 Part 2: Only use 4D points in gfx::Polygon r=kip https://hg.mozilla.org/integration/autoland/rev/437b6c732fec Part 3: Refactor BSPTree to use list instead of deque and use arena for memory allocations r=kip https://hg.mozilla.org/integration/autoland/rev/75657b48bb1c Part 4: Refactor gfx::Polygon to avoid unnecessary work and memory allocations r=kip https://hg.mozilla.org/integration/autoland/rev/5f4e3982c169 Part 5: Cleanup style and comments r=kip

https://hg.mozilla.org/mozilla-central/rev/a8c0d0c65ce6 https://hg.mozilla.org/mozilla-central/rev/dc963e6fa7f9 https://hg.mozilla.org/mozilla-central/rev/437b6c732fec https://hg.mozilla.org/mozilla-central/rev/75657b48bb1c https://hg.mozilla.org/mozilla-central/rev/5f4e3982c169