Closed Bug 1245737 Opened 4 years ago Closed 4 years ago

Reduce dtoa's memory usage

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: njn, Assigned: njn)

Details

Attachments

(3 files, 1 obsolete file)

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.
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.
This saves 50--60 KiB of memory per process.
Attachment #8715635 - Flags: review?(bhackett1024)
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
It's gone from 4,096 bytes per JS thread to 72 bytes per thread.
Attachment #8715639 - Flags: review?(bhackett1024)
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.)
Attachment #8715641 - Flags: review?(bhackett1024)
Attachment #8715639 - Attachment is obsolete: true
Attachment #8715639 - Flags: review?(bhackett1024)
Attachment #8715635 - Flags: review?(bhackett1024) → review+
Attachment #8715641 - Flags: review?(bhackett1024) → review+
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
(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.
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.
Attachment #8718119 - Flags: review?(sphink)
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.
Attachment #8718119 - Flags: review?(sphink) → review+
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.
https://hg.mozilla.org/mozilla-central/rev/f02665b3a963
https://hg.mozilla.org/mozilla-central/rev/0e8d761ff946
https://hg.mozilla.org/mozilla-central/rev/579b314c61d3
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.