748440 - Add telemetry for heap fragmentation

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[Justin Lebar (not reading bugmail)]

We don't currently have telemetry telling us how much memory we waste out of the malloc heap and the JS heap.

As a result, we don't have any idea how bad our fragmentation problem is.

Comment 1

[Justin Lebar (not reading bugmail)]

njn, we don't have an assertion that percentage reporters are less than 100%, right?  I don't see one anywhere, but just to check...

Comment 2

[Justin Lebar (not reading bugmail)]

Ah, I realized that I totally messed up telemetry for js-heap-allocated, because telemetry doesn't collect multi-reporters, anad I moved heap-allocated into a multi-reporter.  :(

Comment 3

[Justin Lebar (not reading bugmail)]

Attachment: Part 1: Add js-summary multi-reporter and tweak some memory reporters.

The main change here is adding a new memory reporter which contains a bunch of KIND_OTHER reporters from the "js" memory reporter.  "js" still reports the old values as before, but now "js-summary" reports those same values, under the path "js-summary/" and with KIND_SUMMARY.

I renamed "*fragmentation" to "*committed-unused-ratio" and changed it to report |waste / allocated|, which I think is a better metric than |waste / committed| -- what we should be getting at with this value is the relative overhead of the allocator.  I also added "*committed-unused" which reports the raw amount of committed, unused memory.

Comment 4

[Justin Lebar (not reading bugmail)]

Attachment: Part 2: Support multi-reporters in telemetry, and report {heap,js}-committed-unused{,-ratio} in telemetry.

In addition to reporting these new values, this patch fixes js-gc-heap-committed reporting, which I broke in bug 741378.  (js-gc-heap-committed was in a memory reporter, so it was inaccessible to telemetry until this patch.)

Comment 5

[Justin Lebar (not reading bugmail)]

Comment on attachment 618123 [details] [diff] [review]
Part 2: Support multi-reporters in telemetry, and report {heap,js}-committed-unused{,-ratio} in telemetry.

Taras, are you OK with Nathan reviewing this patch, or would you like to look at it yourself?

Comment 6

[(dormant account)]

Comment on attachment 618123 [details] [diff] [review]
Part 2: Support multi-reporters in telemetry, and report {heap,js}-committed-unused{,-ratio} in telemetry.

Nathan is ok to review stuff like this, but since I'm here, have an r+. I would prefer if this had tests, I didn't put in any myself because about:memory stuff was not really useful from xpcshell tests. Would be nice to not regress these.

Optional nit: looks like the if, that you reused from my code, can become a switch() statement.

Comment 7

[Justin Lebar (not reading bugmail)]

> Would be nice to not regress these.
...again.  Yes.  :)

So the way we test about:memory is we unregister all the real memory reporters, then register some fake ones which always return the same value.  At the end of the test, we fix things back up.

But it looks like the ping that test_TelemetryPing.js gets is a real ping -- that is, it includes data collected before the test started running.  If I unregister all the memory reporters at the beginning of the test, I still get a histogram for MEMORY_STORAGE_SQLITE, which is backed by a memory reporter.

I don't see a way to clear out this data from telemetry, either.

So it looks like I can test that the data is there, but not that it's correct.  Is that a test you think I should do?

Comment 8

[Nathan Froyd [:froydnj]]

(In reply to Justin Lebar [:jlebar] from comment #7)
> I don't see a way to clear out this data from telemetry, either.

We were just talking today about how it would be useful to have Telemetry.clearHistogram in a different context.  If it would be useful for test purposes, then so much the better.

Comment 9

[Nicholas Nethercote [inactive]]

Is part 1 missing some changes?  You've used KIND_SUMMARY in XPCJSRuntime.cpp but haven't defined it anywhere, AFAICT.

Comment 10

[(dormant account)]

(In reply to Justin Lebar [:jlebar] from comment #7)
> > Would be nice to not regress these.
> ...again.  Yes.  :)
> 
> So the way we test about:memory is we unregister all the real memory
> reporters, then register some fake ones which always return the same value. 
> At the end of the test, we fix things back up.
> 
> But it looks like the ping that test_TelemetryPing.js gets is a real ping --
> that is, it includes data collected before the test started running.  If I
> unregister all the memory reporters at the beginning of the test, I still
> get a histogram for MEMORY_STORAGE_SQLITE, which is backed by a memory
> reporter.
> 
> I don't see a way to clear out this data from telemetry, either.
> 
> So it looks like I can test that the data is there, but not that it's
> correct.  Is that a test you think I should do?

Yes. As Nathan said, bug 748914 should get us a way to clear telemetry histograms

Comment 11

[Justin Lebar (not reading bugmail)]

Since this fixes a regression, I don't want to block waiting on another bug.  I'll write a test once clearing is implemented.

Comment 12

[Justin Lebar (not reading bugmail)]

Oh, I understand, you meant "yes, you should test that the data is there".  I can do that...

Comment 13

[Justin Lebar (not reading bugmail)]

(In reply to Nicholas Nethercote [:njn] from comment #9)
> Is part 1 missing some changes?  You've used KIND_SUMMARY in
> XPCJSRuntime.cpp but haven't defined it anywhere, AFAICT.

We defined it for ghost windows some patches ago.

Comment 14

[Justin Lebar (not reading bugmail)]

Attachment: Part 3: Add a test

Comment 15

[(dormant account)]

Comment on attachment 618483 [details] [diff] [review]
Part 3: Add a test

this will do nicely

Comment 16

[Nicholas Nethercote [inactive]]

Comment on attachment 618123 [details] [diff] [review]
Part 2: Support multi-reporters in telemetry, and report {heap,js}-committed-unused{,-ratio} in telemetry.

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

IIUC, the "js-summary" multi-reporter touches every live thing on the JS heap, and it's run on every telemetry ping.  This isn't acceptable -- we used to do exactly that, but I fixed that in bug 708159.

I think some of the numbers you want to report to telemetry aren't obtainable without touching every live thing on the JS heap... I don't know how to square that circle :(

Comment 17

[Nicholas Nethercote [inactive]]

Comment on attachment 618122 [details] [diff] [review]
Part 1: Add js-summary multi-reporter and tweak some memory reporters.

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

Removing the review request because of my r- for the follow-up patch.

Comment 18

[Justin Lebar (not reading bugmail)]

(In reply to Nicholas Nethercote [:njn] from comment #16)
> IIUC, the "js-summary" multi-reporter touches every live thing on the JS
> heap, and it's run on every telemetry ping.  This isn't acceptable -- we
> used to do exactly that, but I fixed that in bug 708159.

Ah, you're totally right.

> I think some of the numbers you want to report to telemetry aren't obtainable without touching every 
> live thing on the JS heap... I don't know how to square that circle :(

I really want these heap fragmentation numbers, and this patch fixes a regression (js-gc-heap), so I'll split out the JS stuff and we can worry about it separately.

Comment 19

[Justin Lebar (not reading bugmail)]

Anyone have telemetry they want from a memory multi-reporter?  I'd hate to leave unused code in the tree...

Comment 20

[Nicholas Nethercote [inactive]]

(In reply to Justin Lebar [:jlebar] from comment #19)
> Anyone have telemetry they want from a memory multi-reporter?  I'd hate to
> leave unused code in the tree...

Not that I know of.  Running multi-reporters for telemetry seems like a bad idea in general -- too easy for them to take a long time...

Comment 21

[Justin Lebar (not reading bugmail)]

Attachment: Part 1, v2: Add heap-committed-unused and heap-committed-unused-ratio.

Comment 22

[Justin Lebar (not reading bugmail)]

Attachment: Part 2, v2: Report heap-committed-unused{,-ratio} in telemetry.

This is the same as parts 2-3 v1, except it doesn't include js-gc-committed-unused{,-ratio} and doesn't support multi-reporters.  The test is a bit weaker too -- we now test a reporter with UNITS_COUNT, and UNITS_BYTE, but not with UNITS_PERCENTAGE, because the only reporter with those units that's in telemetry is heap-committed-unused-ratio, and that reporter is not available without jemalloc (and therefore the test would fail in trace-malloc builds).

I don't think Taras needs to look at this again, since the changes are mostly code removal.

Comment 23

[Justin Lebar (not reading bugmail)]

Wow, these patches do not even compile.   Let me try again.

Comment 24

[Mozilla RelEng Bot]

Try run for 55757fb6909b is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=55757fb6909b
Results (out of 15 total builds):
    failure: 15
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-55757fb6909b

Comment 25

[Justin Lebar (not reading bugmail)]

Attachment: Part 1, v3

This one compiles.  :)

Comment 26

[Nicholas Nethercote [inactive]]

Comment on attachment 619107 [details] [diff] [review]
Part 1, v3

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

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +1792,5 @@
>  
> +        REPORT(NS_LITERAL_CSTRING("js-main-runtime-gc-heap-committed-unused-ratio"),
> +               nsIMemoryReporter::KIND_OTHER,
> +               nsIMemoryReporter::UNITS_PERCENTAGE,
> +               (PRInt64) 10000 * rtStats.gcHeapUnused /

Can you crib the removed comment from MemoryMetrics.cpp to explain why the 10000 is used here?  Thanks.

Comment 27

[Mozilla RelEng Bot]

Try run for 96e316210931 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=96e316210931
Results (out of 230 total builds):
    exception: 9
    success: 124
    warnings: 27
    failure: 70
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-96e316210931

Comment 28

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/f5a301fe9ba5
https://hg.mozilla.org/mozilla-central/rev/d7db4957bb75