Bug 152938 – Update Sunspider to resume SunSpider coverage.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

Update the patch for Sunspider to resume SunSpider coverage.

Created attachment 268600 [details]
Patch

[WebKit Commit Bot]

Attachment 268600 [details] did not pass style-queue:


ERROR: Tools/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ryosuke Niwa]

Comment on attachment 268600 [details]
Patch

We should fix the sun spider directly since this would affect people running this benchmark in the browser as well.

Created attachment 268722 [details]
Patch

I've tested on major browsers and do not see noticeable score changes with/without my patch.

[Ryosuke Niwa]

Comment on attachment 268722 [details]
Patch

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

> PerformanceTests/SunSpider/ChangeLog:9
> +        Remove 'sunspider.css' subresource in test contents which is unused in the test and may blocks script execution.

You should also increase the sun spider version number since this is a substantial change in the way tests are ran.  1.0.3?

[Geoffrey Garen]

> You should also increase the sun spider version number since this is a
> substantial change in the way tests are ran.  1.0.3?

I think we should avoid bumping the version number since we're officially not maintaining SunSpider anymore, and this is just a small change to keep it running on an old bot, which doesn't affect anyone's score or the core logic of the benchmark.

[Geoffrey Garen]

You'll need to edit Websites/webkit.org if you want to see the change reflected on the web. PerformanceTests/SunSpider/make-hosted is the script to re-generate SunSpider from its template.

[Ryosuke Niwa]

(In reply to comment #8)
> You'll need to edit Websites/webkit.org if you want to see the change
> reflected on the web. PerformanceTests/SunSpider/make-hosted is the script
> to re-generate SunSpider from its template.

In that case, why don't we fix it in SunSpider.patch, which is used by run-benchmark to "fix up" benchmarks as well as adding reporting mechanisms.

Created attachment 268736 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 268736 [details]
Patch

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

> Websites/webkit.org/ChangeLog:13
> +        * perf/sunspider-1.0.1/sunspider-1.0.1/sunspider-test-contents.js:
> +        * perf/sunspider-1.0.2/sunspider-1.0.2/sunspider-test-contents.js:
> +        * perf/sunspider-1.0/sunspider-1.0/sunspider-test-contents.js:

I don't think we don't want to update released versions of the benchmark per Geoff's comment
since SunSpider is no longer maintained.

> PerformanceTests/SunSpider/ChangeLog:9
> +        Remove 'sunspider.css' subresource in test contents which is unused in the test and may block script execution.

If we're not increasing the version number, I think we should go back to your original approach of fixing it up in the patch file.

Comment on attachment 268736 [details]
Patch

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

>> PerformanceTests/SunSpider/ChangeLog:9
>> +        Remove 'sunspider.css' subresource in test contents which is unused in the test and may block script execution.
> 
> If we're not increasing the version number, I think we should go back to your original approach of fixing it up in the patch file.

Which approach should we use in the patch file, moving the setTimeout or removing sub resource?

Created attachment 268790 [details]
Patch

Created attachment 268806 [details]
Patch

Committed r194924: <http://trac.webkit.org/changeset/194924>