nsPresArena::Allocate calls free() a lot

RESOLVED FIXED in Firefox 56

Status

()

P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mats, Assigned: mats)

Tracking

(Blocks: 1 bug, {perf, regression})

Trunk
mozilla56
perf, regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 wontfix, firefox54 wontfix, firefox55 wontfix, firefox56 fixed)

Details

(Whiteboard: [qf])

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
I'm looking at a perf log where I see lot's of calls to free() on this path:
free()
  nsTArray::ShrinkCapacity()
    nsPresArena::Allocate()

I'm pretty sure it comes from removing an entry from the FreeList::mEntries:
http://searchfox.org/mozilla-central/rev/ae94cfb36d804727dfb38f2750bfcaab4621df6e/layout/base/nsPresArena.cpp#120

nsTArray::RemoveElementAt calls ShiftData which calls ShrinkCapacity:
http://searchfox.org/mozilla-central/rev/ae94cfb36d804727dfb38f2750bfcaab4621df6e/xpcom/ds/nsTArray-inl.h#261
*currently* only if it becomes empty, but I don't think we should count on that.
Anyway, I don't see a reason to free the array buffer even if it becomes empty
because it will surely be non-empty very soon again.

WIP: https://hg.mozilla.org/try/rev/ed2cc52291cc7dc817d17f0044247900c8e56e66

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f0c5730da03f73f0d3bf34e4ec8aa664c07d75d5

(bug 497495 is where we started using nsTArray for this, AFAICT)

Comment 1

2 years ago
Shows up in Speedometer.
Blocks: 1339557
status-firefox56: --- → affected
(Assignee)

Comment 2

2 years ago
Legend:
calls: the number of calls to nsPresArena::Allocate where we end up
       taking an object off a free list (i.e. the number of times
       we hit the changed code)
large: same as calls, but where "list->mEntries.Capacity() > 500" is true
       (which corresponds to a malloc size of 4096 bytes or more)
arenas: the number of alive arenas at the end of the test time
free: the number of times free() called from here
overhead: the number of allocated bytes in free list arrays that
          aren't used
histogram: occurences of transitions to empty per malloc size

========================================

https://slashdot.org
before: calls=22015 arenas=24 free=2701 overhead=9600 bytes
after:  large=0 arenas=19 free=0 overhead=14784 bytes
histogram[16]: 503
histogram[32]: 427
histogram[64]: 341
histogram[128]: 122
histogram[256]: 397
histogram[512]: 153
histogram[1024]: 24
histogram[2048]: 81

http://cnn.com/
before: calls=20891 arenas=8 free=1598 overhead=9896 bytes
after:  large=4199 arenas=8 free=3 overhead=13424 bytes
histogram[16]: 319
histogram[32]: 574
histogram[64]: 178
histogram[128]: 86
histogram[256]: 205
histogram[512]: 99
histogram[1024]: 155
histogram[2048]: 9
histogram[4096]: 3

https://www.youtube.com/
before: calls=21958 arenas=6 free=842 overhead=16504 bytes
after:  large=12434 arenas=5 free=7 overhead=28248 bytes
histogram[16]: 153
histogram[32]: 108
histogram[64]: 29
histogram[128]: 164
histogram[256]: 48
histogram[512]: 263
histogram[1024]: 1
histogram[2048]: 17
histogram[4096]: 2
histogram[8192]: 2
histogram[16384]: 1
histogram[32768]: 2

https://en.wikipedia.org/
before: calls=5724 arenas=11 free=1084 overhead=8064 bytes
after:  large=77 arenas=11 free=0 overhead=9280 bytes
histogram[16]: 257
histogram[32]: 235
histogram[64]: 181
histogram[128]: 141
histogram[256]: 273
histogram[512]: 4


The test is: load the URL using a fresh profile and wait 1 minute
15 seconds, then measure the overhead over all arenas.
(Using "firefox -layoutdebug" (at 800x1000) to minimize UI overhead)

========================================

Conclusion: the overhead increases but the numbers are fairly small.
In exchange, we almost eradicate the number of malloc/free calls
associated with these arrays.

Note 1: we should save at least the same amount of malloc calls as
the "free" numbers above

Note 2: the difference in number of arenas before/after is likely due
to ads and other dynamic effects, so all numbers are ball park figures
(Assignee)

Comment 3

2 years ago
FTR, resizing the window tilts the histogram even more towards
lower sizes.
(Assignee)

Comment 4

2 years ago
(I'm removing the "LIFO behavior for best cache utilization" because
I think it's obsolete - it's about the linked list implementation
we had before bug 497495.)
Attachment #8880974 - Flags: review?(dbaron)

Comment 5

2 years ago
Pushed by mpalmgren@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e4aad4b94ca6
Retain the array buffer for PresArena free lists if it's 2048 bytes or less to avoid malloc/free churn.  r=dbaron

Comment 6

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e4aad4b94ca6
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox56: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
status-firefox54: --- → wontfix
status-firefox55: --- → wontfix
status-firefox-esr52: --- → wontfix
You need to log in before you can comment on or make changes to this bug.