1132765 - New performance tool should use MemoryActor's startRecordingAllocations' options

Status: RESOLVED FIXED

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

Description

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

Need to set a sampling probability for dat speed and make the log big so we don't lose data.

Comment 1

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

Actually, I don't understand this whole mocked front stuff and can't figure out how to pass the options object to MemoryFront.startRecordingAllocations.

We should pass an object like:

{
  // Should default to like 1/20 or something. Play with it and find a nice default...
  probability: someProbabilityPref,

  // Biggest integer that fits in a signed 32 bit int. Keep all the data!
  maxLogLength: Math.pow(2, 31)
}

Comment 2

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

woops

Comment 3

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

Attachment: 1132765-allocations-options.patch

Two new prefs for these options. Modified ViewHelper's Prefs to handle floats. Added new method to memory actor just for fetching config for tests.

Comment 4

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

Comment on attachment 8574957 [details] [diff] [review]
1132765-allocations-options.patch

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

::: browser/app/profile/firefox.js
@@ +1455,5 @@
>  pref("devtools.profiler.ui.show-platform-data", false);
>  pref("devtools.profiler.ui.show-idle-blocks", true);
>  
>  // The default Performance UI settings
> +pref("devtools.performance.memory.sample-probability", "0.05");

Why is this a string and not a number?

@@ +1456,5 @@
>  pref("devtools.profiler.ui.show-idle-blocks", true);
>  
>  // The default Performance UI settings
> +pref("devtools.performance.memory.sample-probability", "0.05");
> +pref("devtools.performance.memory.max-log-length", 2147483648);

Why not just do `Math.pow(2, 31)` so that we don't have a magic number here? Or at least document what this magical number is...

Comment 5

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

(In reply to Nick Fitzgerald [:fitzgen] from comment #4)
> Comment on attachment 8574957 [details] [diff] [review]
> 1132765-allocations-options.patch
> 
> Review of attachment 8574957 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/app/profile/firefox.js
> @@ +1455,5 @@
> >  pref("devtools.profiler.ui.show-platform-data", false);
> >  pref("devtools.profiler.ui.show-idle-blocks", true);
> >  
> >  // The default Performance UI settings
> > +pref("devtools.performance.memory.sample-probability", "0.05");
> 
> Why is this a string and not a number?
> 

We don't have Float prefs afaik, and complex values would be overkill for this.

Comment 6

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

Comment on attachment 8574957 [details] [diff] [review]
1132765-allocations-options.patch

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

r- just because of firefox.js butchering. See comments below.

::: browser/app/profile/firefox.js
@@ -199,5 @@
>  // Show the Update Checking/Ready UI when the user was idle for x seconds
>  pref("app.update.idletime", 60);
>  
>  // Whether or not we show a dialog box informing the user that the update was
> -// successfully applied. This is off in Firefox by default since we show a 

You probably don't want to modify this file to this extend. There's a lot of whitespace removed, which will confuse people trying to blame this file later.

::: browser/devtools/performance/test/browser_perf-options-allocations.js
@@ +10,5 @@
> +  let { EVENTS, PerformanceController, $, gFront } = panel.panelWin;
> +  Services.prefs.setBoolPref(MEMORY_PREF, true);
> +
> +  let originalProbability = Services.prefs.getCharPref("devtools.performance.memory.sample-probability");
> +  let originalLogLength = Services.prefs.getIntPref("devtools.performance.memory.max-log-length");

Should maybe make these consts in head.js?

::: browser/devtools/shared/test/browser_prefs.js
@@ -1,1 @@
> -/* Any copyright is dedicated to the Public Domain.

Jordan, hg move is your friend.

Or, here's the trick I use: rename a file normally, then immediately qref. After that, I can continue editing stuff normally and hg won't consider the file moved, but renamed.

::: toolkit/devtools/server/actors/memory.js
@@ +232,5 @@
> +  },
> +  `getting allocations settings`), {
> +    request: {},
> +    response: {
> +      options: RetVal(0, "nullable:json")

Why is this nullable?

Comment 7

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

Comment on attachment 8574957 [details] [diff] [review]
1132765-allocations-options.patch

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

::: browser/app/profile/firefox.js
@@ -199,5 @@
>  // Show the Update Checking/Ready UI when the user was idle for x seconds
>  pref("app.update.idletime", 60);
>  
>  // Whether or not we show a dialog box informing the user that the update was
> -// successfully applied. This is off in Firefox by default since we show a 

+1

@@ +1455,5 @@
>  pref("devtools.profiler.ui.show-platform-data", false);
>  pref("devtools.profiler.ui.show-idle-blocks", true);
>  
>  // The default Performance UI settings
> +pref("devtools.performance.memory.sample-probability", "0.05");

Victor mentioned below, we don't have a Float type

@@ +1456,5 @@
>  pref("devtools.profiler.ui.show-idle-blocks", true);
>  
>  // The default Performance UI settings
> +pref("devtools.performance.memory.sample-probability", "0.05");
> +pref("devtools.performance.memory.max-log-length", 2147483648);

Didn't even know you could do REAL JS here, and top of the header says no computed values anyway, so a comment is a good idea

::: browser/devtools/performance/test/browser_perf-options-allocations.js
@@ +10,5 @@
> +  let { EVENTS, PerformanceController, $, gFront } = panel.panelWin;
> +  Services.prefs.setBoolPref(MEMORY_PREF, true);
> +
> +  let originalProbability = Services.prefs.getCharPref("devtools.performance.memory.sample-probability");
> +  let originalLogLength = Services.prefs.getIntPref("devtools.performance.memory.max-log-length");

Thought about that, but figured these won't be used elsewhere, realistically, but can go either way

::: browser/devtools/shared/test/browser_prefs.js
@@ -1,1 @@
> -/* Any copyright is dedicated to the Public Domain.

I think I just got git format-patch working with renamed files, so wooo, no hg!

::: toolkit/devtools/server/actors/memory.js
@@ +232,5 @@
> +  },
> +  `getting allocations settings`), {
> +    request: {},
> +    response: {
> +      options: RetVal(0, "nullable:json")

nuking

Comment 8

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

Attachment: 1132765-allocations-options.patch

Round 2, now with renames

Comment 9

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

Only running on gum for now

Comment 10

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

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

Comment 11

[Carsten Book [:Tomcat]]

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