Refactor prepareToMeasureValuesAsync to startMeasureValuesAsyn which run test through callback

Refactor prepareToMeasureValuesAsync to startMeasureValuesAsyn which run test through callback

Motivation:
In order to support tracing metrics for prepareToMeasureValuesAsync method,
we need to be able to trigger tracing between prepareToMeasureValuesAsync &
actual test run. This CL rename prepareToMeasureValuesAsync method to 
startMeasureValuesAsyn & refactor it  so that the
actual test code is specified as a callback, which allows the framework
to interject the startTrace() call before running test in the future.

BUG=701059
Review-Url: https://codereview.chromium.org/2858783003
Cr-Commit-Position: refs/heads/master@{#469233}
Committed: https://chromium.googlesource.com/chromium/src/+/8dc2204a41db0b7b0f66c529c53d4c4a5d9d7fb5

[nednguyen, 2017-05-03 16:25:50]

Patchset #1 (id:1) has been deleted

[nednguyen, 2017-05-03 16:41:33]

Description was changed from

==========
Refactor prepareToMeasureValuesAsync

BUG=
==========

to

==========
Refactor prepareToMeasureValuesAsync to run test through callback

Motivation:
In order to support tracing metrics for prepareToMeasureValuesAsync method,
we need to be able to trigger tracing between prepareToMeasureValuesAsync &
actual test run. This CL refactor prepareToMeasureValuesAsync so that the
actual test code is specified as a callback, which allows the framework
to interject the startTrace() call before running test in the future.

BUG=701059
==========

[nednguyen, 2017-05-03 16:41:33]

nednguyen@google.com changed reviewers:
+ wangxianzhu@chromium.org

[nednguyen, 2017-05-03 16:42:07]

This is not the full CL, if you think the high level direction here is OK, I
will refactor all the tests that use prepareToMeasureValuesAsync to use the
callback.

[Xianzhu, 2017-05-03 17:14:25]

https://codereview.chromium.org/2858783003/diff/40001/third_party/WebKit/Perf...
File third_party/WebKit/PerformanceTests/resources/runner.js (right):

https://codereview.chromium.org/2858783003/diff/40001/third_party/WebKit/Perf...
third_party/WebKit/PerformanceTests/resources/runner.js:275: start(test,
undefined, test.run);
How about:
   start(test, function(task) { task(); }, function() { test.run(); });
?

I think this has the following benefits:
1. avoids change to start();
2. allows such tests to collect trace measurement.
3. allows test.run to access the test object through 'this';

[nednguyen, 2017-05-03 18:21:52]

https://codereview.chromium.org/2858783003/diff/40001/third_party/WebKit/Perf...
File third_party/WebKit/PerformanceTests/resources/runner.js (right):

https://codereview.chromium.org/2858783003/diff/40001/third_party/WebKit/Perf...
third_party/WebKit/PerformanceTests/resources/runner.js:275: start(test,
undefined, test.run);
On 2017/05/03 17:14:25, Xianzhu wrote:
> How about:
>    start(test, function(task) { task(); }, function() { test.run(); });
> ?
> 
> I think this has the following benefits:
> 1. avoids change to start();
> 2. allows such tests to collect trace measurement.
> 3. allows test.run to access the test object through 'this';

I tried this first, but doing so lead to invalid test run. :-(

In particular, PerformanceTests/Parser/html-parser-threaded.html will output:

Ignoring warm-up run (undefined ms)
undefined ms
undefined ms
undefined ms
undefined ms
undefined ms
undefined ms
undefined ms
undefined ms
undefined ms
undefined ms
undefined ms
undefined ms
...

[nednguyen, 2017-05-03 18:31:21]

On 2017/05/03 18:21:52, nednguyen wrote:
>
https://codereview.chromium.org/2858783003/diff/40001/third_party/WebKit/Perf...
> File third_party/WebKit/PerformanceTests/resources/runner.js (right):
> 
>
https://codereview.chromium.org/2858783003/diff/40001/third_party/WebKit/Perf...
> third_party/WebKit/PerformanceTests/resources/runner.js:275: start(test,
> undefined, test.run);
> On 2017/05/03 17:14:25, Xianzhu wrote:
> > How about:
> >    start(test, function(task) { task(); }, function() { test.run(); });
> > ?
> > 
> > I think this has the following benefits:
> > 1. avoids change to start();
> > 2. allows such tests to collect trace measurement.
> > 3. allows test.run to access the test object through 'this';
> 
> I tried this first, but doing so lead to invalid test run. :-(
> 
> In particular, PerformanceTests/Parser/html-parser-threaded.html will output:
> 
> Ignoring warm-up run (undefined ms)
> undefined ms
> undefined ms
> undefined ms
> undefined ms
> undefined ms
> undefined ms
> undefined ms
> undefined ms
> undefined ms
> undefined ms
> undefined ms
> undefined ms
> ...

I think the reason here is because the test using prepareToMeasureValuesAsync
already have their own "scheduler". In this case, iframe.onload is such one. Not
sure how to refactor this :-?

[Xianzhu, 2017-05-03 19:16:00]

https://codereview.chromium.org/2858783003/diff/40001/third_party/WebKit/Perf...
File third_party/WebKit/PerformanceTests/resources/runner.js (right):

https://codereview.chromium.org/2858783003/diff/40001/third_party/WebKit/Perf...
third_party/WebKit/PerformanceTests/resources/runner.js:148: if (!test) {
Add "|| !runner" because with this CL, we should always have a runner.

https://codereview.chromium.org/2858783003/diff/40001/third_party/WebKit/Perf...
third_party/WebKit/PerformanceTests/resources/runner.js:185: if
(test.tracingCategories && !(window.testRuner &&
There was a typo "testRuner" here.

How about changing old line 173-188 to:

        if (!test.tracingCategories) {
            scheduleNextRun(scheduler, runner);
            return;
        }

        if (window.testRunner && window.testRunner.supportTracing) {
            testRunner.startTracing(test.tracingCategories, function() {
                scheduleNextRun(scheduler, runner);
            });
            return;
        }

        PerfTestRunner.log("Tracing based metrics are specified but " +
            "tracing is not supported on this platform. To get those " +
            "metrics from this test, you can run the test using " +
            "tools/perf/run_benchmarks script.");

https://codereview.chromium.org/2858783003/diff/40001/third_party/WebKit/Perf...
third_party/WebKit/PerformanceTests/resources/runner.js:275: start(test,
undefined, test.run);
On 2017/05/03 18:21:52, nednguyen wrote:
> On 2017/05/03 17:14:25, Xianzhu wrote:
> > How about:
> >    start(test, function(task) { task(); }, function() { test.run(); });
> > ?
> > 
> > I think this has the following benefits:
> > 1. avoids change to start();
> > 2. allows such tests to collect trace measurement.
> > 3. allows test.run to access the test object through 'this';
> 
> I tried this first, but doing so lead to invalid test run. :-(
> 
> In particular, PerformanceTests/Parser/html-parser-threaded.html will output:
> 
> Ignoring warm-up run (undefined ms)
> undefined ms
> undefined ms
> undefined ms
> undefined ms
> undefined ms
> undefined ms
> undefined ms
> undefined ms
> undefined ms
> undefined ms
> undefined ms
> undefined ms
> ...

I see the problem. scheduleNextRun() doesn't support async measurement.

We should have
  start(test, undefined, function() { test.run() });
here, and add
  if (!scheduler) {
    // This is an async measurement test which has its own scheduler.
    runner();
    return;
  }
in scheduleNextRun().

[nednguyen, 2017-05-03 19:18:51]

https://codereview.chromium.org/2858783003/diff/40001/third_party/WebKit/Perf...
File third_party/WebKit/PerformanceTests/resources/runner.js (right):

https://codereview.chromium.org/2858783003/diff/40001/third_party/WebKit/Perf...
third_party/WebKit/PerformanceTests/resources/runner.js:148: if (!test) {
On 2017/05/03 19:16:00, Xianzhu wrote:
> Add "|| !runner" because with this CL, we should always have a runner.

Right.

https://codereview.chromium.org/2858783003/diff/40001/third_party/WebKit/Perf...
third_party/WebKit/PerformanceTests/resources/runner.js:185: if
(test.tracingCategories && !(window.testRuner &&
On 2017/05/03 19:16:00, Xianzhu wrote:
> There was a typo "testRuner" here.
> 
> How about changing old line 173-188 to:
> 
>         if (!test.tracingCategories) {
>             scheduleNextRun(scheduler, runner);
>             return;
>         }
> 
>         if (window.testRunner && window.testRunner.supportTracing) {
>             testRunner.startTracing(test.tracingCategories, function() {
>                 scheduleNextRun(scheduler, runner);
>             });
>             return;
>         }
> 
>         PerfTestRunner.log("Tracing based metrics are specified but " +
>             "tracing is not supported on this platform. To get those " +
>             "metrics from this test, you can run the test using " +
>             "tools/perf/run_benchmarks script.");

Thanks, I will make a separate CL for this.

https://codereview.chromium.org/2858783003/diff/40001/third_party/WebKit/Perf...
third_party/WebKit/PerformanceTests/resources/runner.js:275: start(test,
undefined, test.run);
On 2017/05/03 19:16:00, Xianzhu wrote:
> On 2017/05/03 18:21:52, nednguyen wrote:
> > On 2017/05/03 17:14:25, Xianzhu wrote:
> > > How about:
> > >    start(test, function(task) { task(); }, function() { test.run(); });
> > > ?
> > > 
> > > I think this has the following benefits:
> > > 1. avoids change to start();
> > > 2. allows such tests to collect trace measurement.
> > > 3. allows test.run to access the test object through 'this';
> > 
> > I tried this first, but doing so lead to invalid test run. :-(
> > 
> > In particular, PerformanceTests/Parser/html-parser-threaded.html will
output:
> > 
> > Ignoring warm-up run (undefined ms)
> > undefined ms
> > undefined ms
> > undefined ms
> > undefined ms
> > undefined ms
> > undefined ms
> > undefined ms
> > undefined ms
> > undefined ms
> > undefined ms
> > undefined ms
> > undefined ms
> > ...
> 
> I see the problem. scheduleNextRun() doesn't support async measurement.
> 
> We should have
>   start(test, undefined, function() { test.run() });
> here, and add
>   if (!scheduler) {
>     // This is an async measurement test which has its own scheduler.
>     runner();
>     return;
>   }
> in scheduleNextRun().

Acknowledged.

[nednguyen, 2017-05-03 21:30:00]

I realize that just by setting test.run = function() {},  we won't change the
current test run & still achieve what we want.

Though I still refactor wherever applied. PTAL.

https://codereview.chromium.org/2858783003/diff/40001/third_party/WebKit/Perf...
File third_party/WebKit/PerformanceTests/resources/runner.js (right):

https://codereview.chromium.org/2858783003/diff/40001/third_party/WebKit/Perf...
third_party/WebKit/PerformanceTests/resources/runner.js:148: if (!test) {
On 2017/05/03 19:16:00, Xianzhu wrote:
> Add "|| !runner" because with this CL, we should always have a runner.

Done.

[Xianzhu, 2017-05-03 22:06:26]

lgtm

https://codereview.chromium.org/2858783003/diff/70001/third_party/WebKit/Perf...
File third_party/WebKit/PerformanceTests/Bindings/post-message.html (right):

https://codereview.chromium.org/2858783003/diff/70001/third_party/WebKit/Perf...
third_party/WebKit/PerformanceTests/Bindings/post-message.html:35: },
Nit: a simpler way is
  run: runTest,

https://codereview.chromium.org/2858783003/diff/70001/third_party/WebKit/Perf...
File
third_party/WebKit/PerformanceTests/Bindings/resources/structured-clone-perf-test.js
(right):

https://codereview.chromium.org/2858783003/diff/70001/third_party/WebKit/Perf...
third_party/WebKit/PerformanceTests/Bindings/resources/structured-clone-perf-test.js:31:
run() { },
Change this line to
  run: pingPongUtilDone,
and remove line 44.

https://codereview.chromium.org/2858783003/diff/70001/third_party/WebKit/Perf...
File third_party/WebKit/PerformanceTests/resources/runner.js (right):

https://codereview.chromium.org/2858783003/diff/70001/third_party/WebKit/Perf...
third_party/WebKit/PerformanceTests/resources/runner.js:195: try {
Nit: Use indentation same as the reset of this file.

https://codereview.chromium.org/2858783003/diff/70001/third_party/WebKit/Perf...
third_party/WebKit/PerformanceTests/resources/runner.js:201: }
Nit: insert a blank line after this line.

https://codereview.chromium.org/2858783003/diff/70001/third_party/WebKit/Perf...
third_party/WebKit/PerformanceTests/resources/runner.js:278:
PerfTestRunner.prepareToMeasureValuesAsync = function (test) {
As test.run is called inside of this function, it no longer just "prepare".

"startMeasureValuesAsync"?

[nednguyen, 2017-05-04 00:10:33]

https://codereview.chromium.org/2858783003/diff/70001/third_party/WebKit/Perf...
File third_party/WebKit/PerformanceTests/Bindings/post-message.html (right):

https://codereview.chromium.org/2858783003/diff/70001/third_party/WebKit/Perf...
third_party/WebKit/PerformanceTests/Bindings/post-message.html:35: },
On 2017/05/03 22:06:25, Xianzhu wrote:
> Nit: a simpler way is
>   run: runTest,

Done.

https://codereview.chromium.org/2858783003/diff/70001/third_party/WebKit/Perf...
File
third_party/WebKit/PerformanceTests/Bindings/resources/structured-clone-perf-test.js
(right):

https://codereview.chromium.org/2858783003/diff/70001/third_party/WebKit/Perf...
third_party/WebKit/PerformanceTests/Bindings/resources/structured-clone-perf-test.js:31:
run() { },
On 2017/05/03 22:06:26, Xianzhu wrote:
> Change this line to
>   run: pingPongUtilDone,
> and remove line 44.

Done. Nice catch!

https://codereview.chromium.org/2858783003/diff/70001/third_party/WebKit/Perf...
File third_party/WebKit/PerformanceTests/resources/runner.js (right):

https://codereview.chromium.org/2858783003/diff/70001/third_party/WebKit/Perf...
third_party/WebKit/PerformanceTests/resources/runner.js:195: try {
On 2017/05/03 22:06:26, Xianzhu wrote:
> Nit: Use indentation same as the reset of this file.

Done.

https://codereview.chromium.org/2858783003/diff/70001/third_party/WebKit/Perf...
third_party/WebKit/PerformanceTests/resources/runner.js:201: }
On 2017/05/03 22:06:26, Xianzhu wrote:
> Nit: insert a blank line after this line.

Done.

[nednguyen, 2017-05-04 00:10:50]

https://codereview.chromium.org/2858783003/diff/70001/third_party/WebKit/Perf...
File third_party/WebKit/PerformanceTests/resources/runner.js (right):

https://codereview.chromium.org/2858783003/diff/70001/third_party/WebKit/Perf...
third_party/WebKit/PerformanceTests/resources/runner.js:278:
PerfTestRunner.prepareToMeasureValuesAsync = function (test) {
On 2017/05/03 22:06:26, Xianzhu wrote:
> As test.run is called inside of this function, it no longer just "prepare".
> 
> "startMeasureValuesAsync"?

Done.

[nednguyen, 2017-05-04 00:11:21]

Description was changed from

==========
Refactor prepareToMeasureValuesAsync to run test through callback

Motivation:
In order to support tracing metrics for prepareToMeasureValuesAsync method,
we need to be able to trigger tracing between prepareToMeasureValuesAsync &
actual test run. This CL refactor prepareToMeasureValuesAsync so that the
actual test code is specified as a callback, which allows the framework
to interject the startTrace() call before running test in the future.

BUG=701059
==========

to

==========
Refactor prepareToMeasureValuesAsync to startMeasureValuesAsyn which run test
through callback

Motivation:
In order to support tracing metrics for prepareToMeasureValuesAsync method,
we need to be able to trigger tracing between prepareToMeasureValuesAsync &
actual test run. This CL refactor prepareToMeasureValuesAsync so that the
actual test code is specified as a callback, which allows the framework
to interject the startTrace() call before running test in the future.

BUG=701059
==========

[nednguyen, 2017-05-04 00:11:49]

Description was changed from

==========
Refactor prepareToMeasureValuesAsync to startMeasureValuesAsyn which run test
through callback

Motivation:
In order to support tracing metrics for prepareToMeasureValuesAsync method,
we need to be able to trigger tracing between prepareToMeasureValuesAsync &
actual test run. This CL refactor prepareToMeasureValuesAsync so that the
actual test code is specified as a callback, which allows the framework
to interject the startTrace() call before running test in the future.

BUG=701059
==========

to

==========
Refactor prepareToMeasureValuesAsync to startMeasureValuesAsyn which run test
through callback

Motivation:
In order to support tracing metrics for prepareToMeasureValuesAsync method,
we need to be able to trigger tracing between prepareToMeasureValuesAsync &
actual test run. This CL rename prepareToMeasureValuesAsync method to 
startMeasureValuesAsyn & refactor it  so that the
actual test code is specified as a callback, which allows the framework
to interject the startTrace() call before running test in the future.

BUG=701059
==========

[nednguyen, 2017-05-04 00:11:56]

The CQ bit was checked by nednguyen@google.com

[nednguyen, 2017-05-04 00:11:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
wangxianzhu@chromium.org
Link to the patchset: https://codereview.chromium.org/2858783003/#ps90001
(title: "Address Xianzhu's review comments")

[commit-bot: I haz the power, 2017-05-04 00:12:36]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-04 01:27:38]

CQ is committing da patch.

Bot data: {"patchset_id": 90001, "attempt_start_ts": 1493856716779110,
"parent_rev": "3c2d1c61befedf006810b4e9d663d6dfae602eeb", "commit_rev":
"8dc2204a41db0b7b0f66c529c53d4c4a5d9d7fb5"}

[commit-bot: I haz the power, 2017-05-04 01:27:46]

Description was changed from

==========
Refactor prepareToMeasureValuesAsync to startMeasureValuesAsyn which run test
through callback

Motivation:
In order to support tracing metrics for prepareToMeasureValuesAsync method,
we need to be able to trigger tracing between prepareToMeasureValuesAsync &
actual test run. This CL rename prepareToMeasureValuesAsync method to 
startMeasureValuesAsyn & refactor it  so that the
actual test code is specified as a callback, which allows the framework
to interject the startTrace() call before running test in the future.

BUG=701059
==========

to

==========
Refactor prepareToMeasureValuesAsync to startMeasureValuesAsyn which run test
through callback

Motivation:
In order to support tracing metrics for prepareToMeasureValuesAsync method,
we need to be able to trigger tracing between prepareToMeasureValuesAsync &
actual test run. This CL rename prepareToMeasureValuesAsync method to 
startMeasureValuesAsyn & refactor it  so that the
actual test code is specified as a callback, which allows the framework
to interject the startTrace() call before running test in the future.

BUG=701059

Review-Url: https://codereview.chromium.org/2858783003
Cr-Commit-Position: refs/heads/master@{#469233}
Committed:
https://chromium.googlesource.com/chromium/src/+/8dc2204a41db0b7b0f66c529c53d...
==========

[commit-bot: I haz the power, 2017-05-04 01:27:47]

Committed patchset #5 (id:90001) as
https://chromium.googlesource.com/chromium/src/+/8dc2204a41db0b7b0f66c529c53d...