Closed Bug 1386511 Opened 2 years ago Closed 2 years ago

Improve the formatting of GC markers

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox-esr52 --- wontfix
firefox56 --- wontfix
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: pbone, Assigned: pbone)

References

Details

Attachments

(2 files, 7 obsolete files)

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".
This is based on suggestions made by sfink in an e-mail.
Assignee: nobody → pbone
Status: NEW → ASSIGNED
I'm revising the formats of all three GC marker types.
Summary: Improve the formatting of GCMinor markers → Improve the formatting of GC markers
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.
Attachment #8898153 - Flags: review?(jcoppeard)
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 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.
Attachment #8898153 - Flags: review?(jcoppeard)
Depends on: 1392511
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.
Attachment #8898153 - Attachment is obsolete: true
Attachment #8900556 - Flags: review?(jcoppeard)
Attachment #8900556 - Flags: review?(jcoppeard) → review+
Updated to apply cleanly after chagnes to 1392511. r+ carried forward.
Attachment #8900556 - Attachment is obsolete: true
Attachment #8901033 - Flags: review+
Keywords: checkin-needed
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 !
Flags: needinfo?(mstange)
Keywords: checkin-needed
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.
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.
Flags: needinfo?(mstange)
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?
Attachment #8901033 - Attachment is obsolete: true
Flags: needinfo?(mstange)
Attachment #8906920 - Flags: review+
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.
Priority: -- → P3
Flags: needinfo?(mstange)
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.
Attachment #8907957 - Flags: review?(jcoppeard)
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.
Attachment #8907957 - Flags: review?(jcoppeard)
(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.
(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.
Flags: needinfo?(jcoppeard)
(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?
Flags: needinfo?(jcoppeard)
(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.
Since my earlier patch would have caused problems with telemetry, maybe this one is a better comprimise.
Attachment #8906920 - Attachment is obsolete: true
Attachment #8907957 - Attachment is obsolete: true
Attachment #8909656 - Flags: review?(jcoppeard)
(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.
Attachment #8909656 - Flags: review?(jcoppeard) → review+
This has landed and is live on perf.html now. So we're ready to land this change.

Thanks.
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3159df8aca1a
Revise the format of the GC profiler markers. r=jonco
Keywords: checkin-needed
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.
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?
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.
I should mention that there are examples of the telemetry processing in bug 1314828.
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?
Flags: needinfo?(jcoppeard)
(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.
Flags: needinfo?(jcoppeard)
Thanks for the pointers Georg, this should be correct.
Attachment #8909656 - Attachment is obsolete: true
Attachment #8922574 - Flags: review?(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()`.
Attachment #8922574 - Flags: review?(gfritzsche) → feedback+
(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.
Flags: needinfo?(gfritzsche)
I have filed bug 1412729 for a later change to reduce the size of the telemetry data.
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
The ok/is/isnot methods are proferred rather than Assert methods. But I did change one of the === to an "is" call.
Attachment #8922574 - Attachment is obsolete: true
Attachment #8923669 - Flags: review?(gfritzsche)
I have also changed some other uses of == === and != to is/isnot.
Attachment #8923670 - Flags: review?(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!
Flags: needinfo?(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.
Attachment #8923669 - Flags: review?(gfritzsche) → review+
Comment on attachment 8923670 [details] [diff] [review]
Bug 1386511 (part 2) - Improve style in TelemetryGC test

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

Thanks!
Attachment #8923670 - Flags: review?(gfritzsche) → review+
(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.
Keywords: checkin-needed
Blocks: 1412729
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7ae1ff4001aa
https://hg.mozilla.org/mozilla-central/rev/463e74189171
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.