Bug 123255 – Add result caching for Math.cos

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Iago Toral]

Currently JavaScriptCore caches results for Math.sin, as introduced by the patch for bug #38714. I think it makes sense to do the same for Math.cos, since the same rationale used for Math.sin applies to Math.cos and also because these two functions are typically used together, for example to compute rotation transformations.

I'll upload a patch shortly.

[Iago Toral]

Created attachment 215035 [details]
Adds result caching for Math.cos

A quick test consisting of averaging 5 consecutive runs of Sunspider shows a 1.9% benefit in my case.

[Brent Fulgham]

Comment on attachment 215035 [details]
Adds result caching for Math.cos

r=me

[WebKit Commit Bot]

Comment on attachment 215035 [details]
Adds result caching for Math.cos

Clearing flags on attachment: 215035

Committed r158281: <http://trac.webkit.org/changeset/158281>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Filip Pizlo]

Was this based on any benchmarking?  If so, why aren't the benchmarks part of the patch?

[Iago Toral]

(In reply to comment #5)
> Was this based on any benchmarking?  If so, why aren't the benchmarks part of the patch?

I ran the sunspider benchmark to assess performance improvement, my tests showed a 1.9% performance improvement on this benchmark as I mention in comment #1.

I did not know there was a policy to include the benchmark results in the ChangeLog as part of the patch, so that was my bad :(

[Filip Pizlo]

(In reply to comment #1)
> Created an attachment (id=215035) [details]
> Adds result caching for Math.cos
> 
> A quick test consisting of averaging 5 consecutive runs of Sunspider shows a 1.9% benefit in my case.

It's customary to post full performance results to bugzilla for optimizations like these.

It's customary to run more than just SunSpider.  It's unwise to rely on a single benchmark for optimizations.  For example, Octane, JSBench, and Kraken are also important benchmarks.

[Filip Pizlo]

Anyway, see: https://bugs.webkit.org/show_bug.cgi?id=123574

[Filip Pizlo]

(In reply to comment #7)
> (In reply to comment #1)
> > Created an attachment (id=215035) [details] [details]
> > Adds result caching for Math.cos
> > 
> > A quick test consisting of averaging 5 consecutive runs of Sunspider shows a 1.9% benefit in my case.
> 
> It's customary to post full performance results to bugzilla for optimizations like these.
> 
> It's customary to run more than just SunSpider.  It's unwise to rely on a single benchmark for optimizations.  For example, Octane, JSBench, and Kraken are also important benchmarks.

Also, I cannot reproduce the speed-up that you're reporting.  I'm seeing a 0.35% difference before and after your patch and that difference is in the noise.