1132758 - Toggling "record memory" hides memory data I've already recorded

Status: RESOLVED FIXED

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

Description

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

STR:

* enable memory recording

* make a recording

* view the recording

* disable memory recording

ER:

I can still see the memory data I already recorded in this recording but new ones won't have it.

AR:

The memory data I've already recorded goes away.

Comment 1

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

We already associate whether a recording had memory enabled or not. This would also flip on/off the memory details view options as well. Possible downside is, you don't see the "additional features" when enabling memory on a recording without memory to know what you're missing out on, unless re-recording, if that makes sense

Comment 2

[Victor Porof [:vporof][:vp]]

Not specifically blocking v2.

Comment 3

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

Attachment: 1132758-feature-based-views.patch

This one got away from me a bit.

Displaying the allocations details view suffers the same fate as the graphs, all based on the current status of a pref, and what we ultimately want is whether or not the current recording supports certain features. So now the overview graphs and details views are based on the configuration of a recording. Many tests were changed in this case, to not set the view before any recordings exist, because that doesn't really make sense if the views are based on recording configuration.

Recording configurations are now also saved on the profile. Useful to in the future display what the allocation precision and profiler frequency are as well.

The handling of overview graph rendering/graph-y stuff was consolidated into performance-graphs, so that overview view just handles the orchestration of the view.

All this makes future feature detection, and things like disabling components for Fx40, way easier.

Welcome back, Victor. 55 files changed, 895 insertions(+), 571 deletions(-)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=01fd8bd09e04

Comment 4

[Victor Porof [:vporof][:vp]]

ffu

Comment 5

[Victor Porof [:vporof][:vp]]

Comment on attachment 8598284 [details]
1132758-feature-based-views.patch

Review of attachment 8598284 [details]:
-----------------------------------------------------------------

T_T

But this was surprisingly easy to parse even though it was huge. A pattern I'm happy to find in all of your patches, Jordan :)

I'd like to see more file splitting and the comments/nits below addressed. Feel free to pick and choose, and ask for another r? if you feel it's necessary.

::: browser/devtools/performance/modules/graphs.js
@@ +111,5 @@
> +    this.dataDuration = duration;
> +    return this.setDataFromTimestamps(ticks, resolution);
> +  }
> +});
> +exports.FramerateGraph = FramerateGraph;

Nit: easier to understand this file if all exports were moved to the bottom.

@@ +144,5 @@
> +  setPerformanceData: MarkersOverview.prototype.setData
> +});
> +exports.TimelineGraph = TimelineGraph;
> +
> +const GRAPH_DEFINITIONS = {

Nit: document this.

@@ +170,5 @@
> + * @param {DOMElement} root
> + * @param {function} getBlueprint
> + * @param {function} getTheme
> + */
> +function GraphsController ({ definition, root, getBlueprint, getTheme }) {

I feel like this should live in it's own file.

@@ +201,5 @@
> +   * some graphs.
> +   * Saves rendering progress as a promise to be consumed by `destroy`,
> +   * to wait for cleaning up rendering during destruction.
> +   */
> +  render: function (recordingData, resolution) {

Nit: Make this entire function a Task.async, instead of returning a new task, since that does it for you automatically and is prettier.

render: Task.async(function*(recordingData, resolution) {
  ...

  yield this._rendering.promise;

  this._rendering = promise.defer();
  for (...) {}
  this._rendering.resolve();
}),

@@ +210,5 @@
> +    // especially when a recording is finished, and triggers a
> +    // fresh rendering at a higher rate
> +    let lastRender = this._rendering;
> +
> +    // TODO can this be done faster? parallel? Use less abstractions?

Not sure if parallel drawing gives us any pref benefits. Not even sure if it's possible (although each graph does live in it's own iframe). In any case, drawing sequentially is usually a better idea since it gives the impression of snappier UIs.

@@ +273,5 @@
> +    yield graph.ready();
> +    return graph;
> +  }),
> +
> +  enable: function (graphName, isEnabled) {

Nit: doc.

::: browser/devtools/performance/modules/io.js
@@ +151,5 @@
> +    profile: profilerData.profile,
> +    // Fake a configuration object here if there's tick data,
> +    // so that it can be rendered
> +    configuration: {
> +      withTicks: !!ticksData.length

Nit: for posterity, how about also adding the other flags as well?

::: browser/devtools/performance/modules/recording-model.js
@@ +271,5 @@
>      switch (eventName) {
>        // Accumulate timeline markers into an array. Furthermore, the timestamps
>        // do not have a zero epoch, so offset all of them by the start time.
>        case "markers": {
> +        if (!config.withMarkers) { return; }

Nit: not really a huge difference for now, but all of those returns should be breaks instead.

::: browser/devtools/performance/performance-controller.js
@@ +486,5 @@
> +  isFeatureSupported: function ({ features, actors, isRecording: shouldBeRecording }, recording) {
> +    recording = recording || this.getCurrentRecording();
> +    let recordingConfig = recording ? recording.getConfiguration() : {};
> +    let currentRecordingState = recording ? recording.isRecording() : void 0;
> +    let actorsSupported = gFront.getActorSupport(); 

Nit: whitespace.

::: browser/devtools/performance/test/browser_perf-details-03.js
@@ +8,5 @@
>   */
>  function spawnTest () {
>    let { panel } = yield initPerformance(SIMPLE_URL);
>    let { EVENTS, PerformanceController, OverviewView, DetailsView } = panel.panelWin;
> +  let { $, WaterfallView, MemoryCallTreeView, RecordingsView, MemoryFlameGraphView } = panel.panelWin;

Uber Nit: order those variables a bit to follow the UI structure:

let { $, RecordingsView, WaterfallView, MemoryCallTreeView, MemoryFlameGraphView } = panel.panelWin;

::: browser/devtools/performance/test/browser_perf-details-04.js
@@ +6,5 @@
>   * doesn't exist or is in progress.
>   */
>  function spawnTest () {
>    let { panel } = yield initPerformance(SIMPLE_URL);
> +  let { EVENTS, $, $$, WaterfallView, PerformanceController, RecordingsView, DetailsView } = panel.panelWin;

Uber nit: ditto:

let { EVENTS, $, $$, PerformanceController, RecordingsView, WaterfallView, DetailsView } = panel.panelWin;

::: browser/devtools/performance/views/details.js
@@ +113,5 @@
> +    //
> +    // 2. If we have a finished recording and no panel was selected yet,
> +    // use a default now that we have the recording configurations
> +    if ((invalidCurrentView && this._initialized && !isRecording) ||
> +        (recording && !isRecording && !this._initialized)) {

Nit: order these the same way:

(invalidCurrentView && this._initialized && !isRecording) ||
(recording && !this._initialized && !isRecording)) {

@@ +171,3 @@
>        return this.selectView("waterfall");
> +    } else {
> +      return this.selectView("js-calltree");

Nit: make a not here that the js call tree should always be supported since the profiler actor is as old as the world.

::: browser/devtools/performance/views/overview.js
@@ +294,5 @@
>    _checkSelection: Task.async(function* (recording) {
> +    let isEnabled = recording ? !recording.isRecording() : false;
> +    yield this.graphs.selectionEnabled(isEnabled);
> +  }),
> +  

Nit: whitespace.

Comment 6

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

Attachment: 1132758-feature-based-views.patch

Adopted all nits, except only added a few options to an imported legacy profiler data (like withMemory, etc was fine, but not sampleFrequency, as that doesn't really make sense to make up).

Comment 7

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

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

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fba992940a64