1128859 - Memory allocations views should be dependent on `enable-memory` pref

Status: RESOLVED FIXED

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

Comment 1

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

https://bugzilla.mozilla.org/show_bug.cgi?id=1122639#c14

Comment 2

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

Attachment: 1128859-enable-allocations-pref.patch

Comment 3

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

Comment on attachment 8559457 [details] [diff] [review]
1128859-enable-allocations-pref.patch

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

Sweet

::: browser/devtools/performance/modules/recording-model.js
@@ +19,5 @@
>  const RecordingModel = function (options={}) {
>    this._front = options.front;
>    this._performance = options.performance;
>    this._label = options.label || "";
> +  

Nit: ws.

::: browser/devtools/performance/performance-controller.js
@@ -57,5 @@
>  
>  // Events emitted by various objects in the panel.
>  const EVENTS = {
>    // Fired by the OptionsView when a preference changes.
> -  PREF_CHANGED: "Preformance:PrefChanged",

Lolol

@@ +234,4 @@
>  
>      this.emit(EVENTS.RECORDING_WILL_STOP, recording);
> +
> +    yield recording.stopRecording({ withAllocations });

Can't `stopRecording` just read from its own model configuration, instead of expecting an argument?

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

Would be better if we made sure everything works when both the MemoryCallTreeView and the MemoryFlameGraphView are toggled, not just the call tree.

::: browser/devtools/performance/test/browser_perf-options-enable-memory-02.js
@@ +18,5 @@
> +  Services.prefs.setBoolPref(MEMORY_PREF, true);
> +  yield stopRecording(panel);
> +
> +  is(PerformanceController.getCurrentRecording().getConfiguration().withAllocations, false,
> +    "The recording finished without tracking memory.");

Should check `withMemory` here as well.

@@ +25,5 @@
> +  yield startRecording(panel);
> +  Services.prefs.setBoolPref(MEMORY_PREF, false);
> +  yield stopRecording(panel);
> +  is(PerformanceController.getCurrentRecording().getConfiguration().withAllocations, true,
> +    "The recording finished with tracking memory.");

Ditto.

::: browser/devtools/performance/test/browser_perf-options-invert-call-tree-02.js
@@ +1,5 @@
>  /* Any copyright is dedicated to the Public Domain.
>     http://creativecommons.org/publicdomain/zero/1.0/ */
>  
>  const INVERT_PREF = "devtools.performance.ui.invert-call-tree";
> +const MEMORY_PREF = "devtools.performance.ui.enable-memory";

Maybe move all these consts the head.js?

::: browser/devtools/performance/views/details.js
@@ +68,5 @@
> +      if (!pref) {
> +        continue;
> +      }
> +      let value = PerformanceController.getPref(pref);
> +      // Hide the button that selects the view

Nit: somewhat redundant comment.

Comment 4

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

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

Comment 5

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

Comment on attachment 8559457 [details] [diff] [review]
1128859-enable-allocations-pref.patch

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

::: browser/devtools/performance/performance-controller.js
@@ +234,4 @@
>  
>      this.emit(EVENTS.RECORDING_WILL_STOP, recording);
> +
> +    yield recording.stopRecording({ withAllocations });

Yeah I like that, if we continue with the assumption that these things can't change

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

+1

::: browser/devtools/performance/test/browser_perf-options-invert-call-tree-02.js
@@ +1,5 @@
>  /* Any copyright is dedicated to the Public Domain.
>     http://creativecommons.org/publicdomain/zero/1.0/ */
>  
>  const INVERT_PREF = "devtools.performance.ui.invert-call-tree";
> +const MEMORY_PREF = "devtools.performance.ui.enable-memory";

+1

Comment 6

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

Attachment: 1128859-enable-allocations-pref.patch

Comment 7

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

Attachment: 1128859-enable-allocations-pref.patch

rebased

Comment 8

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

There's a couple tests where stopRecording is used with a param, when that's not necessary anymore. Should clean those up, and there's a new case added in bug 1130200

Comment 9

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

needinfo? to make sure you see comment 8.

Comment 10

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

Attachment: 1128859-enable-allocations-pref.patch

Removed instances where stopRecording takes arguments

Comment 11

[Carsten Book [:Tomcat]]

Hi,

this patch failed to apply cleanly:

applying 1128859-enable-allocations-pref.patch
patching file browser/devtools/performance/performance-controller.js
Hunk #3 FAILED at 291
1 out of 3 hunks FAILED -- saving rejects to file browser/devtools/performance/performance-controller.js.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh 1128859-enable-allocations-pref.patch

could you take a look? thanks!

Comment 12

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

Rebased and landed myself: https://hg.mozilla.org/integration/fx-team/rev/4cff6f1458dd

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4cff6f1458dd