You see a long GC pause in the timeline and click on it. If you were recording memory, then you should see a button that when clicked jumps you to the allocations view showing only the allocation between this GC and the previous one, since that is the set of allocations that triggered this GC. If you were not recording memory, then you should get a little message blurb about how you can record memory to track allocations that trigger GCs.

This sounds sweet. We should probably do something to make it apparent that this marker specifically is clickable, or make tooltips (displaying the same as the marker details view in the waterfall) with follow up info (like view previous allocation set)

Should this select from the current markers' start time, to the previous marker's end time, where the previous marker is the most recent GC, regardless of type (major/minor)? Also, CC markers have nothing to do with this, correct?

Yes, it should ignore CC markers. Given a target GC (the one whose button is clicked), let Start = the end of the last GC preceding the target GC or the start of the recording if the target GC is the first GC in the recording. Let End = the start of the target GC marker. Clicking the button should automatically select the range [Start, End] and switch to the allocations tab. So given these markers: |------------------------------------ DOM Event -----------------------------------------------| |------------ GC -----------| |------------ GC --------------| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ If you want to debug the latter GC, you would select the range highlighted with "~~~~~". We only have markers for major GCs, so you don't need to worry about major/minor distinction here. Your next question will likely be "what about incremental vs non-incremental"; I got you. Yes we want to do this the same way regardless if the last GC is incremental or not. In particular, if the last GC was incremental, and this one is not, you should be really interested in the allocations that contributed to the non-incremental GC, where previously you were being incremental!

Thanks, Nick -- all the info I need

Working around in the test to no longer need bug 1196945 due to that being a chain of 4 other bugs.

So this features hits every weirdness in the tool. Victor, r? for code, and Nick, r? for the feature as a whole. Some things I'm iffy on: * When selecting a range between GCx and GCy, the markers may appear in the selection. Should they? Should they not? * Often when playing with this, I may not get allocations in the range between two GCs. This seems really weird -- maybe we can check to see if there are allocations in that range first, but I'd imagine this being rather costly maybe? * When there is nothing in the range, we either get a blank table, or just an entry on (root) with 0 samples found. Not sure why this isn't consistent, Victor any ideas? Also made bug 1197526 for some way to solve this weirdness. * Nick please file another bug with more info if you think a message about turning on allocations on these markers is useful * When snapping over to the allocations call tree view, it's very disorienting. It's not clear to me I'm in a new "details view" -- we may solve this by making the selected details view button darker maybe? Right now there's no contextual relationship between the transition if you don't notice the details button highlight switch.

Comment on attachment 8651525 [details] [diff] [review] 1153477-alloc-buttons.patch Review of attachment 8651525 [details] [diff] [review]: ----------------------------------------------------------------- Nice

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #6) > * When selecting a range between GCx and GCy, the markers may appear in the > selection. Should they? Should they not? I'm not sure what you mean? Are you asking if the GC whose triggering allocations are being analyzed should be included in the selected range or not? It *shouldn't* matter because allocations can't happen during a GC. > * Often when playing with this, I may not get allocations in the range > between two GCs. This seems really weird -- maybe we can check to see if > there are allocations in that range first, but I'd imagine this being rather > costly maybe? There are, unfortunately, a lot of GC trigger heuristics that aren't great, and the GC team is working on improving them. Additionally, there are GC triggers that aren't caused by the webdev's (possibly mis-behaving) code. I think we should restrict this feature to only those triggers that are actionable to the webdev (eg caused by too much allocation in one way or another): * TOO_MUCH_MALLOC * ALLOC_TRIGGER * LAST_DITCH * EAGER_ALLOC_TRIGGER Additionally, I was wrong about selecting the allocations since the last GC marker. I just talked to Terrence, and what we want is the allocations since the end of the last gc *cycle* up until the current GC marker (slice), because if the mutator is allocating faster than the collector can keep up incrementally, then we need to account for all of those allocations, not just the ones since the last incremental slice. Here is a diagram: > |--Inc. GC (Cycle 1)--| > |--Inc. GC (Cycle 1)--| > |--Inc. GC (Cycle 2)--| > |--Non Inc. GC (cycle 2)--| > > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ Where "~~~~~" is the region whose allocations we are interested in when analyzing the last GC marker. Note that the onGarbageCollection hook is called at the end of each GC cycle, so you can use that to determine what gc cycle a marker is from and annotate it appropriately. If this is too hard I can easily add it to the objects given out by the hook directly. We can also add it to the allocations log entries if that would help as well. But neither should be needed in order to land this feature, they would just be clean ups. My hope is that these two changes should raise the signal v noise ratio a ton of this feature and we won't see things like no allocations in the selected region. > * Nick please file another bug with more info if you think a message about > turning on allocations on these markers is useful https://bugzilla.mozilla.org/show_bug.cgi?id=1197914 > * When snapping over to the allocations call tree view, it's very > disorienting. It's not clear to me I'm in a new "details view" -- we may > solve this by making the selected details view button darker maybe? Right > now there's no contextual relationship between the transition if you don't > notice the details button highlight switch. Slide the tabs in and out? #hashtagmaterialdesign

Comment on attachment 8651525 [details] [diff] [review] 1153477-alloc-buttons.patch Review of attachment 8651525 [details] [diff] [review]: ----------------------------------------------------------------- Looks nice! Just need to consider those two things I commented above before we ship this.

Oops thought I resubmitted this for review. Filed follow up bugs for the snapping (bug 1198019) and platform work for adding GC cycle IDs on the markers

Comment on attachment 8652129 [details] [diff] [review] 1153477-alloc-buttons.patch Review of attachment 8652129 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/app/profile/firefox.js @@ +1447,5 @@ > pref("devtools.performance.ui.enable-allocations", false); > pref("devtools.performance.ui.enable-framerate", true); > pref("devtools.performance.ui.enable-jit-optimizations", false); > +pref("devtools.performance.ui.show-triggers-for-gc-types", > + "TOO_MUCH_MALLOC ALLOC_TRIGGER LAST_DITCH EAGER_ALLOC_TRIGGER"); You prefer space separated to comma separated?

Comment on attachment 8652129 [details] [diff] [review] 1153477-alloc-buttons.patch Review of attachment 8652129 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/app/profile/firefox.js @@ +1447,5 @@ > pref("devtools.performance.ui.enable-allocations", false); > pref("devtools.performance.ui.enable-framerate", true); > pref("devtools.performance.ui.enable-jit-optimizations", false); > +pref("devtools.performance.ui.show-triggers-for-gc-types", > + "TOO_MUCH_MALLOC ALLOC_TRIGGER LAST_DITCH EAGER_ALLOC_TRIGGER"); If their enums, sure -- can do the weird hacky JSON thing later if we need

more logs https://treeherder.mozilla.org/#/jobs?repo=try&revision=d9ea15565a9b

Was failing due to CC_WAITING markers appearing and not being filtered out for allocs view (and had a negative start time which is possible with GC markers) https://treeherder.mozilla.org/#/jobs?repo=try&revision=1b822c587e22

I'm guessing this is failing for the same reason as bug 1161817, where the waterfall isn't matched up with marker details. Skipping this test, and landing, unfortunately.