854763 - Add a memory reporter for asm.js array buffers.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

asm.js array buffers aren't being reported on x64.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Add a memory reporter for asm.js array buffers.

This patch cleaves the "objects-extra/elements" measurement in twain:
"elements/non-asm.js" and "elements/asm.js".  I moved
JSObject::sizeOfExcludingThis into jsobj.cpp to avoid mutual dependence between
between jsobjinlines.h and jstypedinlines.h.

I've tested on x64 on Citadel, because that's the only asm.js example I know
of!  The output looks reasonable -- we have a 512 MiB buffer.

1,729.85 MB (100.0%) -- explicit
├────899.29 MB (51.99%) -- window-objects
│    ├──889.18 MB (51.40%) -- top(file:///home/njn/Downloads/asm.js/Citadel.html, id=8)
│    │  ├──886.54 MB (51.25%) -- active/window(file:///home/njn/Downloads/asm.js/Citadel.html)
│    │  │  ├──886.26 MB (51.23%) -- js-compartment(file:///home/njn/Downloads/asm.js/Citadel.html)
│    │  │  │  ├──884.47 MB (51.13%) -- objects-extra
│    │  │  │  │  ├──884.31 MB (51.12%) -- elements
│    │  │  │  │  │  ├──512.00 MB (29.60%) ── asm.js
│    │  │  │  │  │  └──372.31 MB (21.52%) ── non-asm.js

Comment 2

[Andrew McCreight [:mccr8]]

Part of me is bothered by the fact that we'll have objects-extra/elements/non-asm.js on the 99.9% of pages that don't have asm.js, but I don't have any better suggestion aside from maybe splitting it into objects-extra/elements and objects-extra/elements-asm.js or something so I'll just stay over here in the peanut gallery.

Comment 3

[Nicholas Nethercote [inactive]]

> maybe splitting it
> into objects-extra/elements and objects-extra/elements-asm.js or something
> so I'll just stay over here in the peanut gallery.

I had something like that originally, but then added the suffix because "elements-asm.js" sounds like a subset of "elements".

Comment 4

[Luke Wagner [:luke]]

Comment on attachment 729387 [details] [diff] [review]
Add a memory reporter for asm.js array buffers.

Thanks!

Comment 5

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e47387009162

Comment 6

[Ryan VanderMeulen [:RyanVM]]

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

Comment 7

[Nicholas Nethercote [inactive]]

Bugger, I just realized there's a big problem with this.

Each multi-reporter has a GetExplicitNonHeap() method, which returns the sum of the sizes of all the KIND_NONHEAP reports it would report for the "explicit" tree.  This is used for fast computation of the "explicit" measurement, which is used in telemetry pings and is also available via nsIMemoryReporterManager::explicit.

For the JS multi-reporter, GetExplicitNonHeap() just gets the size of the entire heap (quick and easy) and the size of a handful of other things (the stack, and generated code).

But, thanks to asm.js, arbitrary JS objects can now hold pointers to non-heap memory, and GetExplicitNonHeap() isn't measuring these.  For example, when running Citadel, the measurement at the top of the explicit tree (which is computed by summing all the individual reports) is this:

  2,263.30 MB (100.0%) -- explicit

but the one in the "Other Measurements" section, which uses GetExplicitNonHeap() and is used by telemetry is this:

  1,751.22 MB ── explicit

It's missing exactly the 512 MiB asm.js array buffer.

To make GetExplicitNonHeap() accurate we'd need to iterate over every object in the JS heap, which is slow and entirely defeats the purpose of GetExplicitNonHeap().

Options I can see:

- Live with the discrepancy -- it only occurs for asm.js.  Yuk.

- Change things so that asm.js puts the 4 GiB allocation on the heap.  I don't know if this can be done in a sensible way that avoids committing most of it.

- Give up on measuring "explicit" for telemetry.  We'd also need to remove nsIMemoryReporterManager::explicit, which would break the endurance tests.  (AWSY would be ok, though, because it runs all the reporters.)

From a code maintenance POV, that third option would be a good thing -- you wouldn't have to worry about forgetting to update GetExplicitNonHeap.  (Note that there is a warning that gets printed if the two measurements don't match, but I didn't see it because it's only in debug builds and Citadel runs too slowly in debug builds to be usable...)

Comment 8

[Justin Lebar (not reading bugmail)]

> - Change things so that asm.js puts the 4 GiB allocation on the heap.  I don't know if 
> this can be done in a sensible way that avoids committing most of it.

On Linux and probably mac, sure.  I'm not so sure about Windows.

Ideally only the committed part of the array buffer would be in explicit...

Comment 9

[Nicholas Nethercote [inactive]]

> Ideally only the committed part of the array buffer would be in explicit...

It is.  The array has a certain size (e.g. 512 MiB) and everything past that up to the 4 GiB limit isn't counted.  (The 4 GiB is somehow used to catch out-of-bounds accesses, AIUI.)  So that's not a problem.

Comment 10

[Justin Lebar (not reading bugmail)]

>> Ideally only the committed part of the array buffer would be in explicit...
> 
> It is. 

Okay, that's good.  But if we allocated it on the heap all 4gb would be in explicit, right?  If so, that speaks against that solution...

Comment 11

[Nicholas Nethercote [inactive]]

I worked out a way to fix this problem with a counter.  Patch coming shortly.

Comment 12

[Nicholas Nethercote [inactive]]

Attachment: (follow-up): Include asm.js arrays the standalone "explicit" reporter.

Please don't ask me how long it took to get this patch working reliably.  I
could tell you but then I'd have to kill you.

This patch also fixes a leak in a failure case -- if the
mprotect/VirtualAlloc(MEM_COMMIT) call fails, we need to release the array
buffer.

Comment 13

[Luke Wagner [:luke]]

Comment on attachment 730402 [details] [diff] [review]
(follow-up): Include asm.js arrays the standalone "explicit" reporter.

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

::: js/src/jstypedarray.cpp
@@ +366,5 @@
>      // Enable access to the valid region.
>      JS_ASSERT(buffer->byteLength() % AsmJSAllocationGranularity == 0);
>  # ifdef XP_WIN
> +    if (!VirtualAlloc(p, PageSize + buffer->byteLength(), MEM_COMMIT, PAGE_READWRITE)) {
> +        VirtualAlloc(p, AsmJSMappedSize, MEM_RESERVE, PAGE_NOACCESS);

Holy crap, I think I should've used VirtualFree in ArrayBufferObject::releaseAsmJSArrayBuffer, to actually release the virtual address space allocation.  If you agree, can you change this failure case to use VirtualFree as well as in releaseAsmJSArrayBuffer?

Comment 14

[Nicholas Nethercote [inactive]]

> Holy crap, I think I should've used VirtualFree in
> ArrayBufferObject::releaseAsmJSArrayBuffer, to actually release the virtual
> address space allocation.

Indeed.  I'll fix both instances.  I just started a try run, I'll land once it's done.

Comment 15

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/257e193d6bdf

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/257e193d6bdf

Comment 17

[Nicholas Nethercote [inactive]]

Attachment: (part 2, Aurora) - Use VirtualFree instead of VirtualAlloc where appropriate.

This part seems worth backporting to Aurora.

Comment 18

[Nicholas Nethercote [inactive]]

Comment on attachment 732626 [details] [diff] [review]
(part 2, Aurora) - Use VirtualFree instead of VirtualAlloc where appropriate.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): asm.js.

User impact if declined: a 4 GiB memory leak for each asm.js array on Win64. Win64 is a non-tier-1 platform but it is the platform that was (ahem) used for the GDC demo.

Testing completed (on m-c, etc.): landed on m-c.

Risk to taking this patch (and alternatives if risky): negligible.

String or IDL/UUID changes made by this patch:

Comment 19

[Luke Wagner [:luke]]

Comment on attachment 732626 [details] [diff] [review]
(part 2, Aurora) - Use VirtualFree instead of VirtualAlloc where appropriate.

Thanks!

Comment 20

[Alex Keybl [:akeybl]]

Comment on attachment 732626 [details] [diff] [review]
(part 2, Aurora) - Use VirtualFree instead of VirtualAlloc where appropriate.

If the risk is negligible and limited to ASM.js, we can take this on Aurora.

Comment 21

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d47000234ce7