Bug 71801

Created attachment 114045 [details] Patch

Compared to before, where these routines measured integral milliseconds, this looks to be a huge lose. However it is more accurate. TEST COMPARISON FROM TO DETAILS ============================================================================= ** TOTAL **: *1.087x as slow* 162.2ms +/- 1.0% 176.2ms +/- 1.0% significant ============================================================================= 3d: *1.091x as slow* 25.3ms +/- 5.2% 27.6ms +/- 5.7% significant cube: ?? 10.1ms +/- 12.7% 11.8ms +/- 13.6% not conclusive: might be *1.164x as slow* morph: *1.075x as slow* 7.2ms +/- 4.2% 7.7ms +/- 3.4% significant raytrace: *1.013x as slow* 8.0ms +/- 0.0% 8.1ms +/- 0.8% significant access: *1.076x as slow* 21.2ms +/- 2.1% 22.8ms +/- 1.2% significant binary-trees: *1.112x as slow* 2.0ms +/- 0.0% 2.2ms +/- 1.0% significant fannkuch: ?? 10.9ms +/- 2.1% 11.2ms +/- 2.5% not conclusive: might be *1.026x as slow* nbody: *1.115x as slow* 5.3ms +/- 6.5% 5.9ms +/- 1.1% significant nsieve: *1.162x as slow* 3.0ms +/- 0.0% 3.5ms +/- 2.6% significant bitops: *1.143x as slow* 14.3ms +/- 2.4% 16.3ms +/- 1.7% significant 3bit-bits-in-byte: *1.123x as slow* 2.0ms +/- 0.0% 2.2ms +/- 0.9% significant bits-in-byte: *1.101x as slow* 5.2ms +/- 5.8% 5.7ms +/- 3.6% significant bitwise-and: *1.102x as slow* 3.0ms +/- 0.0% 3.3ms +/- 2.4% significant nsieve-bits: *1.24x as slow* 4.1ms +/- 5.5% 5.1ms +/- 2.8% significant controlflow: *1.93x as slow* 1.0ms +/- 0.0% 1.9ms +/- 5.2% significant recursive: *1.93x as slow* 1.0ms +/- 0.0% 1.9ms +/- 5.2% significant crypto: *1.156x as slow* 10.1ms +/- 2.2% 11.7ms +/- 2.2% significant aes: *1.088x as slow* 6.0ms +/- 0.0% 6.5ms +/- 0.5% significant md5: *1.38x as slow* 2.1ms +/- 10.8% 2.9ms +/- 6.9% significant sha1: *1.131x as slow* 2.0ms +/- 0.0% 2.3ms +/- 1.6% significant date: *1.060x as slow* 18.1ms +/- 1.2% 19.2ms +/- 0.9% significant format-tofte: *1.039x as slow* 10.1ms +/- 2.2% 10.5ms +/- 1.2% significant format-xparb: *1.088x as slow* 8.0ms +/- 0.0% 8.7ms +/- 1.7% significant math: *1.095x as slow* 16.1ms +/- 1.4% 17.6ms +/- 1.0% significant cordic: *1.165x as slow* 5.1ms +/- 4.4% 5.9ms +/- 1.1% significant partial-sums: *1.041x as slow* 8.0ms +/- 0.0% 8.3ms +/- 1.7% significant spectral-norm: *1.120x as slow* 3.0ms +/- 0.0% 3.4ms +/- 3.1% significant regexp: *1.066x as slow* 12.3ms +/- 2.8% 13.1ms +/- 1.5% significant dna: *1.066x as slow* 12.3ms +/- 2.8% 13.1ms +/- 1.5% significant string: *1.049x as slow* 43.8ms +/- 1.0% 45.9ms +/- 0.7% significant base64: *1.080x as slow* 4.0ms +/- 0.0% 4.3ms +/- 4.2% significant fasta: *1.106x as slow* 6.2ms +/- 4.9% 6.9ms +/- 1.4% significant tagcloud: ?? 11.0ms +/- 0.0% 11.3ms +/- 3.2% not conclusive: might be *1.028x as slow* unpack-code: *1.024x as slow* 17.6ms +/- 2.1% 18.0ms +/- 0.6% significant validate-input: *1.086x as slow* 5.0ms +/- 0.0% 5.4ms +/- 2.0% significant

As you would expect, the effect on v8-v4 is much less: TEST COMPARISON FROM TO DETAILS ============================================================================= ** TOTAL **: *1.011x as slow* 1074.8ms +/- 0.6% 1087.0ms +/- 0.4% significant ============================================================================= v8: *1.011x as slow* 1074.8ms +/- 0.6% 1087.0ms +/- 0.4% significant crypto: ?? 186.0ms +/- 1.3% 186.3ms +/- 0.7% not conclusive: might be *1.002x as slow* deltablue: *1.012x as slow* 255.7ms +/- 0.7% 258.7ms +/- 0.2% significant earley-boyer: *1.011x as slow* 100.9ms +/- 0.7% 102.0ms +/- 0.6% significant raytrace: *1.019x as slow* 64.9ms +/- 1.0% 66.2ms +/- 0.3% significant regexp: ?? 111.4ms +/- 0.8% 113.6ms +/- 2.4% not conclusive: might be *1.020x as slow* richards: *1.013x as slow* 219.0ms +/- 0.6% 221.9ms +/- 0.6% significant splay: *1.010x as slow* 136.9ms +/- 0.4% 138.3ms +/- 0.3% significant

Adding JSC and SunSpider folk to the cc.

You could also use Tools/Scripts/bencher, which does the same thing.

(In reply to comment #2) > Compared to before, where these routines measured integral milliseconds, this looks to be a huge lose. However it is more accurate. > > TEST COMPARISON FROM TO DETAILS > > ============================================================================= > > ** TOTAL **: *1.087x as slow* 162.2ms +/- 1.0% 176.2ms +/- 1.0% significant I'd be fine with this patch except for this giant slow-down. If you want this functionality, you should probably investigate what happened here. Tools/Scripts/bencher uses our internal preciseTime() function, which returns an untruncated time. So if you want more precise results, you could just use that.

The "slowdown" is not a slowdown in actual fact; it simply reflects increased precision when measuring times. Because the original sunspider harness measures integral milliseconds only, produced by truncating (not rounding) the double-precision time values, it is natural to expect that the higher-precision times indicate longer test runs.

Created attachment 116051 [details] Patch

Updated the driver to use preciseTime() instead of jsc's run(). Note that this does change the default environment of the test code; `run' uses a fresh global, whereas `load' exposes the current global to the loaded code. We do want `load' behavior in the data case though, and having them unified makes a little more sense. I promise I'll switch to bencher soon, just wanted to follow up with this ;-)

Created attachment 116053 [details] Patch

Comment on attachment 116053 [details] Patch Fix log message, set cq?

Does it mean that the other JS engines (e.g., V8, Mozilla) also need to provide preciseTime() interface, if we want to run them using the scripts?

(In reply to comment #12) > Does it mean that the other JS engines (e.g., V8, Mozilla) also need to provide preciseTime() interface, if we want to run them using the scripts? The approach taken by bencher is that it only uses preciseTime() if all VMs being tested provide it. I tend to think that the run() function and the Date API should be consistent across VMs, and especially with the Date API, it's unclear if we could make that more precise without breaking compliance.

(In reply to comment #12) > Does it mean that the other JS engines (e.g., V8, Mozilla) also need to provide preciseTime() interface, if we want to run them using the scripts? This is a moot point, as our sunspider harness currently uses `run', which is not available on V8 at least. Dunno about SpiderMonkey.

CCing other JSC experts who might be willing to review this.

Filip, Geoff, Gavin: would someone care to review this please? I think I adequately addressed the concerns (notably, that it's not an actual slowdown, and we're already using jsc-specific API). Thanks, Andy

(In reply to comment #16) > Filip, Geoff, Gavin: would someone care to review this please? I think I adequately addressed the concerns (notably, that it's not an actual slowdown, and we're already using jsc-specific API). Kind ping :-)

(In reply to comment #17) > (In reply to comment #16) > > Filip, Geoff, Gavin: would someone care to review this please? I think I adequately addressed the concerns (notably, that it's not an actual slowdown, and we're already using jsc-specific API). > > Kind ping :-) Another kind ping :)

Comment on attachment 116053 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=116053&action=review > PerformanceTests/SunSpider/resources/sunspider-standalone-driver.js:-54 > - times[j] = run(testName); Where did this line go?

MJS has been the historical keeper of sunspider. i woudl be happy to approve this if I was convinced there was no behavior change other than using a more precise timer. Also, doesn't v8-builds use this file as well?

(In reply to comment #19) > (From update of attachment 116053 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=116053&action=review > > > PerformanceTests/SunSpider/resources/sunspider-standalone-driver.js:-54 > > - times[j] = run(testName); > > Where did this line go? It went away. Instead we use preciseTime() before and after. (In reply to comment #20) > MJS has been the historical keeper of sunspider. i woudl be happy to approve this if I was convinced there was no behavior change other than using a more precise timer. Also, doesn't v8-builds use this file as well? V8 version 3.8.6 (candidate) [console: readline] d8> run (d8):1: ReferenceError: run is not defined run ^ So we are already using JSC-specific API here; I don't see how v8 builds could be running this file. Regards, Andy

> MJS has been the historical keeper of sunspider. I'd be really interested to hear his thoughts on this as well. Please forgive the naive question, but I'm wondering, does this affect what runs here: http://www.webkit.org/perf/sunspider-0.9.1/sunspider-0.9.1/driver.html ? If so, don't we need to keep it cross browser compatible? If not, are we concerned that the results used within the project are not comparable to the public ones? Should we maintain support for timing the same way as the public one? Should the results on rniwa's perf dashboard match internal or public? Also worth noting there is talk in the public-web-perf group about adding a high-res monotonically increasing timer to the web platform -- likely as window.performance.now() -- see bug 66684.

(In reply to comment #22) > does this affect what runs here: http://www.webkit.org/perf/sunspider-0.9.1/sunspider-0.9.1/driver.html ? I don't think so, no -- those runs are "hosted", whereas this change is to the "standalone" driver. > If not, are we concerned that the results used within the project are not comparable to the public ones? Good question. My thought is that while hosted and standalone timings should be similar, they have never been the same, for many good reasons. I wouldn't worry about it -- especially if, as you note, a window.performance.now() lands. But, Maciej's (and anyone else's) opinions are welcome here. Cheers, Andy

Is anyone willing to review this patch?

(In reply to comment #24) > Is anyone willing to review this patch? As Eric mentioned, Maciej is probably the only person who can review this patch.

Comment on attachment 116053 [details] Patch We sometimes use command-line SunSpider to compare to other JS engines. So making a JSC-specific change that materially alters the timings would be unfortunate. It's great that it is better precision, but it shows a ~9% hit that does not affect the other engines. Also, as written, it has no fallback path. Therefore, I suggest making the new, more precise timing an optional mode explicitly invoked by a command-line argument to the script. This would allow the option of more precise measurements without losing the cross-engine comparison features of the old mode.

Maciej! Thank you for commenting. (In reply to comment #26) > (From update of attachment 116053 [details]) > We sometimes use command-line SunSpider to compare to other JS engines. Evidently not very often :) $ ./sunspider --shell ~/src/v8/d8 Found 26 tests Running SunSpider once for warmup, then 10 times Discarded first run. [resources/sunspider-standalone-driver.js:54: ReferenceError: run is not defined times[j] = run(testName); ^ ReferenceError: run is not defined I mentioned this in comment 14. Does this change your opinion on the matter? Should we re-add a portable path? The code has been non-portable since November 2010, with r72842. Andy

(In reply to comment #27) > Maciej! Thank you for commenting. > > (In reply to comment #26) > > (From update of attachment 116053 [details] [details]) > > We sometimes use command-line SunSpider to compare to other JS engines. > > Evidently not very often :) > > $ ./sunspider --shell ~/src/v8/d8 > Found 26 tests > Running SunSpider once for warmup, then 10 times > Discarded first run. > [resources/sunspider-standalone-driver.js:54: ReferenceError: run is not defined > times[j] = run(testName); > ^ > ReferenceError: run is not defined > > I mentioned this in comment 14. > > Does this change your opinion on the matter? Should we re-add a portable path? The code has been non-portable since November 2010, with r72842. Another way to look at Maciej's comments is that with this change, it would be very difficult to compare old versions of jsc to new ones. I do this all the time, so this change would be a showstopper for me.

Hi Filip :) (In reply to comment #28) > Another way to look at Maciej's comments is that with this change, it would be very difficult to compare old versions of jsc to new ones. I certainly do not mean to disrespect the voice of experience :) However, I wonder if I am communicating clearly: this patch does /not/ change the behavior of `run' in jsc.cpp; it instead changes the sunspider harness to use preciseTime, which is present in very old jsc.

It looks like Filip added preciseTime: http://trac.webkit.org/changeset/91817

I support Maicej's point that we shouldn't make this change unless it's optional. And yes, we should be fixing those exceptions so that it runs on all major browsers.

(In reply to comment #29) > Hi Filip :) > > (In reply to comment #28) > > Another way to look at Maciej's comments is that with this change, it would be very difficult to compare old versions of jsc to new ones. > > I certainly do not mean to disrespect the voice of experience :) However, I wonder if I am communicating clearly: this patch does /not/ change the behavior of `run' in jsc.cpp; it instead changes the sunspider harness to use preciseTime, which is present in very old jsc. And I like to compare to very old jsc.