Bug 98691

Created attachment 167623 [details] Patch

Created attachment 167628 [details] Sample output

Created attachment 167629 [details] Patch

Created attachment 167630 [details] Sample output 2

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.

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; }

Comment on attachment 167629 [details] Patch This patch looks sane to me.

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.

(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.

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