Bug 98691 – Perf. test results page is broken when runs have different sets of tests

• Component: New Bugs
• Status: RESOLVED FIXED

[Ryosuke Niwa]

Perf. test results page is broken when runs have different sets of tests

[Ryosuke Niwa]

Created attachment 167623 [details]
Patch

[Ryosuke Niwa]

Created attachment 167628 [details]
Sample output

[Ryosuke Niwa]

Created attachment 167629 [details]
Patch

[Ryosuke Niwa]

Created attachment 167630 [details]
Sample output 2

[Daniel Bates]

Comment on attachment 167629 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=167629&action=review

> PerformanceTests/ChangeLog:11
> +        Also fix regressions from the previous patch that broke the reference run switch
> +        and that results page fails to load js files on the remote server.

This sentence does not read well. I take it that these regressions are related closely enough to the test results page being broken with runs that have different sets of tests that it makes sense to fix all these issues in the same patch.

> PerformanceTests/resources/results-template.html:496
> +        while (runs[runIndex] !== results[i].run() && runIndex < runs.length)

Without loss of generality, take runs.length := 0. Then we access runs[0], which is outside the array bounds. 

I suggest reversing the order of the conjuncts here such that we check that runIndex < runs.length before accessing runs[runIndex]. Although accessing an array element whose index is outside the bounds of the array returns undefined in JavaScript, it's good practice to ensure that we're within the array bounds before accessing an array element. A side benefit of this change is that we avoid a comparison and a call to results[i].run() (which I haven't verified whether it has side effects (*)).

(*) Can you save me some time, does calling results[i].run() have side effects? If I have a moment, I'll take a look at the code. If it doesn't have side effects then I suggest we cache it so that we don't recompute it on each while-loop iteration.

[Ryosuke Niwa]

Comment on attachment 167629 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=167629&action=review

>> PerformanceTests/resources/results-template.html:496
>> +        while (runs[runIndex] !== results[i].run() && runIndex < runs.length)
> 
> Without loss of generality, take runs.length := 0. Then we access runs[0], which is outside the array bounds. 
> 
> I suggest reversing the order of the conjuncts here such that we check that runIndex < runs.length before accessing runs[runIndex]. Although accessing an array element whose index is outside the bounds of the array returns undefined in JavaScript, it's good practice to ensure that we're within the array bounds before accessing an array element. A side benefit of this change is that we avoid a comparison and a call to results[i].run() (which I haven't verified whether it has side effects (*)).
> 
> (*) Can you save me some time, does calling results[i].run() have side effects? If I have a moment, I'll take a look at the code. If it doesn't have side effects then I suggest we cache it so that we don't recompute it on each while-loop iteration.

Sure. I can reverse the order. run() doesn't have a side effect but there probably isn't a need to cache either because it's defined as:
this.run = function () { return associatedRun; }

[Daniel Bates]

Comment on attachment 167629 [details]
Patch

This patch looks sane to me.

[Daniel Bates]

Comment on attachment 167629 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=167629&action=review

> PerformanceTests/resources/results-template.html:499
> +        if (runIndex == referenceIndex)
> +            referenceResult = results[i];

Is this intended? In particular, consider when runIndex, referenceIndex := runs.length.

[Ryosuke Niwa]

(In reply to comment #8)
> (From update of attachment 167629 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=167629&action=review
> 
> > PerformanceTests/resources/results-template.html:499
> > +        if (runIndex == referenceIndex)
> > +            referenceResult = results[i];
> 
> Is this intended? In particular, consider when runIndex, referenceIndex := runs.length.

Yeah, that's fine because then results[i] will be undefined in that case.

[Ryosuke Niwa]

Committed r130711: <http://trac.webkit.org/changeset/130711>