1245737 - Reduce dtoa's memory usage

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

We're wasting space in DtoaState.

- Our dtoa implementation has a custom allocator that uses a pre-allocated
  2,304 byte pool. (It may also do additional allocations if necessary beyond
  that).

- Our jemalloc rounds up allocation requests in the range 2,049--4,096 bytes to
  4,096 bytes.

- DtoaState is stored in PerThreadData, so we have one per JS thread. That's
  typically 12--15 per process, for 50--60 KiB per process.

- With e10s we currently have 2 processes, but that's likely to go up in the
  future.

There's a #define constant called Omit_Private_Memory that you can set so
that vanilla malloc/free is used instead of the 2,304 byte pool. I tried that
and it turns out to be much better -- the 2,304 bytes is total overkill, and
appears to be chosen for extreme cases involving numbers with hundreds of
digits (see the comment on Omit_Private_Memory).

I tried starting the browser with one content process and opening gmail.com,
cnn.com, nytimes.com and bbc.co.uk. With the pool I got 28 dtoa_malloc calls,
each requesting 2,384 bytes (which is the pool size plus a little more for
other stuff). After rounding up that gives 114,688 bytes.

Without the pool I got the following dtoa_malloc requests:

> 50 counts:
> (  1)       28 (56.0%, 56.0%): dtoa_malloc(72)
> (  2)       10 (20.0%, 76.0%): dtoa_malloc(36)
> (  3)        8 (16.0%, 92.0%): dtoa_malloc(44)
> (  4)        3 ( 6.0%, 98.0%): dtoa_malloc(60)
> (  5)        1 ( 2.0%,100.0%): dtoa_malloc(32)

72 bytes is the size of DtoaState with the pool disabled.

If I weight those counts by size, I get the following:

> 2940 counts:
> (  1)     2016 (68.6%, 68.6%): dtoa_malloc(72)
> (  2)      360 (12.2%, 80.8%): dtoa_malloc(36)
> (  3)      352 (12.0%, 92.8%): dtoa_malloc(44)
> (  4)      180 ( 6.1%, 98.9%): dtoa_malloc(60)
> (  5)       32 ( 1.1%,100.0%): dtoa_malloc(32)

That's 2,940 bytes compared to 114,688 bytes. Let's turn the pool off.

Comment 1

[Nicholas Nethercote [inactive]]

It looks like we once used Omit_Private_Memory but removed it in bug 384244. But we only had one process and one JS thread back then, so 2304 bytes wasn't much.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Enable |Omit_Private_Memory| for our dtoa implementation

This saves 50--60 KiB of memory per process.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Don't bother measuring DtoaState

It's gone from 4,096 bytes per JS thread to 72 bytes per thread.

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Don't bother measuring DtoaState

It's gone from 4,096 bytes per JS thread to 72 bytes per thread. (Also, the
old code only measured the DtoaState for the main thread.)

Comment 5

[Nicholas Nethercote [inactive]]

Ugh, dtoa appears to be leaky, and the use of the private pool masked that fact: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba92f14d6bed

Comment 6

[Nicholas Nethercote [inactive]]

(In reply to Nicholas Nethercote [:njn] from comment #5)
> Ugh, dtoa appears to be leaky, and the use of the private pool masked that
> fact

Or it might be caused by us not calling destroydtoa() on every DtoaState that we create with newdtoa(). Though if that's the case, I don't know why we wouldn't have gotten leak reports from ASAN before this change, given that the DtoaStates are themselves heap-allocated. So I'm not yet sure what the exact problem is.

Comment 7

[Nicholas Nethercote [inactive]]

Attachment: (part 0) - Fix a leak in DtoaState

DtoaState has a field |p5s| that stores a linked list of Bigints which never
gets freed. This patch adds freeing code for it to destroydtoa().

This has never shown up before because DtoaState currently uses a private chunk
for allocations instead of malloc/free and that private chunk does get freed.
But the next patch will turn off the use of that private chunk so we now need
to free |p5s| so that ASAN doesn't complain.

The new code follows the bizarre local style used by dtoa.c.

Comment 8

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8718119 [details] [diff] [review]
(part 0) - Fix a leak in DtoaState

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

Wow. Ok, I sort of kind of traced through the code, and I see where it's allocating these and how it isn't freeing them. Good enough. I attempted to figure out how 'freelist' is managed, exactly; clearly its contents are freed. But I think I'm cutting my losses now. If no problems are reported by tools, I'm totally fine with this change since it's clearly more gooder.

Comment 9

[Nicholas Nethercote [inactive]]

Thank you for the fast review.

BTW, the way I figured this out was write a small test program that #included dtoa.c and did a few conversions, and ran it under Valgrind which pointed out the leaks. This was much easier to deal with than a full Firefox or even SpiderMonkey build.
And then I confirmed on try server that ASAN was happy as well.

Comment 10

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f02665b3a963a6df9a598875a0edf29f776e4d0d
Bug 1245737 (part 0) - Fix a leak in DtoaState. r=sfink.

https://hg.mozilla.org/integration/mozilla-inbound/rev/0e8d761ff9468f924a372008333ddf803444ba6c
Bug 1245737 (part 1) - Enable |Omit_Private_Memory| for our dtoa implementation. r=bhackett.

https://hg.mozilla.org/integration/mozilla-inbound/rev/579b314c61d300698a75e241d00c1c15facd2b56
Bug 1245737 (part 2) - Don't bother measuring DtoaState. r=bhackett.

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f02665b3a963
https://hg.mozilla.org/mozilla-central/rev/0e8d761ff946
https://hg.mozilla.org/mozilla-central/rev/579b314c61d3