Add Async Runner for News Site Next Testing

[flashdesignory]

A potential solution to: #441

This pr adds an Async Runner, which waits for each test.run() to complete.

Tests for comparison:

1. NewsSite-Next - the current implementation with the current runner
2. NewsSite-Async - async runner - but no changes to the tests
   3. NewsSite-Delay - async runner with a 500ms delay, before the test resolves

crossbench scores

async:
browser                Safari                    Firefox                Google Chrome
NewsSite-Next          58.20 ± 1.6%              74.8 ± 2.2%            73.8 ± 1.9%
NewsSite-Next-Delayed  1535.24 ± 0.050%          1563.6 ± 0.34%         1534.93 ± 0.050%
NewsSite-Next-Async    70.40 ± 1.2%              79.6 ± 1.9%            75.28 ± 1.1%
Score                  5.430 ± 0.73%             4.761 ± 1.1%           4.901 ± 0.71%

[bgrins]

The semantics here are tricky, but great to be opening this discussion. @smaug---- @rniwa the current behavior is

requestAnimationFrame(() => this._syncCallback());
requestAnimationFrame(() => {
  setTimeout(() => {
    this._asyncCallback();
    setTimeout(async () => {
      await this._reportCallback();
      resolve();
    }, 0);
  }, 0);
});

Since we'd now need to await for _syncCallback in a world with async steps, this new behavior wraps the second rAF into the first rAF's callback (after awaiting for the sync step to finish). Is this how you envision measuring async steps?

[bgrins]

I also wonder if it would be feasible to make an "in place" change where, rather than having separate AsyncRAFTestInvoker & AsyncTimerTestInvoker & AsyncSuiteRunner classes, we
(a) update the existing ones to await in the appropriate places
(b) change the existing _runTestAndRecordResults to await test.run(this._page);
(c) allow tests to opt in to the behavior by simply passing async functions into the BenchmarkTestStep rather than setting a type and switching which classes are used.

Obviously this would rely on us being happy with the core async measurement loop, and would change the overall timing & semantics by awaiting on non-promises. But it would also be quite a bit simpler and may allow use cases for v4 where tests have one step that's async and others that aren't.

[smaug----]

Wouldn't this work rather badly in those tests where the sync part is really fast. There would be idle time between the rAF calls, and thus the measured time would also include idle time.

[flashdesignory]

@smaug---- any idea how we could prevent this, while still waiting for the syncCallback ?

[smaug----]

It would get messy, but we could perhaps have another rAF callback right before _syncCallback call.
That new one should fire at the same tick as the one which _syncCallback() uses (assuming I guess correctly how it schedules new work.) The new raf callback would start async time.
In addition to that we'd need a setTimeout(,0) to stop the sync time - that would be used in case there is time to run setTimeout(,0) between the relevant ticks. But the sync time might also just stop when the next raf starts - the order of rAFs and setTimeout can be anything.
(Of course currently there isn't such thing as "stop sync time", there is just starting async time)

[flashdesignory]

@smaug---- honestly, I don't fully follow the above, but here are some additional thoughts.

The following would keep the two sequential rafs, but would enable workloads that might take longer to resolve the promise of the _syncCallback. Feels 'old school', but works in my initial testing in Chrome:

_scheduleCallbacks(resolve) {
        const checkedIn = {
            __proto__: null,
            sync: false,
            async: false,
        };

        const checkin = async (type) => {
            checkedIn[type] = true;

            if (checkedIn.sync && checkedIn.async){
                setTimeout(() => {
                    this._asyncCallback();
                    setTimeout(async () => {
                        await this._reportCallback();
                        resolve();
                    }, 0);
                }, 0);
            }
        };

        requestAnimationFrame(async () => {
            await this._syncCallback();
            checkin("sync");
        });

        requestAnimationFrame(() => {
            checkin("async");
        });
    }

A super small sample run:

Yet another though: do we even need two rafs, or could we do something simpler.
If we're awaiting a Promise, which is a microtask with higher priority, can we follow that with a setTimeout directly?

requestAnimationFrame(async () => {
            await this._syncCallback();
            setTimeout(() => {
                this._asyncCallback();
                setTimeout(async () => {
                    await this._reportCallback();
                    resolve();
                }, 0);
            }, 0);
        });

[smaug----]

@smaug---- honestly, I don't fully follow the above,

Not surprised, it is confusing :) Basically what I was thinking is to try to detect when the sync time ends, explicitly.
And then not count the time between sync end and and async start as measured time.

setTimeout after _syncCallback would bring back the same issues sp2 has. There some random animation ticks may or may not run during measured time, because it is relying on setTimeout and not rAF callbacks.

[smaug----]

Assuming react uses postMessage similarly to setTimeout(0),
would it help if the runner was something like

requestAnimationFrame(() => this._syncCallback());
requestAnimationFrame(() => {
  var gotTimer = false;
  var gotMessage = false;

  setTimeout(()=> { gotTimer = true;  tryTriggerAsyncCallback() });

  var mc = new MessageChannel();
  mc.port1.onmessage =  () => {  gotMessage = true; tryTriggerAsyncCallback(); }
  mc.port2.postMessage("sp3");

  function tryTriggerAsyncCallback() {
   if (!gotTimer || !gotMessage) {
    return;
   }
    this._asyncCallback();
    setTimeout(async () => {
      await this._reportCallback();
      resolve();
    }, 0);
  }, 0);
});

So basically require both postMessage and setTimeout to be processed.

Looks like there is a message channel and its port is used for communication
https://share.firefox.dev/48D7xwJ

[flashdesignory]

I assume the MessageChannel is from the React framework? - I'd need to dig in a little more though...

It probably depends, if we're trying to fix the React workloads, or if we're trying to allow async test steps. For the later, we'd need to await this._syncCallback(); ideally, so that the test steps could control or signal when they are done?

[smaug----]

The MessageChannel in my example is from HTML spec.
I added locally some profiler markers to check whether React is using window.postMessage or MessaChannel/MessagePort.postMessage, and it was the latter.

[flashdesignory]

Summarizing an offline conversation I had with @smaug---- :

• The idea @smaug---- proposed with the MessageChannel might be a great solve for the issue Add Async Runner for News Site Next Testing #442, but should be handled in a separate PR, since that solution should get implemented directly in the RAFTestInvoker class
• The AsyncRunner pr (this pr) should focus only on enabling async test steps
• We are ok, with explicitly marking a test async and in that way keeping a separate AsyncRAFTestInvoker class that handles these cases. We should be explicit about sync, async or remote test steps.
• The current proposed AsyncRAFTestInvoker implementation should be OK in its current implementation with the two nested RAFs.
• Trying to combine sync and async test steps in one RAFTestInvoker class (as discussed briefly in comments above) gets us in a tricky situation where one solution might be overly complicated.

[smaug----]

And hopefully use of MessageChannel really helps. The idea isn't tested yet ;)

[flashdesignory]

since i'm extending and overriding the runTest function, I had to create getters for the private variables.
just more boilerplate..

[bgrins]

If there are now external consumers for these, should we just make them public? cc @rniwa

[flashdesignory]

I'm only using getters at the moment, so more boilerplate, but maybe more "secure" than just making them public? 🤷

[camillobruni]

+1 to making the public and lower overhead.

[flashdesignory]

no need to create a new SuiteRunner, since only the TestRunner is different and that's dynamically selected.

[bgrins]

Do we plan to use both the timer and RAF invoker for async? I had a sense that we kept the sync timer invoker around for backwards compat or non-default testing environments.

I'm assuming the overall properties are quire different once we are async (potentially reducing utility of timer), and it's complex enough to reason about one implementation, so should we consider just having one and exposing it on both the default and non-default configuration?

I'm open to having both but would like to find ways to reduce the amount of work if possible :). @camillobruni @smaug---- @rniwa thoughts?

[camillobruni]

I'd be fine to drop the setTimeout approach altogether since we haven't really discovered additional issues with it?

[camillobruni]

let's not use private properties, they add needless overhead in the runner. The runner code should aim at maximum performance and minimal startup costs.

Last I tested this is measurable in Firefox and Chrome.

[rniwa]

I don't think we should restrict ourselves from using new ECMAScript features. With that argument, we should be going back to writing ES5 code without any classes.

[camillobruni]

If there is performance penalty attached to it for the test runner, then yes, we should restrict ourselves, we should aim at the lowest possible overhead there. Especially if it uses code that is not popular out in the wild.

And I would even agree that we should transpile and minify to ES5 the runner code for a big part for performance reasons!

It's a different story for workloads though, a good mix of old and new features is very welcome.
Note: we did not include the latest framework versions based on this groups feedback... so there is precedence for going equally the other way.

[camillobruni]

+1 to making the public and lower overhead.