Bug 152938 - Update Sunspider to resume SunSpider coverage.
Summary: Update Sunspider to resume SunSpider coverage.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: dewei_zhu
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-01-08 18:47 PST by dewei_zhu
Modified: 2016-01-12 13:53 PST (History)
6 users (show)

See Also:


Attachments
Patch (2.04 KB, patch)
2016-01-08 18:47 PST, dewei_zhu
no flags Details | Formatted Diff | Diff
Patch (1.24 KB, patch)
2016-01-11 15:56 PST, dewei_zhu
no flags Details | Formatted Diff | Diff
Patch (18.27 KB, patch)
2016-01-11 17:53 PST, dewei_zhu
no flags Details | Formatted Diff | Diff
Patch (2.07 KB, patch)
2016-01-12 11:34 PST, dewei_zhu
no flags Details | Formatted Diff | Diff
Patch (6.90 KB, patch)
2016-01-12 13:42 PST, dewei_zhu
rniwa: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description dewei_zhu 2016-01-08 18:47:23 PST
Update the patch for Sunspider to resume SunSpider coverage.
Comment 1 dewei_zhu 2016-01-08 18:47:38 PST
Created attachment 268600 [details]
Patch
Comment 2 WebKit Commit Bot 2016-01-08 18:49:30 PST
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.
Comment 3 Ryosuke Niwa 2016-01-09 19:59:52 PST
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.
Comment 4 dewei_zhu 2016-01-11 15:56:15 PST
Created attachment 268722 [details]
Patch
Comment 5 dewei_zhu 2016-01-11 16:13:06 PST
I've tested on major browsers and do not see noticeable score changes with/without my patch.
Comment 6 Ryosuke Niwa 2016-01-11 16:39:23 PST
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?
Comment 7 Geoffrey Garen 2016-01-11 16:46:14 PST
> 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.
Comment 8 Geoffrey Garen 2016-01-11 16:46:55 PST
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.
Comment 9 Ryosuke Niwa 2016-01-11 17:12:53 PST
(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.
Comment 10 dewei_zhu 2016-01-11 17:53:42 PST
Created attachment 268736 [details]
Patch
Comment 11 Ryosuke Niwa 2016-01-11 18:16:04 PST
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 12 dewei_zhu 2016-01-12 11:28:42 PST
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?
Comment 13 dewei_zhu 2016-01-12 11:34:19 PST
Created attachment 268790 [details]
Patch
Comment 14 dewei_zhu 2016-01-12 13:42:25 PST
Created attachment 268806 [details]
Patch
Comment 15 dewei_zhu 2016-01-12 13:53:34 PST
Committed r194924: <http://trac.webkit.org/changeset/194924>