GC Markers in filter list should be generalized

RESOLVED FIXED in Firefox 43

Status

DevTools
Performance Tools (Profiler/Timeline)
RESOLVED FIXED
3 years ago
10 days ago

People

(Reporter: jsantell, Assigned: jsantell)

Tracking

41 Branch
Firefox 43
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox43 fixed)

Details

Attachments

(1 attachment)

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)
(Assignee)

Updated

3 years ago
Blocks: 1191480
Created attachment 8653865 [details] [diff] [review]
1197613-generalized-gc.patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4632dd68d729
Attachment #8653865 - Flags: review?(nfitzgerald)
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.
Attachment #8653865 - Flags: review?(nfitzgerald)
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"
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?
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)
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.
https://hg.mozilla.org/mozilla-central/rev/6c0573155c27
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox43: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43

Updated

10 days ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.