720595 - Report how many JS GC bytes were allocated while generating about:memory

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

about:memory is generated with some JS code.  The generation allocates some memory, which perturbs what is being measured.

It'd be really nice to know how many bytes of memory are allocated when generating about:memory, i.e. how big the perturbation is.  We could use this facility to reduce the perturbation.  This wouldn't count how many bytes have been freed -- the point is that if a GC happens in the middle of the generation, that shouldn't affect the result.

Some kind of counter that can be read before and after would be good.  If it can cover both GC heap memory and malloc memory, that'd be even great.

billm volunteered to do the JS side of this, I'll do the XPCOM plumbing.

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: patch to track collected bytes

I think this does what you want. Currently, we track the number of allocated bytes in rt->gcBytes. This patch adds a new fields, rt->gcCollectedBytes, that tracks the total number of bytes collected since startup. If you add rt->gcBytes + rt->gcCollectedBytes, you will get the total bytes ever allocated.

A few caveats:

First, gcCollectedBytes can overflow. But since you're probably going to be subtracting before and after values of (gcBytes + gcCollectedBytes), the overflow shouldn't matter as long as it doesn't happen during about:memory. And, in fact, you could detect the overflow if you were really careful, by checking if the before value is greater than the after value (that is, assuming you won't be allocated 2^64 bytes while collecting about:memory stats).

Also, these numbers are collected in arena-sized increments. You could do better by iterating over the entire heap before and after about:memory runs, but I'm guessing you already knew that and decided against it.

Comment 2

[Nicholas Nethercote [inactive]]

Comment on attachment 594261 [details] [diff] [review]
patch to track collected bytes

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

Looks fine.  I'll give f+ since I need to add the XPCOM plumbing before this lands.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: Patch 1: add JS::CumulativeGCBytes()

This is an updated version of Bill's original patch.  It adds the JSAPI
function JS::CumulativeGCBytes().

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: Patch 2: Add nsIMemoryReporter::cumulativeGCBytes

This patch adds nsIMemoryReporterManager::cumulativeGCBytes.  It was khuey's suggestion to use nsIJSRuntimeService for getting hold of the JSRuntime.

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: Patch 3: show cumulative GC bytes in about:memory

This shows at the bottom of about:memory the number of GC bytes allocated while generating about:memory.  This is only part of the memory allocated, but it has several important qualities:  (a) it's cheap to get, (b) it's very stable.  It'll be really useful to help reduce the perturbation caused by about:memory.

I also added a count of the number of values shown, just because it seems useful.

Comment 6

[Bobby Holley (:bholley)]

Comment on attachment 624300 [details] [diff] [review]
Patch 2: Add nsIMemoryReporter::cumulativeGCBytes

I don't think I'm the appropriate reviewer here. Bouncing to khuey, since this is apparently his idea.

Comment 7

[Justin Lebar (not reading bugmail)]

Comment on attachment 624301 [details] [diff] [review]
Patch 3: show cumulative GC bytes in about:memory

r=me, but maybe make this copy-pasteable?  The amount of memory allocated would be interesting to get from reporters, don't you think?

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 624299 [details] [diff] [review]
Patch 1: add JS::CumulativeGCBytes()

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

::: js/public/MemoryMetrics.h
@@ +183,5 @@
>  
>  extern JS_PUBLIC_API(size_t)
>  UserCompartmentCount(const JSRuntime *rt);
>  
> +// This returns the number of bytes ever allocated on the GC heap.  It's

One space after the period.

::: js/src/jscompartment.h
@@ +195,5 @@
>          gcPreserveCode = preserving;
>      }
>  
>      size_t                       gcBytes;
> +    size_t                       gcCollectedBytes;

Could you take out this field and the update to it in jsgc.cpp? It looks like you're not using it, and it costs us an op to maintain it. Also, I forgot to initialize it to 0.

Comment 9

[Justin Lebar (not reading bugmail)]

> Could you take out this field and the update to it in jsgc.cpp?

In part 1:

> --- a/js/src/MemoryMetrics.cpp
> +++ b/js/src/MemoryMetrics.cpp
> +JS_PUBLIC_API(int64_t)
> +CumulativeGCBytes(const JSRuntime *rt)
> +{
> +    return rt->gcBytes + rt->gcCollectedBytes;
> +}

Comment 10

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Justin Lebar [:jlebar] from comment #9)
> > --- a/js/src/MemoryMetrics.cpp
> > +++ b/js/src/MemoryMetrics.cpp
> > +JS_PUBLIC_API(int64_t)
> > +CumulativeGCBytes(const JSRuntime *rt)
> > +{
> > +    return rt->gcBytes + rt->gcCollectedBytes;
> > +}

That's a field of the runtime. I want the field on the compartment (with the same name) to be removed.

Comment 11

[Justin Lebar (not reading bugmail)]

> That's a field of the runtime. I want the field on the compartment (with the same name) to be 
> removed.

Ah, I totally missed "jscompartment.h".  Thanks!

Comment 12

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 624300 [details] [diff] [review]
Patch 2: Add nsIMemoryReporter::cumulativeGCBytes

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

::: xpcom/base/nsIMemoryReporter.idl
@@ +345,1 @@
>  };

You need to change the IID of the interface.

Comment 13

[Nicholas Nethercote [inactive]]

I won't land these patches.  I ended up not using them that much :(