1324808 - Improve string allocation

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

See https://bugzilla.mozilla.org/show_bug.cgi?id=1301025#c9

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

And StringBuffer allocates realloc(aHdr, sizeof(nsStringBuffer) + aSize), so need to deal with the sizeof nsStringBuffer too.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Attachment: string_alloc_optimization.diff

A bit ugly code.

Try to ensure we get the next jemalloc bucket size (pow2 or large bucket) including the sizeof (nsStringBuffer) and also the extra sizeof(char_type) for null termination.
This should ensure we don't allocate too much when StringBuffer::Alloc/Realloc are called.

Currently, say we're allocating for nsString length 17, we end up rounding capacity to 32, then multiply with sizeof(char_type) to 64
and add 2 bytes for null and 8 for nsStringBuffer, and end up having allocation 74, which then
malloc rounds to 80 or 96 or 112 or 128 or such

With new setup we round (17 + 5) -> 32, decrease 5 -> 27, add 1 for null, 28, multiple with sizeof(char_type) -> 56 and add sizeof(nsStringBuffer) -> 64

The difference is more when doing large allocations.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

remote: View your change here:
remote:   https://hg.mozilla.org/try/rev/d7199ee695435ea767d5404a27f68134b443605b
remote: 
remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=d7199ee695435ea767d5404a27f68134b443605b
remote: 
remote: It looks like this try push has talos jobs. Compare performance against a baseline revision:
remote:   https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=d7199ee695435ea767d5404a27f68134b443605b

m-i base 9034d9e08ee3

Comment 4

[Nathan Froyd [:froydnj]]

Comment on attachment 8820385 [details] [diff] [review]
string_alloc_optimization.diff

Review of attachment 8820385 [details] [diff] [review]:
-----------------------------------------------------------------

Had to think through this a bit, but I believe this is correct.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9d75cfe7af3
Improve string allocation, r=nfroyd

Comment 6

[Claudio]

Awesome, thanks for the quick fix!

However, considering bug 1301025, won't this be a problem until the fix is released in version 53?
Wouldn't it make more sense to release bug 1301025 and bug 1324808 together so we won't have the problem in release? Or at least as soon as possible?

Thank you very much!!

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Well, even before bug 1301025 we've allocated somewhat random jemalloc buckets for strings, and often increased the allocated size more than needed.
Is there some particular memory usage pattern you're worried about? Have you seen higher memory usage after bug 1301025? (It should have still reduced memory usage in many cases, especially when dealing with very long strings)

Comment 8

[Claudio]

Well yeah.
I compared Firefox 51b9 and 50.1.0 using F12->Memory snapshot and it seems Firefox 51 is using much more memory in the "strings" block.

Well I think it might have to do with the fact that bug 1301025 made strings with curCapacity = 0 get allocated to their nearest power of 2, and not exactly their initial size, increasing memory usage..

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

I don't know what that "strings" block is measuring. Is it about JS strings or all strings. My guess is JS strings.
And anyhow, I tried without this patch, without patch for bug 1301025 and with both patches and couldn't really see difference in "strings"

Comment 10

[Claudio]

Well, I noticed it on some popular sites, probably depends on the content..

Anyway, I think the ESR will be released with version 52, right?
I really hoped we could make it by then - so we won't have this effect for a long period of time in the ESR

What do you think?

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

I was told in #devtools that 'strings' in Memory means JS strings.

Oh, right, 52 makes sense.
And if we can see some regression caused by bug 1301025 which this bug fixes, this should land to 51 too.

Comment 12

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

https://hg.mozilla.org/mozilla-central/rev/d9d75cfe7af3

Comment 13

[Claudio]

Sounds good, it could be great if we make it to the ESR,
I see now that we're still not fixed in the aurora channel (which is Firefox 52..)

Don't we need to uplift this to the aurora channel to make it to 52? 

Thanks! :)

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8820385 [details] [diff] [review]
string_alloc_optimization.diff

Hmm, I guess we could take this to Aurora.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1301025 may have increased memory usage in some cases (but reduce in others). This patch tries to always ensure better memory usage with Gecko C++ strings.
[User impact if declined]: Possibly higher memory usage
[Is this code covered by automated tests?]: We use nsString everywhere
[Has the fix been verified in Nightly?]: NA
[Needs manual test from QE? If yes, steps to reproduce]: NA 
[List of other uplifts needed for the feature/fix]: NA
[Is the change risky?]: Should be rather safe. Just allocate less extra memory
[Why is the change risky/not risky?]: See above
[String changes made/needed]: NA

Comment 15

[Julien Cristau [:jcristau]]

Comment on attachment 8820385 [details] [diff] [review]
string_alloc_optimization.diff

string allocation tweak, aurora52+

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9e033ffdf2fb