1189901 - Add tscroll-apz talos test

Status: RESOLVED FIXED

(Testing :: Talos, defect)

Description

[Benoit Girard (:BenWa)]

Right now tscroll isn't appropriate for measuring APZ improvements (and later catching regressions). It scroll via the main thread so it wont really interact with APZ the way a user would via touch/fling based scrolling.

IMO the TP pageset and tscroll is the thing we want, we just want to have a variant of tscroll, say tscroll-apz, that uses an async scroll api.

Comment 1

[Benoit Girard (:BenWa)]

Kats point out we have APZ scroll api. This might just be a matter of adding a flag to tscroll to run using this scroll api instead:

scrollTo(ScrollOptions)
http://mxr.mozilla.org/mozilla-central/source/dom/webidl/Window.webidl#190
http://mxr.mozilla.org/mozilla-central/source/dom/webidl/Window.webidl#150

Ther's also:
https://developer.mozilla.org/en-US/docs/Web/CSS/scroll-behavior

Comment 2

[Avi Halachmi (:avih)]

Well, there would be few differences:

1. Scroll trigger: we could use SCCOM, but this has non-linear progression which isn't best for us. kats suggested to add a pref to make it scroll linearly. This should work well for talos.

2. What do we measure. Few options:
- How many frames with checkerboarding (aka CB from now on).
- Accumulate visible CB from each frame during scroll.
- Measure how far ahead APZ has the data ready during scroll, and normalize it with the optimal look-ahead distance.

The third option is interesting for several reasons:
1. It can measure the effectiveness of APZ even without visible CB.
2. It's continuous, wheres the CB tests only measure when there is CB, so it won't have a cliff.
3. It's probably the most sensitive of the three approaches.

Problem, we need an API to fetch these two pieces of data (how far ahead we have the tiles, what's the optimal data-ahead distance).

Comment 3

[Benoit Girard (:BenWa)]

I prefer the third option. This way we wont miss regressions because have some extra room and we wont have a patch suddenly throw us over the boundary on a particular page complain about a sudden regression. It will show regression much more continuously.

The caveat is we really need to implement this right. It's tricky because the DisplayPort isn't constant. We need to deal with the edge of the page (easy) and also with the DisplayPort sizing heuristics (maybe hard?).

Comment 4

[Kartikaya Gupta (email:[e-mail])]

I was thinking about this and actually I would like to implement the test so that it synthesizes mouse wheel events and sends them directly into APZ. That is closest to what would happen in a real user scenario. We can use the exact same code that runs tp5o_scroll for example but replace the scrollBy call with a synthetic mouse wheel event.

For looping, I'm not sure if rAF makes the most sense, but I think it's not unreasonable. It's cheap and easy to start with and we can change it later if needed. It has the advantage that if we add more painting on the main thread it will impact the score.

For timing, I would want to measure time from start of first wheel event to when the main thread reaches the final scroll offset, as that is the end-to-time of everything that we care about.

I would also want to record the total checkerboard severity over this period and report that - we have machinery that can do this but will need some plumbing probably.

I think the easiest way to get started here is to make the scrolling code in the tp5o_scroll test switch between what it does now and the synthetic wheel approach based on some pref, and the Talos harness can set the pref for the new apz job. That will get us 90% of the way there. Thoughts?

Comment 5

[Joel Maher ( :jmaher ) (UTC -8)]

:kats, I am excited to see thoughts here.  Extending existing tests seems like the right thing to do.  I am sure others like :avih would have more detailed thoughts on this.

Comment 6

[Avi Halachmi (:avih)]

So before we look into implementation details, I think the biggest question IMO is what metrics we should use.

One obvious metric subject is checkeboarding, but within this subject, what do we actually accumulate such that it can reflect small perf changes reliably and continously?

One thing to keep in mind which we _don't_ want is to get a test which have "inherent thresholds" or result buckets, I.e. where perf can change with some tolerance with zero effect on the results, and then some threshold is crossed and suddenly the results start changing (or worse - just jump to the next bucket and then we're back at square 1).

I'd say the first thing we need to list is things the user can notice with APZ. I can think of the following:

1. Checkerboard "severity" (continuous metrics TBD).
2. Missed frames (i.e. not rendered in time to maintain 60fps).

For 2, to make the results non-discrete, I'd hope we could use ASAP mode to measure the actual throughput where small changes get reflected at the results continuously.

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Mostly agree with comment 6. One caveat about ASAP mode though: there are two different ASAP modes. One is controlled by the layout.frame_rate pref, and keeps layout/composition in lockstep. So that would look like this:

   V      V      V  V         V    
MT |------|------|--|---------|---- ...
CT |---+++|---+++|---|---+++++|---+ ...

MT is the main thread, CT is the compositor thread, the vertical bars are vsyncs that get posted, the '-' are busy times and the '+' are idle times. In this ASAP mode, vsyncs are posted whenever the main thread is done processing the previous vsync. So the main thread is going to busy all the time, and whenever it's not a new vsync/tick will start. The compositor thread, however, has a different amount of work to do. In my diagram above it always has three units of work, so each vsync is followed by three '-' for the compositor. The rest of the time it's idle, waiting for the next vsync, which will only get posted when the main thread is done. If the main thread is done faster than the compositor (such as on the third vsync above) then the compositor will just receive the vsync later. In general though the main thread being busy can hide regressions in the compositor, because we could go from 3 units of compositor work to 4 units of compositor work in this diagram without changing the end result.

The other ASAP mode, which can be enabled by setting layers.offmainthreadcomposition.frame-rate to 0, allows the compositor to also go full-ASAP so it would do this:

MT V------V------V--V---------V---- ...
CT V---V---V---V---V---V---V---V--- ...

I think this is what we want in order to catch regression in the compositor code, but it requires ensuring that the test exercises a fixed number of composites, and that the main thread is not the bottleneck.

Does that make sense?

One way you can see this in action is by first setting layout.frame_rate = 10 in your nightly build, and watch how everything slows down to 10fps. Then, leaving that pref as-is, also set layers.offmainthreadcomposition.frame-rate to 0, which lets the compositor go ASAP. With APZ, this will allow 60 FPS scrolling even though the main thread is plodding along at 10fps.

Comment 9

[Kartikaya Gupta (email:[e-mail])]

Also ni? mchang to read comment 8 and make sure I'm not talking crazy. I might be!

Comment 10

[Mason Chang [Inactive] [:mchang]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #9)
> Also ni? mchang to read comment 8 and make sure I'm not talking crazy. I
> might be!

Can confirm, not crazy :). Just a heads up with comment 8 where layout/composition is in lockstep. Technically there are no vsyncs in ASAP mode in lockstep mode. We run the nsRefreshDriver on a simple timer that schedules ASAP. Once the transaction is sent over to the compositor, it composites ASAP. The general gist is right, but instead of waiting for vsync, the compositor will composite once the refresh driver sends the transaction, not on the vsync in ASAP mode.

One question about this on Windows though. Don't input events come in on the main thread or do we synthesize them on the compositor thread / somewhere else?

Comment 11

[Kartikaya Gupta (email:[e-mail])]

Input events do come in on the main thread (in the parent process) on windows, yes.

Comment 12

[Avi Halachmi (:avih)]

Thanks!

After some discussion on IRC, we've identified the following subjects which could benefit from performance tracking:

1. Checkerboarding.
2. Compositor throughput on different use cases.
3. "kick in" duration (how long from trigger to actual scrolling).

We've mostly discussed 2 so far, which, combined with existing results of main-thread bottlenecks (our current scroll tests) might also imply to some degree on checkerboarding.

We agreed that 3 can be "a thing" but for now decided to put it aside.

To test compositor throughput, we'd need a sequence similar to this:

1. Enable "unlocked ASAP" mode.
2. Trigger APZ scroll (possibly with CSSOM DOM APIs).
3. Possibly identify when scroll actually starts.
4. Record composition intervals (probably) using Start[/Stop]FrameTimeRecording.
5. possibly identify when scrolling stops.
6. crunch the numbers into a reported value (e.g. drop some first/last numbers and average the rest).

For 3 and 5, we've think that setting the scroll duration to long enough (e.g. 5 seconds) and then dropping the first and last (say 1 second worth of intervals on each "side") might let us avoid 3 and 5.

We'll have to experiment a bit to make sure these things end up usable (e.g. with short pages, etc), but for now that's the plan and assumption.

As for which content to scroll, so far we have two main subjects: tp5[o] corpus, and the tscrollx pages.

While tp5 is interestingly varied, kats doesn't think it's varied enough to exercise all the important code paths for composition (and neither are the tscrollx pages).

kats identified at least three cases we'd want to test: "top-level scrolling, subframe scrolling, pages with/without fixed-pos content".

I think at some stage we'd like to add synthetic test cases, but we can start with the sets of pages we already have.

I can take it at some later stage, or, if someone wants to take it, I can help.

Comment 13

[Avi Halachmi (:avih)]

Here we go.

The following patches include some preparations which are otherwise mostly useless before they're required (e.g. by this APZ scroll test), so bear with me, and if some of them require extensive discussions, I'll spin them off to their own bugs.

The patches will enhance the existing scroll tests - tscrollx and tp5o_scroll, to also test APZ scroll.

The APZ test will include average composition interval and checkerboard severity.

So overall, instead of one result per page which we previously had (average synchronous scroll intervals), we will now have three, where <name> is the name we had before adding the APZ tests:
1. <name>                  - Same sync scroll we had before.
2. <name>.APZ              - Average composition intervals during APZ scroll.
3. <name>.apz.checkerboard - checkerboarding severity during APZ scroll.

Patches coming now.

Comment 14

[Avi Halachmi (:avih)]

Attachment: bug1189901.part1.remove-redundant-code.patch

Joel, this removes the duplicated scroll test code which we had for both tscrollx and tp5o_scroll, such that tscrollx ends up using the js code at the pageloader addon folder.

Comment 15

[Avi Halachmi (:avih)]

Attachment: bug1189901.part2.promises.patch

Mike, pretty much what you suggested on IRC few days ago. The indentations are awkward to make the diff smaller. I'll fix this after the review.

Comment 16

[Avi Halachmi (:avih)]

Attachment: bug1189901.part3.talosproxy-addon.patch

So, this adds an addon - talosproxy, which can be used within talos to execute functions which require elevated privileges or need to execute at the chrome process - such as the API which the APZ test requires.

It includes a sample service - getPref and sample usage just for demonstration.

Note that while this code works for tscrollx, apparently something with the invocation/injection of the API into the page doesn't work. That's why it's f? and not r?.

Part5 with the APZ test itself includes a workaround which creates the talosproxy object directly at the page instead of injecting it by the framescript, which makes it work also for tpo5o_scroll.

Once this gets solved properly, this workaround will be removed.

Comment 17

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8751316 [details] [diff] [review]
bug1189901.part1.remove-redundant-code.patch

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

sort of funky using code from a plugin as a raw file, but I would rather have that than duplicated code- thanks for the proper comments!

Comment 18

[Avi Halachmi (:avih)]

Attachment: bug1189901.part4.report-custom-name.patch

Joel, currently, for tscrollx and tp5o_scroll, the reported page names are calculated by the pageloader addon.

However, the page itself does not know the name which is reported for it. In order for each page to generate 3 results, it needs to know the name.

This patch only changes the scroll test to calculate the reported name on its own without help from the pageloader addon. The name it reports is identical to that which it was reported before.

The next part where the APZ tests are added will add 2 new test names to report.

Comment 19

[Avi Halachmi (:avih)]

Attachment: bug1189901.part5.add-apz-test.patch

So, finally the actual test.

It sets the APZ spring constant such that the scroll duration is roughly 1.5-2s (impossible to set it explicitly).

It then triggers smooth scroll from top to bottm, and measures the numbers 1000ms later.

WRT the checkerboarding severity, it currently accumulate the severity from all the checkerboard events during the scroll, and then normalizes this value by the number of compositions.

We can probably discuss this, as I was not sure what would be the best thing to report here.

Comment 20

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8751321 [details] [diff] [review]
bug1189901.part3.talosproxy-addon.patch

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

remember this will need to be signed prior to landing.

Comment 21

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8751323 [details] [diff] [review]
bug1189901.part4.report-custom-name.patch

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

the rest of the patch looks just fine, I would like to make the testBaseName more robust.

::: testing/talos/talos/pageloader/chrome/tscroll.js
@@ +17,5 @@
> +    values: [],
> +  };
> +  // We report multiple results, so we base the name on the file we're testing
> +  var testBaseName = win.location.href.split("tp5n/")[1]
> +                  || win.location.href.split("scroll/")[1];

this seems as though we will have issues accessing [1] if it doesn't exist, or if we reuse this for neither tp5n or scroll

Comment 22

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8751324 [details] [diff] [review]
bug1189901.part5.add-apz-test.patch

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

This looks good to me!

::: testing/talos/talos/pageloader/chrome/tscroll.js
@@ +227,5 @@
> +              return arr.length ? sum / (arr.length) : 0;
> +          }
> +
> +          // remove two frames on each side of the recording to get a cleaner result
> +          result.values.push(average(intervals.slice(2, intervals.length - 4)));

I believe slice takes start and end as parameters, rather than start and length, so you probably want (2, intervals.length - 2) here.

@@ +241,5 @@
> +                if (rts >= startts && rts < endts)
> +                  sum += reports[i].severity;
> +              }
> +              // Normalize the severity by number of compositions
> +              return sum / intervals.length;

intervals.length might be 0, probably want to guard against that.

Comment 23

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8751321 [details] [diff] [review]
bug1189901.part3.talosproxy-addon.patch

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

There's already a TalosPowers add-on that offers similar "powers" to Talos Tests. Could we not re-use that?

Comment 24

[Avi Halachmi (:avih)]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #23)
> There's already a TalosPowers add-on that offers similar "powers" to Talos
> Tests. Could we not re-use that?

Looks like we should be able to. Was not aware of that, thanks.

Looking at it though, It's not yet clear to me what steps should be taken to end up with a function which runs at the chrome process, can return a value, and how it should be accessed from the content.

Still examining the code.

Comment 25

[Avi Halachmi (:avih)]

(In reply to Avi Halachmi (:avih) from comment #24)
> (In reply to Mike Conley (:mconley) - Needinfo me! from comment #23)
> > There's already a TalosPowers add-on that offers similar "powers" to Talos
> > Tests. Could we not re-use that?
> 
> Looks like we should be able to. Was not aware of that, thanks.

It looks like the main framework which TalosPowers provides is the fact that it's an addon which is loaded automatically in talos. It is used to execute code at the parent process, but as far as I can tell each function implements the communication between the processes mostly its own.

Because I need 3 functions here, two of which also return a value, I've re-implemented the generic functionality of talosproxy from comment 16 at the TalosPowers addon, and intend to use it for start[/stop]FrameTimeRecording and the checkerboard service.

So far it works with a sample service at the parent but I still didn't integrate it to the scroll tests. Will hopefully finish that tomorrow.

Comment 26

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8751317 [details] [diff] [review]
bug1189901.part2.promises.patch

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

Hey avih,

Thanks! I like the transition to Promises. I have some general questions / suggestions - see below.

::: testing/talos/talos/pageloader/chrome/tscroll.js
@@ +12,1 @@
>      win = window;

The comment at the top of this file asks us to keep this file in sync with scroll-test.js. Is that not necessary anymore? If so, can you please remove those comments from the top of each file?

And if it _is_ necessary, can you sync them up?

And if it's nuanced, can you explain? :)

@@ +13,5 @@
>    }
>  
> +  var report;
> +
> +  function P_setupReportFn() {

Can you please add a docstring to this function? What it does, what it returns, etc.

@@ +26,5 @@
> +        return;
> +      }
> +
> +      // Not part of the test and does nothing if we're within talos.
> +      // Provides an alternative tpRecordTime (with some stats display) if running in a browser.

So it sounds like this file can be used in a couple of different contexts. Can you document those at the top of this file?

@@ +27,5 @@
> +      }
> +
> +      // Not part of the test and does nothing if we're within talos.
> +      // Provides an alternative tpRecordTime (with some stats display) if running in a browser.
> +      if(!report && document.head) {

Space before (

@@ +44,5 @@
> +    });
> +  }
> +
> +  function FP_wait(ms) {
> +    return function() {

I think this can just return the Promise directly, without returning the wrapper function, can't it?

@@ +89,5 @@
>    // For reference, rAF should fire on vsync, but Gecko currently doesn't use vsync.
>    // Instead, it uses 1000/layout.frame_rate
>    // (with 60 as default value when layout.frame_rate == -1).
> +  function P_syncScrollTest()
> +  { return new Promise(function(resolve) {

Nit: There are a variety of bracketing styles in this code. Let's not add another one. :)

function P_syncScrollTest() {
  return new Promise ...
}

is probably sufficient.

@@ +140,4 @@
>        Profiler.resume();
>      }
>      rAF(tick);
> +  });}

Same as above, re: brackets.

@@ +146,5 @@
> +  .then(FP_wait(260))
> +  .then(gotoTop)
> +  .then(P_rAF)
> +  .then(P_syncScrollTest)
> +  .then(function(result) { // function since 'report' might not be ready when invoking 'then'

I don't think I fully understand this... if we're invoking 'then', we're calling the function right away, and if report is ready here in this function, why wasn't it ready before?

Comment 27

[Avi Halachmi (:avih)]

(In reply to Mike Conley (:mconley) - (catching up on reviews) from comment #26)
> The comment at the top of this file asks us to keep this file in sync with
> scroll-test.js. Is that not necessary anymore? If so, can you please remove
> those comments from the top of each file? ...

You're reviewing part2. Part 1 already removed the duplication and adjusted the comment accordingly. I'm ignoring this comment unless you think something is still due here.


> > +  function P_setupReportFn() {
> Can you please add a docstring to this function? ...

Sure.


> > +      // Not part of the test and does nothing if we're within talos.
> > +      // Provides an alternative tpRecordTime (with some stats display) if running in a browser.
> So it sounds like this file can be used in a couple of different contexts.
> Can you document those at the top of this file?

It already is for some years ;) - https://dxr.mozilla.org/mozilla-central/source/testing/talos/talos/scripts/talos-debug.js

Ignoring this comment unless you think there's more to do here.


> > +      if(!report && document.head) {
> 
> Space before (

OK.


> > +  function FP_wait(ms) {
> > +    return function() {
> 
> I think this can just return the Promise directly, without returning the
> wrapper function, can't it?

It can't, or at least shouldn't according to the Promise A+ specification. Because the return value of this function is used as onFulfilled handler (e.g.  X.then(FP_wait(200)) ), the spec mandates that the handler is ignored unless it's a function.

The onFullfilled handler, however, once invoked after then's this has been resolved/rejected, may return a promise.

See https://github.com/promises-aplus/promises-spec

I'm ignoring this comment unless you still think I shouldn't.


> > +  function P_syncScrollTest()
> > +  { return new Promise(function(resolve) {
> 
> Nit: There are a variety of bracketing styles in this code. Let's not add
> another one. :)
> ...
> Same as above, re: brackets.

(In reply to Avi Halachmi (:avih) from comment #15)
> ... The indentations are awkward to make the diff smaller. I'll fix this after the review.

I think there is just one bracing style, with the first/last lines of this function the only exception, as explained above. But I'll re-check the whole file again.


> > +  .then(FP_wait(260))
> > +  .then(gotoTop)
> > +  .then(P_rAF)
> > +  .then(P_syncScrollTest)
> > +  .then(function(result) { // function since 'report' might not be ready when invoking 'then'
> 
> I don't think I fully understand this... if we're invoking 'then', we're
> calling the function right away, and if report is ready here in this
> function, why wasn't it ready before?

Calling 'then' only "registers" the onFullfilled/onRejected function handlers (this happens synchronously), but otherwise only invokes them once then's this has been fulfilled/rejected.

The line above where your quote was just cut is a Promise which assigns a value to 'report', possibly asynchronously:

>  P_setupReportFn()
>  .then(FP_wait(260))
> ...

Hence, when 'then' is being called (synchronously), the value of 'report' might not be final, and it is 'undefined' if P_setupReportFn did not complete synchronously.

However, if we use a function instead, then 'report' is resolved according to its lexical scope at the time of invocation, which happens to be whatever value P_setupReportFn assigned to it, synchronously or not.

Comment 28

[Avi Halachmi (:avih)]

Attachment: bug1189901.part2.v2.promises.patch

Address review comments.

After agreeing on IRC about comment 27, the remaining issues which this patch addresses are:
- the docstring
- missing space
- the fixed indentation (which makes the diff look much bigger than it actually is).

Comment 29

[Avi Halachmi (:avih)]

Attachment: bug1189901.part3.v2.TalosPowers-ParentExec.patch

Following comment 23, add generic-parent-execution functionality to talos-powers (instead of adding it in a new addon talosproxy).

Comment 30

[Avi Halachmi (:avih)]

Attachment: bug1189901.part4.v2.report-custom-name.patch

(In reply to Joel Maher (:jmaher) from comment #21)
> the rest of the patch looks just fine, I would like to make the testBaseName
> more robust.

Addressed.

> this seems as though we will have issues accessing [1] if it doesn't exist,
> or if we reuse this for neither tp5n or scroll

It won't have issues (will just return undefined) and that's exactly the point: if there's nothing after '/tp5n/' (because it doesn't exist) then return whatever comes after the ||.

Comment 31

[Avi Halachmi (:avih)]

Attachment: bug1189901.part5.v2.add-apz-test.patch

Carrying r+ but requesting feedback again because:

Other than rebase related changes (mostly using talos-powers instead of talosproxy to execute at the parent), it has the following differences from the previous version:

1. The new test names use .CSSOM and .CSSOM.checkerboard (instead .APZ and .APZ.checkerboard).

The reason is that we don't actually trigger APZ scrolling explicitly. We trigger CSS object-model based scroll, which Firefox knows to scroll using APZ where possible.

Also, since this test also runs without e10s, then the name is much more correct - in non-e10s we're getting the performance of CSSOM scroll (no APZ), and checkerboarding severity should be 0.


2. After some discussion with kats on IRC how to normalize the checkerboard severity reports, we agreed to normalize by the duration over which we measured. The new patch reflects this.

I'm also currently treating the severity report values as opaque, ignoring the fact that they're square root of some other values. This makes the severity/ms value which we report within a similar order of magnitude as the other results (when there actually is some checkerboarding).

Comment 32

[Avi Halachmi (:avih)]

Also, once we're good, I'll update and sign talos-powers-signed.xpi (with new version number).

Comment 33

[Avi Halachmi (:avih)]

(In reply to Avi Halachmi (:avih) from comment #29)
> Created attachment 8754355 [details] [diff] [review]
> bug1189901.part3.v2.TalosPowers-ParentExec.patch

I just noticed noticed that this patch has one error message with 'talosproxy`. I've already fixed it locally to refer to talos-powers instead.

Comment 34

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8754360 [details] [diff] [review]
bug1189901.part5.v2.add-apz-test.patch

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

SGTM!

Comment 35

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8754357 [details] [diff] [review]
bug1189901.part4.v2.report-custom-name.patch

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

thanks avih!

Comment 36

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8754355 [details] [diff] [review]
bug1189901.part3.v2.TalosPowers-ParentExec.patch

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

I think this is mostly fine - but I think you might be overbroadcasting responses to messages from content.

::: testing/talos/talos/pageloader/chrome/tscroll.js
@@ +189,5 @@
>    .then(P_syncScrollTest)
>    .then(function(result) { // function since 'report' might not be ready when invoking 'then'
> +
> +    // Sample usage of ParentExec with TalowPowers
> +    TalosPowersParent.exec("sampleParentService", 12345, function(rv) {

I think we can safely get rid of this before landing, along with the handler in the parent.

::: testing/talos/talos/talos-powers/chrome/talos-powers-content.js
@@ +52,5 @@
>    Cu.forceShrinkingGC();
>    sendSyncMessage("TalosPowersContent:ForceCCAndGC");
>  });
> +
> +/* Mediator for the generic ParentExec mechanism.

Nit: Open the blockquote with

/**
 * Mediator ...
 * ...
 */

@@ +62,5 @@
> + */
> +addEventListener("TalosPowers:ParentExec:QueryEvent", function (e) {
> +  if (content.location.protocol != "file:" &&
> +      content.location.hostname != "localhost" &&
> +      content.location.hostname != "127.0.0.1")

Nit - the rest of this file does braces on the same line as the closing parens, so this should be:

```
if (content.location.protocol != "file:" &&
    content.location.protocol != "localhost" &&
    content.location.hostname != "127.0.0.1") {
  throw new Error...
}
```

@@ +66,5 @@
> +      content.location.hostname != "127.0.0.1")
> +  {
> +    throw new Error("talosproxy may only be used with local content");
> +  }
> +  var uniqueMessageId = "TalosPowers:ParentExec:"

let, not var

@@ +77,5 @@
> +
> +    removeMessageListener("TalosPowers:ParentExec:ReplyMsg", done);
> +
> +    // reply to content via an event
> +    var contentevent = Cu.cloneInto({

let, not var.

Also, contentEvent.

::: testing/talos/talos/talos-powers/components/TalosPowersService.js
@@ +253,5 @@
> +  },
> +
> +  RecieveParentExecCommand(msg) {
> +    function sendResult(result) {
> +      Services.mm.broadcastAsyncMessage("TalosPowers:ParentExec:ReplyMsg", {

I'm curious - why are we broadcasting this reply to _all_ message listeners, instead of to the target of what sent us the message to begin with? Surely only the sender is interested in a reply?

You can get the sender via msg.target. That'll be the <xul:browser>, so you can do:

let mm = msg.target.messageManager;
mm.sendAsyncMessage(...

And be sure that you're sending a response to the message to the right thing.

@@ +259,5 @@
> +        result: result
> +      });
> +    }
> +
> +    var command = msg.data.command;

let, not var

::: testing/talos/talos/talos-powers/content/TalosPowersContent.js
@@ +34,5 @@
> +    replyId: 1,
> +
> +    // dispatch an event to the framescript and register the result/callback event
> +    exec: function(commandName, arg, callback, opt_custom_window) {
> +      var win = opt_custom_window || window;

let, not var here and 39

Comment 37

[Avi Halachmi (:avih)]

(In reply to Mike Conley (:mconley) - (needinfo me!) from comment #36)
> I think we can safely get rid of this before landing, along with the handler
> in the parent.

The sample usage is already gone at part4. I thought the sample service is small enough to keep for posterity but I'm fine with removing it too at part 4.

The main thing I wanted is that the commit which adds this system includes a fully working example - service and consumer sides.


> Nit: Open..., Nit this, Nit that, camelCase, s/var/let/

Aye aye sir!


> Nit - the rest of this file does braces on the same line as the closing
> parens...

Seemingly yes, however, this was intentional, and it's something I picked from $somewhere style guidelines.

That style-guide says on this subject that if the condition is one line, the opening brace follows at the same line, but if the condition spans multiple lines, then it's more readable if the opening brace is on a line of its own.

Consider the following (I'm using 4 indents to make the point clearer):

> if (content.location.protocol != "file:" &&
>     content.location.protocol != "localhost" &&
>     content.location.hostname != "127.0.0.1") {
>     throw new Error("I'm an error");
> }

And

> if (content.location.protocol != "file:" &&
>     content.location.protocol != "localhost" &&
>     content.location.hostname != "127.0.0.1")
> {
>     throw new Error("I'm an error");
> }

But I'm fine also with your suggestion. Will do.


> > +      Services.mm.broadcastAsyncMessage("TalosPowers:ParentExec:ReplyMsg", {
> 
> I'm curious - why are we broadcasting this reply to _all_ message listeners ...
> 
> You can get the sender via msg.target. That'll be the <xul:browser>, so you
> can do: ...

TBH, this part of the code was copied from an earlier version of this system which I wrote for making TART e10s compatible (bug 1078250). Apparently it didn't seem offensive back then when it landed, and I didn't give it a second thought now.

But what you say makes sense and I agree in general.

However, assuming we take your approach, does it mean we now need to check if msg.target is still "alive"? I don't know if it's captured via a closure, and it's still possible that the tab has since closed (especially once we deploy process per tab), and this could make this system more sensitive.

Broadcast is blissfully ignorant of such potential issues, and assuming these commands are not used in a performance-critical context (which I think is the case anyway) then it does not pose an issue and does make the system more robust.

But I'll experiment with this a bit.

Thanks for the review.

Comment 38

[Avi Halachmi (:avih)]

Attachment: bug1189901.part3.v3.TalosPowers-ParentExec.patch

Addressed all review comments, and I will remove the sample service and consumer before I land, and will leave the sample service as a comment only (the comment already has/had sample consumer).

Comment 39

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8755030 [details] [diff] [review]
bug1189901.part3.v3.TalosPowers-ParentExec.patch

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

Assuming this lands without the sample-y bits as we discussed, this lgtm. Thanks avih!

Comment 40

[Avi Halachmi (:avih)]

Thanks Mike! will do.

So, I did a try push yesterday - https://treeherder.mozilla.org/#/jobs?repo=try&revision=8968f92aac93 (ignore the sea of orange - flake8 does not like a perfectly valid syntax of extra space before array closing brace - so it makes all tests appear as failures).

I did dig through the logs though, and for the most part the numbers of the CSSOM scroll composition intervals look reasonable, but the checkerboard reports are mostly 0.

When I tested it locally (while developing the test), some pages were 0 - because they are short or the scroll was up too slow to cause any CB, but some pages, especially the amazon page of tp5 - consistently had non-zero CB reports.

With the try push, I initially suspected that something is pathologically broken, but then I did spot one or two which are non-zero.

After discussing this with kats, we suspect that the reason for the zero reports is because the report is requested at the middle of a checkerboard event (the test intentionally tries to measure while the page still scrolls) - so this evens would be excluded from the report.

I filed bug 1275314 to also support reports which do not ignore a currently ongoing event. It can be a new API, and the test doesn't care what happens with this specific event later.

Also, we might need to come up with "harder" pages which have higher chance to have CB.

Comment 41

[Avi Halachmi (:avih)]

Attachment: 0002-Bug-1189901-part-1-remove-code-redundancy-between-ts.patch

Carrying r+.

As decided on bug 1275314 comment 22, we'll go ahead with the APZ iterations test but without the checkerboard test.

I'm attaching all the parts again rebased and fixed where requested.

Comment 42

[Avi Halachmi (:avih)]

Attachment: 0003-Bug-1189901-part-2-scroll-tests-serialize-asyncs-wit.patch

Carrying r+

Comment 43

[Avi Halachmi (:avih)]

Attachment: 0004-Bug-1189901-part-3-talos-powers-add-generic-ParentEx.patch

Carrying r+

Comment 44

[Avi Halachmi (:avih)]

Attachment: 0005-Bug-1189901-part-4-scroll-report-custom-test-name.-r.patch

Carrying r+

Comment 45

[Avi Halachmi (:avih)]

Attachment: 0006-Bug-1189901-part-5-add-CSSOM-scroll-test-uses-APZ-wh.patch

Carrying r+

This changes the previous part 5 by removing the code related to the checkerboard test.

Comment 46

[Avi Halachmi (:avih)]

Attachment: 0007-Bug-1189901-part-6-add-signed-pageloader-addon.-r-jm.patch

This adds the signed pageloader addon xpi.

Doesn't really need a review but adding r? for the sake of landing this.

Comment 47

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8795288 [details] [diff] [review]
0007-Bug-1189901-part-6-add-signed-pageloader-addon.-r-jm.patch

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

thanks for signing this :)

Comment 48

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/081c6b4986a9
part 1: remove code redundancy between tscrollx and tp5o_scroll. r=jmaher
https://hg.mozilla.org/integration/mozilla-inbound/rev/866c0244a7c1
part 2: scroll-tests: serialize asyncs with Promises. r=mconley
https://hg.mozilla.org/integration/mozilla-inbound/rev/610b70634239
part 3: talos-powers: add generic ParentExec. r=mconley
https://hg.mozilla.org/integration/mozilla-inbound/rev/926a610af8a7
part 4: scroll: report custom test name. r=jmaher
https://hg.mozilla.org/integration/mozilla-inbound/rev/647db9f59d9e
part 5: add CSSOM scroll test - uses APZ when available. r=kats
https://hg.mozilla.org/integration/mozilla-inbound/rev/14660f4f2b23
part 6: add signed pageloader addon. r=jmaher

Comment 49

[Avi Halachmi (:avih)]

Note that this should change the results of tp5o_scroll and tscrollx - in both cases we're expecting slightly lower numbers - in e10s probably a bit lower than non-e10s, but still both should probably be a bit lower.

This is because an additional new thing is now being measured (CSSOM scroll, which in e10s utilizes APZ), and its results are typically a bit better than plain synchronous scroll, so on average the numbers should get a bit lower.

Comment 50

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

I had to back this out for making a few Windows talos jobs permafail: https://treeherder.mozilla.org/logviewer.html#?job_id=36542577&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/9dfac0d146bc

Comment 51

[Avi Halachmi (:avih)]

(In reply to Wes Kocher (:KWierso) from comment #50)
> I had to back this out for making a few Windows talos jobs permafail:
> https://treeherder.mozilla.org/logviewer.html#?job_id=36542577&repo=mozilla-
> inbound
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/9dfac0d146bc

Weird - earlier try pushes have been green. Anyway, thanks, investigating.

Comment 52

[Avi Halachmi (:avih)]

(In reply to Avi Halachmi (:avih) from comment #51)
> Weird - earlier try pushes have been green. Anyway, thanks, investigating.

OK, I think figured it out.

I did a try push today with identical patch series as the patches which were backed out, over a recent/current m-c, and it's all great, including g1 and s talos suits on all platforms - https://treeherder.mozilla.org/#/jobs?repo=try&revision=4c646994cd46

The only difference between this and the series which got backed out is the added pageloader signed addon, but there's no issue with the pageloader.

What's missing is the signed talos-powers addon which I forgot I need to update as well.

Without the new functionality in talos powers (which these patches add and use) the new scroll tests would hang, but they don't hang on try pushes since it doesn't use the packaged/signed xpi (i.e. it creates an xpi on the fly from the source files - which are updated).

I'll re-push the series and the additional updated and signed talos-powers addon.

Comment 53

[Avi Halachmi (:avih)]

Attachment: 0007-Bug-1189901-part-7-add-signed-talos-powers-addon.-r-.patch

Just signing the talos-powers code from previous patches.

Comment 54

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4cb1b2ae58c0
part 1: remove code redundancy between tscrollx and tp5o_scroll. r=jmaher
https://hg.mozilla.org/integration/mozilla-inbound/rev/f8719709d98a
part 2: scroll-tests: serialize asyncs with Promises. r=mconley
https://hg.mozilla.org/integration/mozilla-inbound/rev/a72a4afe756e
part 3: talos-powers: add generic ParentExec. r=mconley
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b747bcd819e
part 4: scroll: report custom test name. r=jmaher
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a294b6d4847
part 5: add CSSOM scroll test - uses APZ when available. r=kats
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1c1f217e194
part 6: add signed pageloader addon. r=jmaher
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b788911d3e2
part 7: add signed talos-powers addon. r=jmaher

Comment 55

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4cb1b2ae58c0
https://hg.mozilla.org/mozilla-central/rev/f8719709d98a
https://hg.mozilla.org/mozilla-central/rev/a72a4afe756e
https://hg.mozilla.org/mozilla-central/rev/2b747bcd819e
https://hg.mozilla.org/mozilla-central/rev/1a294b6d4847
https://hg.mozilla.org/mozilla-central/rev/f1c1f217e194
https://hg.mozilla.org/mozilla-central/rev/2b788911d3e2

Comment 56

[Alison Shiue]

Talos has detected a Firefox performance regression from push 2b788911d3e2e946a8f17a79f6cfe28821ffa933.

https://treeherder.mozilla.org/perf.html#/alerts?id=3655

Regressions:

  7%  tscrollx summary windows7-32 opt e10s     2.68 -> 2.85
  6%  tscrollx summary windows8-64 opt e10s     2.26 -> 2.39
  4%  tscrollx summary windowsxp opt e10s       1.65 -> 1.72

Improvements:

 17%  tp5o_scroll summary windowsxp opt e10s     2.39 -> 1.97
 11%  tp5o_scroll summary windows8-64 opt        3.79 -> 3.36
 10%  tp5o_scroll summary windows7-32 opt        4.16 -> 3.73
  7%  tp5o_scroll summary windowsxp opt          4.47 -> 4.17

Hi Avi, are these tscrollx regressions expected?

Comment 57

[Avi Halachmi (:avih)]

(In reply to Alison Shiue from comment #56)
> Talos has detected a Firefox performance regression ...

It's actually the other way around here - Firefox has detected a Talos change ;)

These patches change the tp5o_scroll and tscrollx talos tests without changing anything in Firefox itself, so Firefox definitely hasn't regressed, although the test numbers have changed because the tests changed.


> Hi Avi, are these tscrollx regressions expected?

These tests have changed, different results are expected. I've also looked at the subtests comparisons and the new numbers for all subtests, and they look reasonable for these changes.

We should take the new numbers as the new baseline for these tests.

Comment 58

[Joel Maher ( :jmaher ) (UTC -8)]

Avi, what information did you need for Alison?

Comment 59

[Avi Halachmi (:avih)]

(In reply to Joel Maher ( :jmaher) from comment #58)
> Avi, what information did you need for Alison?

Nothing, but this way there's no need to poll this bug, as it will send a notification that I've replied.

Comment 60

[Alison Shiue]

Got it, thanks for the reply!