1197613 - GC Markers in filter list should be generalized

Status: RESOLVED FIXED

(DevTools :: Performance Tools (Profiler/Timeline), defect)

Description

[Jordan Santell [:jsantell] [@jsantell]]

Right now it's "Incremental GC", and should just be "Garbage Collection", including both inc and non-inc GC markers (this is just a label change)

Comment 1

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1197613-generalized-gc.patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4632dd68d729

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8653865 [details] [diff] [review]
1197613-generalized-gc.patch

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

I don't really understand this change, what its doing, and what the goal is?

::: browser/devtools/performance/modules/logic/marker-utils.js
@@ +354,5 @@
>      return marker.name || L10N.getStr("marker.label.unknown");
>    },
>  
> +  GCLabel: function (marker) {
> +    if (!marker) {

Why wouldn't we have a marker?

::: browser/locales/en-US/chrome/browser/devtools/markers.properties
@@ +22,5 @@
>  marker.label.parseHTML=Parse HTML
>  marker.label.parseXML=Parse XML
>  marker.label.domevent=DOM Event
>  marker.label.consoleTime=Console
> +marker.label.garbageCollection2=Garbage Collection

So is this only used for non-incremental GCs?

I feel it is more important to highlight non-incremental GCs than incremental. By having a "normal" GC and a "incremental GC" we are highlighting the latter. It should be the other way: we should have "non-incremental GC" and a "normal gc" to put highlight on the worse case.

Comment 3

[Jordan Santell [:jsantell] [@jsantell]]

The 'label' function is used to generate titles for each marker -- so either "Non-incremental GC" or "Incremental GC" in this case. The same function is also used to generate the generalized title, when no marker is present, for the filter marker list. Right now the filter list has "Incremental GC" for all GC markers, which is not correct or accurate.

This leaves markers with the same names as they do now, just changes the filter title for GC markers as "Garbage Collection"

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Ok that makes a lot more sense, thank you.

I have to ask though: why is it using the same function but with a special case of not passing in an actual marker? It seems like the function is providing an answer to two different questions depending on the parameters passed, which makes me feel that there should be a different function for each question. File a follow up?

Comment 5

[Jordan Santell [:jsantell] [@jsantell]]

There's a follow up to handle grouping markers bug 1176540 (which would fix this, as well as get rid of having 2 CC marker types in the filter list, for example)

Comment 6

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Would that separate enumerating marker categories from the function to get a specific marker's category from the function to get a specific marker's label? Having those all entangled in the same function is what I see as suboptimal here.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/6c0573155c27

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6c0573155c27