1386511 - Improve the formatting of GC markers

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect, P3)

Description

[Paul Bone [:pbone]]

The format of the GCMinor markers could be improved, for example each of the three alternatives here: http://searchfox.org/mozilla-central/source/js/src/gc/Nursery.cpp#480 should have a status field that can be used to switch between them in the perf.html tool.

Likewise status="no collection" could be improved with more specific statuses like "empty nursery".

Comment 1

[Paul Bone [:pbone]]

This is based on suggestions made by sfink in an e-mail.

Comment 2

[Paul Bone [:pbone]]

I'm revising the formats of all three GC marker types.

Comment 3

[Paul Bone [:pbone]]

Attachment: Bug 1386511 (part 1) - Revise the format of the GCMajor marker

I have also added a slice_number property to this marker. If each slice is also numbered then we can use that to match slices to their collections within perf.html.

Comment 4

[Jon Coppeard (:jonco)]

Comment on attachment 8898153 [details] [diff] [review]
Bug 1386511 (part 1) - Revise the format of the GCMajor marker

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

::: js/src/gc/Statistics.cpp
@@ +556,5 @@
> +    /*
> +     * The format of the JSON message is specified by the GCMajorMarkerPayload
> +     * type in perf.html
> +     * https://github.com/devtools-html/perf.html/blob/master/src/types/markers.js#L62
> +     *

What we currently supply doesn't seem to match up with that format at all.  Am I reading this wrong, or is this the problem you're addressing?

Comment 5

[Paul Bone [:pbone]]

Comment on attachment 8898153 [details] [diff] [review]
Bug 1386511 (part 1) - Revise the format of the GCMajor marker


> What we currently supply doesn't seem to match up with that format at all. 
> Am I reading this wrong, or is this the problem you're addressing?

Yes, that format was Just Enough(TM) to get the basic tooltips working.  This will be easier to review if I can also show the corresponding changes to perf.html.  I withdraw my review request until I have the corresponding work ready for review by the perf.html team.

Comment 6

[Paul Bone [:pbone]]

Attachment: Bug 1386511 - Revise the format of the GC profiler markers

This change has a corresponding change in perf.html, where I add complete type information for the JSON in these markers: https://github.com/devtools-html/perf.html/pull/539

They do not need to be committed at the same time, and there will be some incompatibility which will degrade but not break perf.html.

Comment 7

[Paul Bone [:pbone]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b3f222ef8ad5652d1d160800bdb9c6bb90f80dfb

Talos: https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=b3f222ef8ad5652d1d160800bdb9c6bb90f80dfb

Comment 8

[Paul Bone [:pbone]]

Attachment: Bug 1386511 - Revise the format of the GC profiler markers r=jonco

Updated to apply cleanly after chagnes to 1392511. r+ carried forward.

Comment 9

[Julien Wajsberg [:julienw]]

Hey Paul,

I'm removing checkin-needed for now, as I'd like to first make sure we don't need to increment the profile format version too.

NI Markus on this.

Thanks !

Comment 10

[Paul Bone [:pbone]]

Okay,

I don't think we need to increment it, since the conversions that I added to perf.html yesterday take care of everything, but I'm happy to wait for Markus' answer.

Cheers.

Comment 11

[Markus Stange [:mstange]]

I'm pretty sure that you do need to increment the version. I wrote a longer explanation at https://github.com/devtools-html/perf.html/pull/539#issuecomment-327857230 .

If you don't increment the version in the Gecko profile, the consumer of the profile won't know what shape to expect in the markers.

Comment 12

[Paul Bone [:pbone]]

Attachment: Bug 1386511 - Revise the format of the GC profiler markers r=jonco

This is the same as the previous version (r+ carried forward) but it also increments the profile version number.  This seems to be the only change in Gecko I need to make for a new version of the profile format, is that right Markus?

Comment 13

[Markus Stange [:mstange]]

Comment on attachment 8906920 [details] [diff] [review]
Bug 1386511 - Revise the format of the GC profiler markers r=jonco

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

That's right.

Comment 14

[Paul Bone [:pbone]]

Attachment: Bug 1386511 - Use Microseconds for all phase times

Hi Jon,

I noticed that the phase times arn't all in the same units.  I'd like to make them all microseconds.  Will this break compatibility with anything other than perf-html (which I will handle).

I'll fold this patch into the existing one since it's so tiny and covered by the version bump in that other change.

Comment 15

[Jon Coppeard (:jonco)]

Comment on attachment 8907957 [details] [diff] [review]
Bug 1386511 - Use Microseconds for all phase times

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

Yes, the JSON data for selected GCs is included as part of the browser telemetry.  I don't know where that ends getting processed up but this change will break it.

I know minor and major GCs use different time units and this is not ideal, but I also don't think it's really a big deal.  Let's just leave this.

Comment 16

[Paul Bone [:pbone]]

(In reply to Jon Coppeard (:jonco) from comment #15)

> Yes, the JSON data for selected GCs is included as part of the browser
> telemetry.  I don't know where that ends getting processed up but this
> change will break it.

Some of the fields ahve changed names in my previous patch.  Won't that then break the telemetry data?

> I know minor and major GCs use different time units and this is not ideal,
> but I also don't think it's really a big deal.  Let's just leave this.

I agree, it's not that important, and perf.html has a step when it reads a profile can can tidy up this kind of thing.

Comment 17

[Paul Bone [:pbone]]

(In reply to Paul Bone [:pbone] from comment #16)
> (In reply to Jon Coppeard (:jonco) from comment #15)
> 
> > Yes, the JSON data for selected GCs is included as part of the browser
> > telemetry.  I don't know where that ends getting processed up but this
> > change will break it.
> 
> Some of the fields ahve changed names in my previous patch.  Won't that then
> break the telemetry data?

Hi Jon, your respose made me think that this (renaming and changing other JSON fields) might be a problem.  What do you think?  I'd like to get this on the 57 train so that any perf issues that come up in that version will have nicer profiling data.

Comment 18

[Jon Coppeard (:jonco)]

(In reply to Paul Bone [:pbone] from comment #17)
> > Some of the fields ahve changed names in my previous patch.  Won't that then
> > break the telemetry data?

Yes, it probably will.

On one hand I'm inclined to say this is not as bad as changing the units for a field: a missing field will be noticed immediately, whereas a change in meaning may slip though undetected.

However, it's probably best if we don't do this at all since I don't know exactly how this data is being used.

Can we keep the original names and preserve compatibility?

Comment 19

[Paul Bone [:pbone]]

(In reply to Jon Coppeard (:jonco) from comment #18)
> (In reply to Paul Bone [:pbone] from comment #17)
> > > Some of the fields ahve changed names in my previous patch.  Won't that then
> > > break the telemetry data?
> 
> Yes, it probably will.
> 
> On one hand I'm inclined to say this is not as bad as changing the units for
> a field: a missing field will be noticed immediately, whereas a change in
> meaning may slip though undetected.
> 
> However, it's probably best if we don't do this at all since I don't know
> exactly how this data is being used.
> 
> Can we keep the original names and preserve compatibility?

Mostly:

 + Some are new fields I've added, so those are okay,
 + Some fields I removed because they can be calculated from others.  For example nursery promotion rate can be   
   calculated by the tenured amount divided by the nursery size.  I want to keep leaning towards removing thouse, and 
   calculating them later but I don't know of the telemetry system is written to handle that.
 + Others had stupid names "nursery_bytes" which i renamed to something more meanigful "bytes_used" (compared with 
   capacity).
 + There were also others where I changed the units, such as mmu_20ms.
 + And there was one case where two names clashed, so I _had_ to rename one.

I made these types of changes to all three GC marker types. GCMinor, GCSlice and GCMajor.  In the case of GCMinor these have only been in nightly for a little while.  So I feel that breaking them now is okay, since not much will depend on them.  For the others I'm happy not to agressively rename them.

Comment 20

[Paul Bone [:pbone]]

Attachment: Bug 1386511 - Revise the format of the GC profiler markers

Since my earlier patch would have caused problems with telemetry, maybe this one is a better comprimise.

Comment 21

[Jon Coppeard (:jonco)]

(In reply to Paul Bone [:pbone] from comment #19)
> I made these types of changes to all three GC marker types. GCMinor, GCSlice
> and GCMajor.  In the case of GCMinor these have only been in nightly for a
> little while.  So I feel that breaking them now is okay, since not much will
> depend on them.  For the others I'm happy not to agressively rename them.

Great, that sounds fine to me.

Comment 22

[Paul Bone [:pbone]]

This has landed and is live on perf.html now. So we're ready to land this change.

Thanks.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3159df8aca1a
Revise the format of the GC profiler markers. r=jonco

Comment 24

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/ac5d290791da for timeouts in toolkit/components/telemetry/tests/browser/browser_TelemetryGC.js like https://treeherder.mozilla.org/logviewer.html#?job_id=138379397&repo=mozilla-inbound

Comment 25

[Paul Bone [:pbone]]

I see the problem.  I have broken telemetry compatability.  In this case I want to because there was a problem with the JSON format that I'm correcting.  I'm seeking help from telemetry people to see what they would advice to fix the origianal format without breaking too much compatibility.

Comment 26

[Georg Fritzsche [:gfritzsche]]

billm added the original version in bug 1308040, but i see sfink is looped in here already.

You will need to update:
- this test: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/tests/browser/browser_TelemetryGC.js
- this documentation: https://dxr.mozilla.org/mozilla-central/rev/69e24c678a28dc46a4c1bda3ff04b2f6106ff71a/toolkit/components/telemetry/docs/data/main-ping.rst#453

The ping schemas only track "gc" as an "object", so no change should be needed there:
https://github.com/mozilla-services/mozilla-pipeline-schemas/blob/master/schemas/telemetry/main/main.4.schema.json

I don't know who the specific consumers of the data are.
We don't use this data in our general datasets.
Is perf.html the only consumer of this data or are there others that need changing?

Comment 27

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

Assuming I'm understanding what this is about, I believe the only way this Telemetry data is used is via more or less manual explorations using IPython notebooks at analysis.telemetry.mozilla.org. There are some sample scripts available for walking over the data that could be updated, but generally speaking, I don't think you need to worry about breaking their expectations; you kind of need to tailor the code to whatever you're looking for anyway.

I'm another consumer; I have lots of profiles stashed away that I explore via my json tool. But I am totally fine with this stuff changing. I always look at the data to remind myself of the schema anyway.

Comment 28

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

I should mention that there are examples of the telemetry processing in bug 1314828.

Comment 29

[Paul Bone [:pbone]]

Thanks!

jonco expressed some concern about telemetry in a review of one of my earlier patches.  I changed the patch so that it altered the GCSlice and GCMajor markers less.  Jon, when you were concerned was there something specific you were thinking of that I might break?

Comment 30

[Jon Coppeard (:jonco)]

(In reply to Paul Bone [:pbone] from comment #29)
There weren't any specific things I was concerned about breaking.  This change sounds like it will be fine.

Comment 31

[Paul Bone [:pbone]]

Attachment: Bug 1386511 - Revise the format of the GC profiler markers

Thanks for the pointers Georg, this should be correct.

Comment 32

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8922574 [details] [diff] [review]
Bug 1386511 - Revise the format of the GC profiler markers

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

This looks like the right places in toolkit/components/telemetry to change.
I can't judge if the format is correct and can't review the js/ and tools/ code.

My main question here: Is there a need to increase the number of keys?

::: toolkit/components/telemetry/GCTelemetry.jsm
@@ +110,4 @@
>  // make sure to update the JSON schema at:
>  // https://github.com/mozilla-services/mozilla-pipeline-schemas/blob/master/telemetry/main.schema.json
>  // You should also adjust browser_TelemetryGC.js.
> +const MAX_GC_KEYS = 30;

This will have payload size impact.
Was the previous number of keys not sufficient?

::: toolkit/components/telemetry/tests/browser/browser_TelemetryGC.js
@@ +45,4 @@
>  
>        // Sanity check the GC data.
> +      ok("status" in gc, "status field present");
> +      ok(gc.status === "completed", "status field correct");

This can use `equal()`.
See: https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Assert.jsm

@@ +51,5 @@
>  
> +      ok("slices_list" in gc, "slices_list field present");
> +      ok(Array.isArray(gc.slices_list), "slices_list is an array");
> +      ok(gc.slices_list.length > 0, "slices_list array non-empty");
> +      ok(gc.slices_list.length <= 4, "slices_list array is not too long");

While this is changed - it can use `greater()` and `lessOrEqual()`.

Comment 33

[Paul Bone [:pbone]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #32)
> 
> This looks like the right places in toolkit/components/telemetry to change.
> I can't judge if the format is correct and can't review the js/ and tools/
> code.

That's fine, jonco and mstange already reviewed those parts respectively, I should probably r= all three of you.

> My main question here: Is there a need to increase the number of keys?

Yes, it crashed without it.  I wrote it this way to preseve bnackward compatability with telemetry before I understood that telemetery didn't depend on this so strictly.  We could probably reduce the size a bit now that I understand exactly what depends on this, but I'd prefer to do that seperately since the corresponding changes have already landed in perf.html and I'd like to land this to bring them into sync.

I did:

    ok(gc.status.equal("completed"), "status field correct");

But it didn't work:

    toolkit/components/telemetry/tests/browser/browser_TelemetryGC.js:48 - TypeError: gc.status.equal is not a function

equals didn't work either.

Likewise greater and lessOrEqual didn't work either.  Maybe I misunderstood.  I'm a beginner at JavaScript.

I havn't updated the patch.

Thanks.

Comment 34

[Paul Bone [:pbone]]

I have filed bug 1412729 for a later change to reduce the size of the telemetry data.

Comment 35

[Julien Wajsberg [:julienw]]

About equal/greater/lessOrEqual, I think Georg suggests to use the Assert library (see [1]).
For example: 

  Assert.equal(gc.status, "completed", message)

Although this one could just use Mochitests' `is` :)

But MDN says "Usage of these functions is strongly discouraged in mochitest-plain and mochitest-chrome tests"...

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Projects/Mochitest#Test_functions

Comment 36

[Paul Bone [:pbone]]

Attachment: Bug 1386511 (part 1) - Revise the format of the GC profiler markers r=jonco r=mstange

The ok/is/isnot methods are proferred rather than Assert methods. But I did change one of the === to an "is" call.

Comment 37

[Paul Bone [:pbone]]

Attachment: Bug 1386511 (part 2) - Improve style in TelemetryGC test

I have also changed some other uses of == === and != to is/isnot.

Comment 38

[Georg Fritzsche [:gfritzsche]]

(In reply to Paul Bone [:pbone] from comment #33)
> > My main question here: Is there a need to increase the number of keys?
> 
> Yes, it crashed without it.  I wrote it this way to preseve bnackward
> compatability with telemetry before I understood that telemetery didn't
> depend on this so strictly.  We could probably reduce the size a bit now
> that I understand exactly what depends on this, but I'd prefer to do that
> seperately since the corresponding changes have already landed in perf.html
> and I'd like to land this to bring them into sync.

Makes sense, ideally this would aim to land on the same Firefox version/train then.

Sorry for the delay!

Comment 39

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8923669 [details] [diff] [review]
Bug 1386511 (part 1) - Revise the format of the GC profiler markers r=jonco r=mstange

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

This looks good from the Telemetry side.

Comment 40

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8923670 [details] [diff] [review]
Bug 1386511 (part 2) - Improve style in TelemetryGC test

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

Thanks!

Comment 41

[Paul Bone [:pbone]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #38)
> (In reply to Paul Bone [:pbone] from comment #33)
> > > My main question here: Is there a need to increase the number of keys?
> > 
> > Yes, it crashed without it.  I wrote it this way to preseve bnackward
> > compatability with telemetry before I understood that telemetery didn't
> > depend on this so strictly.  We could probably reduce the size a bit now
> > that I understand exactly what depends on this, but I'd prefer to do that
> > seperately since the corresponding changes have already landed in perf.html
> > and I'd like to land this to bring them into sync.
> 
> Makes sense, ideally this would aim to land on the same Firefox
> version/train then.

I'll see what I can do.

> Sorry for the delay!

No problem.

Comment 42

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ae1ff4001aa
Part 1: Revise the format of the GC profiler markers. r=jonco, r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/463e74189171
Part 2: Improve style in TelemetryGC test. r=gfritzsche

Comment 43

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/7ae1ff4001aa
https://hg.mozilla.org/mozilla-central/rev/463e74189171