Closed Bug 1154239 Opened 5 years ago Closed 5 years ago

Change the API of PerformanceStats.jsm for modularity and asynchronicity

Categories

(Toolkit :: Performance Monitoring, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: Yoric, Assigned: Yoric)

References

Details

Attachments

(1 file, 2 obsolete files)

At the moment, PerformanceStats.jsm may be used to monitor:
1. CPOW usage;
2. activations;
3. jank.

In a near future, we will add:
4. monitoring per-compartment (in addition to per-addon/per-webpage/per-platform) (bug 1147664);

And in the longer term, we will add:
5. monitoring memory use.

Performance-wise, 1. and 2. are basically free, 3. causes some regressions (bug 1152930), 4. is known to be quite slower (~2-3x slower than 3. in my experiments) and we have no idea about the performance cost of 5.

After brainstorming in the perf team, we decide that the best way forward was to decouple the various data we can probe, so as to:
- e.g. let us activate per-compartment monitoring only on Talos, or only when looking at about:performance, etc.
- let us activate/deactivate e.g. jank monitoring to make it work only when we are not loading a page, or only when looking at about:performance, etc.

Also, future evolutions of the low-level code may require going async, so let's take this opportunity to return snapshots asynchronously.
Attached patch Making the API modular and async (obsolete) — Splinter Review
Assignee: nobody → dteller
Attachment #8592754 - Flags: review?(dtownsend)
Comment on attachment 8592754 [details] [diff] [review]
Making the API modular and async

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

Going to finish up this review tomorrow but wanted to post this to make sure it doesn't get lost. All looking pretty good so far.

::: toolkit/components/aboutperformance/content/aboutPerformance.js
@@ +194,4 @@
>  
> +    dump(`about:performance deltas: ${JSON.stringify(deltas, null, "\t")}\n`);
> +    for (let item of [deltas.process, ...deltas.components]) {
> +      dump(`about:performance component: ${JSON.stringify(item, null, "\t")}\n`);

Remove the dumps before landing.

::: toolkit/components/perfmonitoring/PerformanceStats.jsm
@@ +257,5 @@
> +    },
> +    substract: function(a, b) {
> +      if (!b) {
> +        return a;
> +      }

This is unnecessary

@@ +269,5 @@
> +
> +/**
> + * A monitor for a set of probes.
> + *
> + * Keeping a monitor active when it is unused

This sentence trails off, is it meant to be here?

@@ +317,5 @@
> +    if (!this._finalizer) {
> +      throw new Error("dispose() has already been called, this PerformanceMonitor is not usable anymore");
> +    }
> +    // Current implementation is actually synchronous.
> +    return Promise.resolve().then(() => new Snapshot({

Why the .then here?

@@ +371,5 @@
> +
> +  // Prepare monitor and finalization mechanism.
> +  let id = this.makeId();
> +  let monitor = new PerformanceMonitor({id, probes: probeNames});
> +  this._monitors.set(id, probeNames);

Given that it is the PerformanceMonitor constructor that sets up the finalization watcher to handle releasing the probes I'd rather make that also acquire the probes and set them in the map. It makes it impossible for future code to cause a problem by accidentally bypassing PerformanceMonitor.make

@@ +496,5 @@
>    }
>  
> +  for (let probeName of Object.keys(Probes)) {
> +    let other = old ? old[probeName] : null;
> +    this[probeName] = Probes[probeName].substract(current[probeName], other);

This assumes that current is a monitor for every probe. You either need to pass a list of probes or check current[probeName] exists.

::: toolkit/components/perfmonitoring/tests/browser/browser_compartments.js
@@ +27,4 @@
>    addMessageListener("compartments-test:getStatistics", () => {
>      try {
> +      monitor.promiseSnapshot().then(snapshot => {
> +        sendAsyncMessage("compartments-test:getStatistics", snapshot);        

Nit: trailing whitespace
Comment on attachment 8592754 [details] [diff] [review]
Making the API modular and async

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

Looks good, one question here though

::: toolkit/components/perfmonitoring/AddonWatcher.jsm
@@ +26,5 @@
>                                    "resource://gre/modules/Services.jsm");
>  
> +const FILTERS = [
> +  {probe:"jank", field: "longestDuration"},
> +  {probe:"cpow", field:"totalCPOWTime"},

Nit: spaces after :

::: toolkit/components/perfmonitoring/PerformanceStats.jsm
@@ +370,5 @@
> +  }
> +
> +  // Prepare monitor and finalization mechanism.
> +  let id = this.makeId();
> +  let monitor = new PerformanceMonitor({id, probes: probeNames});

I wonder if passing around probe names all the time is the right thing. Why not just pass around the probe implementations instead? Saves all the indexes into Probes throughout this file.
Attachment #8592754 - Flags: review?(dtownsend)
Attached file MozReview Request: bz://1154239/Yoric (obsolete) —
/r/7325 - Bug 1154239 - Rework PerformanceStats.jsm for modularity and asynchronicity;r=Mossop

Pull down this commit:

hg pull -r 8e4872b35aabe7eeb4c365bf49a052c98187fb29 https://reviewboard-hg.mozilla.org/gecko/
Attachment #8594848 - Flags: review?(dtownsend)
Dave: reviewboard had internal errors when I submitted this for review and it doesn't show up on my dashboard, so I'm adding a needinfo? to make sure that this shows up somewhere on yours.
Flags: needinfo?(dtownsend)
(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #5)
> Dave: reviewboard had internal errors when I submitted this for review and
> it doesn't show up on my dashboard, so I'm adding a needinfo? to make sure
> that this shows up somewhere on yours.

reviewboard's dashboard isn't generally helpful to me, I go off bugzilla review requests.
Flags: needinfo?(dtownsend)
Comment on attachment 8594848 [details]
MozReview Request: bz://1154239/Yoric

https://reviewboard.mozilla.org/r/7323/#review6141

::: toolkit/components/perfmonitoring/AddonWatcher.jsm:29
(Diff revision 1)
> +=======

Pretty sure this isn't right!

::: toolkit/components/perfmonitoring/PerformanceStats.jsm:127
(Diff revision 1)
> +  substract: function(a, b) {

s/substract/subtract/ throughout

::: toolkit/components/perfmonitoring/PerformanceStats.jsm:269
(Diff revision 1)
> +      }

Per my last review this is unnecessary
Attachment #8594848 - Flags: review?(dtownsend)
Abandoned this review because it looks like you haven't addressed my earlier comments.
Sorry about that. I saw comment 3 but not comment 2.
(In reply to Dave Townsend [:mossop] from comment #2)
> @@ +317,5 @@
> > +    if (!this._finalizer) {
> > +      throw new Error("dispose() has already been called, this PerformanceMonitor is not usable anymore");
> > +    }
> > +    // Current implementation is actually synchronous.
> > +    return Promise.resolve().then(() => new Snapshot({
> 
> Why the .then here?

This forces execution to proceed on the next tick, to make sure that clients are not accidentally synchronous, in which case they will break once the underlying implementation becomes async. Also, as a side-effects, this has nice properties (i.e. without it, `getSnapshot()` won't be displaying the data from the calling tick, as it's not available yet).
(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo" - away until May 6th) from comment #10)
> (In reply to Dave Townsend [:mossop] from comment #2)
> > @@ +317,5 @@
> > > +    if (!this._finalizer) {
> > > +      throw new Error("dispose() has already been called, this PerformanceMonitor is not usable anymore");
> > > +    }
> > > +    // Current implementation is actually synchronous.
> > > +    return Promise.resolve().then(() => new Snapshot({
> > 
> > Why the .then here?
> 
> This forces execution to proceed on the next tick, to make sure that clients
> are not accidentally synchronous, in which case they will break once the
> underlying implementation becomes async. Also, as a side-effects, this has
> nice properties (i.e. without it, `getSnapshot()` won't be displaying the
> data from the calling tick, as it's not available yet).

The latter point is a good one. Just so I understand for the future, wouldn't just Promise.resolve(...) cause clients see it on the next tick anyway because they have to use .then themselves?
(In reply to Dave Townsend [:mossop] from comment #11)
> The latter point is a good one. Just so I understand for the future,
> wouldn't just Promise.resolve(...) cause clients see it on the next tick
> anyway because they have to use .then themselves?

Well, as any function call, `Promise.resolve(expr)` evaluates `expr` immediately, so we would see the result of calling `getSnapshot()` immediately, i.e. the result at the end of the previous tick.
(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo" - away until May 6th) from comment #12)
> (In reply to Dave Townsend [:mossop] from comment #11)
> > The latter point is a good one. Just so I understand for the future,
> > wouldn't just Promise.resolve(...) cause clients see it on the next tick
> > anyway because they have to use .then themselves?
> 
> Well, as any function call, `Promise.resolve(expr)` evaluates `expr`
> immediately, so we would see the result of calling `getSnapshot()`
> immediately, i.e. the result at the end of the previous tick.

Sorry I meant in terms of whether clients see the result asynchronously or not.
https://reviewboard.mozilla.org/r/7323/#review6301

> s/substract/subtract/ throughout

Ah, right. I'll fix this in the next iteration.
(In reply to Dave Townsend [:mossop] from comment #13)
> Sorry I meant in terms of whether clients see the result asynchronously or
> not.

Right.
Attachment #8594848 - Flags: review?(dtownsend)
Comment on attachment 8594848 [details]
MozReview Request: bz://1154239/Yoric

/r/7325 - Bug 1154239 - Rework PerformanceStats.jsm for modularity and asynchronicity;r=Mossop

Pull down this commit:

hg pull -r 0b35ce4ad963844c00c2d1d4d0be264ac9fbf0cb https://reviewboard-hg.mozilla.org/gecko/
Comment on attachment 8594848 [details]
MozReview Request: bz://1154239/Yoric

https://reviewboard.mozilla.org/r/7323/#review6991
Attachment #8594848 - Flags: review?(dtownsend) → review+
Bug 1154239 - Rework PerformanceStats.jsm for modularity and asynchronicity;r=Mossop
Attachment #8614620 - Flags: review?(dtownsend)
Comment on attachment 8614620 [details]
MozReview Request: Bug 1154239 - Rework PerformanceStats.jsm for modularity and asynchronicity;r=Mossop

Bug 1154239 - Rework PerformanceStats.jsm for modularity and asynchronicity;r=Mossop
Attachment #8614620 - Flags: review?(dtownsend)
Attachment #8592754 - Attachment is obsolete: true
Attachment #8594848 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/ca96c76db6a2
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.