1362876 - Reduce the cost of dynamic allocations for FloatManager

Status: RESOLVED FIXED

(Core :: Layout, enhancement)

Description

[(no longer active)]

When profiling bug 1357621, I noticed a couple of cases where we can easily reduce the cost of dynamic memory allocation that we currently do for FloatManager.

Comment 1

[(no longer active)]

Attachment: Part 1: Allocate nsFloatManager on the stack instead of on the heap

Comment 2

[(no longer active)]

Attachment: Part 2: Use reserved storage for 10 FloatInfo objects on the stack in nsFloatManager::mFloats

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

My initial quick reaction is that 10 seems too high -- my guess is that almost all float managers have 0 floats, and then the distribution of those that do have floats peaks at 1.  I'd assume that increasing the size of stack-allocation objects reduces memory locality for other things on the stack, so I think there is a real tradeoff.  And FloatInfo isn't particularly small (2 words and 6 32-bit numbers.)

Comment 4

[Mats Palmgren (inactive)]

> Part 1: Allocate nsFloatManager on the stack instead of on the heap

It seems we already cache these, so allocating them on the stack
probably doesn't help much:
http://searchfox.org/mozilla-central/rev/8b70b0a5038ef9472fe7c53e04a70c978bb06aed/layout/generic/nsFloatManager.cpp#66-100

Caching the FloatInfo arrays in some way might help though.

Comment 5

[Mats Palmgren (inactive)]

... which is what Part 2 does in a way, so taking only that part seems reasonable to me.

Comment 6

[Mats Palmgren (inactive)]

FYI, part 2 changes the size of nsFloatManager from 56 bytes to 464 bytes on a 64-bit machine.
The nearest jemalloc sizes are 448 and 512, so we might as well make it 11 elements which
makes it 504 bytes.  8 fits exactly in a jemalloc 384 byte chunk, 9 needs 424 bytes which
fits in a 448 byte chunk but spills 24 bytes.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8865239 [details] [diff] [review]
Part 1: Allocate nsFloatManager on the stack instead of on the heap

Given that we already cache these, I think I'm inclined to think we shouldn't do this.  It's a pretty sizeable increase in the size of stack frames in layout, which has two problems:
 1. reduction in memory locality
 2. reduction in the depth of frame tree at which we start hitting stack overflow crashes in Reflow (which is really a think, due to the large amount of stack storage we use in Reflow)

As an alternative, I'd suggest we should probably substantially increase NS_FLOAT_MANAGER_CACHE_SIZE.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

s/think/thing/.

(I'd suggest maybe 32 or 64 for the cache size?)

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8865240 [details] [diff] [review]
Part 2: Use reserved storage for 10 FloatInfo objects on the stack in nsFloatManager::mFloats

On the assumption of *not* doing part 1, I think this probably becomes a good idea, as Mats suggests.

Comment 10

[(no longer active)]

OK taking only part 2 sounds good!  I will file a follow-up to increase NS_FLOAT_MANAGER_CACHE_SIZE.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/912f1804ec56
Use reserved storage for 10 FloatInfo objects on the stack in nsFloatManager::mFloats; r=dbaron

Comment 12

[Mats Palmgren (inactive)]

Is there a reason this must be precisely 10?
8 or 11 seems more optimal to me (see comment 6)

Comment 13

[(no longer active)]

Follow-up: bug 1363252.

Comment 14

[(no longer active)]

(In reply to Mats Palmgren (:mats) from comment #12)
> Is there a reason this must be precisely 10?
> 8 or 11 seems more optimal to me (see comment 6)

D'oh sorry, I have an autolander setup locally that keeps landing patches as soon as I put them on a local branch.  :-(  It already did it once again earlier today before I remembered to address the review comment (https://bugzilla.mozilla.org/show_bug.cgi?id=1362880#c5).  I'm still not quite used to my new computer setup yet.  So sorry about this, will land a follow-up to bump up to 11.  :-/

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1eb6f7736fa4
follow-up: Bump up the reserved size to 11

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/912f1804ec56
https://hg.mozilla.org/mozilla-central/rev/1eb6f7736fa4