New performance tool should use MemoryActor's startRecordingAllocations' options

RESOLVED FIXED in Firefox 39

Status

RESOLVED FIXED
4 years ago
4 months ago

People

(Reporter: fitzgen, Assigned: jsantell)

Tracking

unspecified
Firefox 39
x86
Mac OS X
Dependency tree / graph

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Need to set a sampling probability for dat speed and make the log big so we don't lose data.
No longer depends on: 1132764
Depends on: 1132764
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)
}
Assignee: nfitzgerald → nobody
Status: ASSIGNED → NEW
(Assignee)

Updated

4 years ago
Assignee: nobody → jsantell
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
woops
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Created attachment 8574957 [details] [diff] [review]
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.
Attachment #8574957 - Flags: review?(vporof)
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...
(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 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?
Attachment #8574957 - Flags: review?(vporof) → review-
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
Created attachment 8575408 [details] [diff] [review]
1132765-allocations-options.patch

Round 2, now with renames
Attachment #8574957 - Attachment is obsolete: true
Attachment #8575408 - Flags: review?(vporof)
Attachment #8575408 - Flags: review?(vporof) → review+
Only running on gum for now
Keywords: checkin-needed
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e8f897f0af1b
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 39

Updated

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