123255 2013-10-24 01:03:55 -0700 Add result caching for Math.cos 2013-10-31 11:49:34 -0700 1 1 1 Unclassified WebKit JavaScriptCore 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 0 itoral webkit-unassigned commit-queue fpizlo ggaren oldest_to_newest 943018 0 itoral 2013-10-24 01:03:55 -0700 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. 943021 1 215035 itoral 2013-10-24 01:13:52 -0700 Created attachment 215035 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. 944956 2 215035 bfulgham 2013-10-30 10:06:05 -0700 Comment on attachment 215035 Adds result caching for Math.cos r=me 944998 3 215035 commit-queue 2013-10-30 10:45:49 -0700 Comment on attachment 215035 Adds result caching for Math.cos Clearing flags on attachment: 215035 Committed r158281: <http://trac.webkit.org/changeset/158281> 944999 4 commit-queue 2013-10-30 10:45:51 -0700 All reviewed patches have been landed. Closing bug. 945234 5 fpizlo 2013-10-30 16:44:44 -0700 Was this based on any benchmarking? If so, why aren't the benchmarks part of the patch? 945423 6 itoral 2013-10-31 00:04:59 -0700 (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 :( 945539 7 fpizlo 2013-10-31 09:22:24 -0700 (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. 945541 8 fpizlo 2013-10-31 09:23:40 -0700 Anyway, see: https://bugs.webkit.org/show_bug.cgi?id=123574 945618 9 fpizlo 2013-10-31 11:49:34 -0700 (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. 215035 2013-10-24 01:13:52 -0700 2013-10-30 10:45:49 -0700 Adds result caching for Math.cos webkit.jsc.cachedcos.patch text/plain 1665 itoral SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMTU3OTE3KQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE0IEBA CisyMDEzLTEwLTI0ICBJYWdvIFRvcmFsIFF1aXJvZ2EgIDxpdG9yYWxAaWdhbGlhLmNvbT4KKwor ICAgICAgICBBZGQgcmVzdWx0IGNhY2hpbmcgZm9yIE1hdGguY29zCisgICAgICAgIGh0dHBzOi8v YnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xMjMyNTUKKworICAgICAgICBSZXZpZXdl ZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICAqIHJ1bnRpbWUvTWF0aE9iamVjdC5jcHA6 CisgICAgICAgIChKU0M6Om1hdGhQcm90b0Z1bmNDb3MpOgorICAgICAgICAqIHJ1bnRpbWUvVk0u aDoKKwogMjAxMy0xMC0yMyAgRmlsaXAgUGl6bG8gIDxmcGl6bG9AYXBwbGUuY29tPgogCiAgICAg ICAgIFB1dCBhbGwgdXNlcyBvZiBMTFZNIGludHJpbnNpY3MgYmVoaW5kIGEgc2luZ2xlIE9wdGlv bgpJbmRleDogU291cmNlL0phdmFTY3JpcHRDb3JlL3J1bnRpbWUvTWF0aE9iamVjdC5jcHAKPT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PQotLS0gU291cmNlL0phdmFTY3JpcHRDb3JlL3J1bnRpbWUvTWF0aE9iamVjdC5jcHAJ KHJldmlzaW9uIDE1NzkxNykKKysrIFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9ydW50aW1lL01hdGhP YmplY3QuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC0xMzYsNyArMTM2LDcgQEAKIAogRW5jb2RlZEpT VmFsdWUgSlNDX0hPU1RfQ0FMTCBtYXRoUHJvdG9GdW5jQ29zKEV4ZWNTdGF0ZSogZXhlYykKIHsK LSAgICByZXR1cm4gSlNWYWx1ZTo6ZW5jb2RlKGpzRG91YmxlTnVtYmVyKGNvcyhleGVjLT5hcmd1 bWVudCgwKS50b051bWJlcihleGVjKSkpKTsKKyAgICByZXR1cm4gSlNWYWx1ZTo6ZW5jb2RlKGV4 ZWMtPnZtKCkuY2FjaGVkQ29zKGV4ZWMtPmFyZ3VtZW50KDApLnRvTnVtYmVyKGV4ZWMpKSk7CiB9 CiAKIEVuY29kZWRKU1ZhbHVlIEpTQ19IT1NUX0NBTEwgbWF0aFByb3RvRnVuY0V4cChFeGVjU3Rh dGUqIGV4ZWMpCkluZGV4OiBTb3VyY2UvSmF2YVNjcmlwdENvcmUvcnVudGltZS9WTS5oCj09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT0KLS0tIFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9ydW50aW1lL1ZNLmgJKHJldmlzaW9uIDE1 NzkxNykKKysrIFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9ydW50aW1lL1ZNLmgJKHdvcmtpbmcgY29w eSkKQEAgLTQyMCw2ICs0MjAsNyBAQAogICAgICAgICBUaHJlYWRJZGVudGlmaWVyIGV4Y2x1c2l2 ZVRocmVhZDsKIAogICAgICAgICBDYWNoZWRUcmFuc2NlbmRlbnRhbEZ1bmN0aW9uPHN0ZDo6c2lu PiBjYWNoZWRTaW47CisgICAgICAgIENhY2hlZFRyYW5zY2VuZGVudGFsRnVuY3Rpb248c3RkOjpj b3M+IGNhY2hlZENvczsKIAogICAgICAgICBKU19FWFBPUlRfUFJJVkFURSB2b2lkIHJlc2V0RGF0 ZUNhY2hlKCk7CiAK