Closed
Bug 1351426
Opened 8 years ago
Closed 8 years ago
BSP tree memory allocations take a lot of time
Categories
(Core :: Graphics: Layers, defect)
Core
Graphics: Layers
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: mikokm, Assigned: mikokm)
References
Details
Attachments
(5 files)
59 bytes,
text/x-review-board-request
|
mattwoodrow
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
kip
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
kip
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
kip
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
kip
:
review+
|
Details |
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.
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → mikokm
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 6•8 years ago
|
||
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 7•8 years ago
|
||
mozreview-review |
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.
Attachment #8854473 -
Flags: review?(kgilbert) → review+
Comment 8•8 years ago
|
||
mozreview-review |
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?
Attachment #8854474 -
Flags: review?(kgilbert) → review+
Comment 9•8 years ago
|
||
mozreview-review |
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
Attachment #8854475 -
Flags: review?(kgilbert) → review+
Comment 10•8 years ago
|
||
mozreview-review |
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!
Attachment #8854476 -
Flags: review?(kgilbert) → review+
Comment 11•8 years ago
|
||
mozreview-review |
Comment on attachment 8854472 [details]
Bug 1351426 - Part 1: Move the layer geometry instead of copying
https://reviewboard.mozilla.org/r/126402/#review129114
Attachment #8854472 -
Flags: review?(matt.woodrow) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 17•8 years ago
|
||
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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 21•8 years ago
|
||
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.
Comment 22•8 years ago
|
||
Assignee | ||
Comment 23•8 years ago
|
||
(In reply to Eric Rahm [:erahm] from comment #22)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=63c94f6844f492153c99e677adbefe3e3632b58a
Awesome! Thank you for this Eric.
Assignee | ||
Comment 24•8 years ago
|
||
Keywords: checkin-needed
Comment 25•8 years ago
|
||
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
Keywords: checkin-needed
Comment 26•8 years ago
|
||
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
Flags: needinfo?(mikokm)
Comment 27•8 years ago
|
||
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
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 33•8 years ago
|
||
Another try. Fixed a bug where layer lists were not released during BSPTree destruction.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=517aebb6fd205e6573296c4429c0adfb454ce319
Flags: needinfo?(mikokm)
Keywords: checkin-needed
Comment 34•8 years ago
|
||
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
Keywords: checkin-needed
![]() |
||
Comment 35•8 years ago
|
||
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
Flags: needinfo?(mikokm)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 41•8 years ago
|
||
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
Flags: needinfo?(mikokm)
Keywords: checkin-needed
Comment 42•8 years ago
|
||
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
Keywords: checkin-needed
Comment 43•8 years ago
|
||
bugherder |
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
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in
before you can comment on or make changes to this bug.
Description
•