160799 2016-08-11 22:22:08 -0700 Replace % by bitwise & 2016-08-15 23:45:43 -0700 1 1 1 Unclassified WebKit Web Template Framework WebKit Nightly Build Unspecified Unspecified NEW P2 Normal --- 1 rajeevmisraforapple webkit-unassigned benjamin cdumez cmarcelo commit-queue oldest_to_newest 1219540 0 rajeevmisraforapple 2016-08-11 22:22:08 -0700 Replace % by bitwise & 1219541 1 285895 rajeevmisraforapple 2016-08-11 22:27:02 -0700 Created attachment 285895 Patch 1219548 2 285895 achristensen 2016-08-11 23:17:47 -0700 Comment on attachment 285895 Patch This needs a change log entry. Use Tools/Scripts/prepare-ChangeLog to generate a template 1219551 3 285899 rajeevmisraforapple 2016-08-11 23:33:54 -0700 Created attachment 285899 Patch 1219581 4 285899 ap 2016-08-12 07:30:45 -0700 Comment on attachment 285899 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=285899&action=review > Source/WTF/ChangeLog:8 > + Replacing mod operation by bitwise & Did you measure a performance improvement from this change? 1219598 5 285899 rajeevmisraforapple 2016-08-12 09:45:20 -0700 Comment on attachment 285899 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=285899&action=review > Source/WTF/ChangeLog:9 > + If we make size of HashTable as power I tried to run TestWTF with "/usr/bin/time" to measure usr/sys/real times, but variation for different test runs made me conclude that it is not the reliable way to measure performance. This variation is probably because tests in TestWTF depends on scheduling order of different threads which might be different in each run. Is there any official tool/tests/process to measure performance in webkit? I am not sure if there will be a immediate measurable performance improvement with this single change. This change was done with the thought that "If same result can be achieved with lesser cpu cycle, we should use that approach". I am guessing division by "power of 2 number" done thru & uses lesser cpu cycle than division by some arbitrary number. I think these smaller improvements could slowly adds up to give us better performance over time. 1220390 6 285899 darin 2016-08-15 23:45:43 -0700 Comment on attachment 285899 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=285899&action=review >> Source/WTF/ChangeLog:9 >> + If we make size of HashTable as power > > I tried to run TestWTF with "/usr/bin/time" to measure usr/sys/real times, but variation for > different test runs made me conclude that it is not the reliable way to measure performance. > This variation is probably because tests in TestWTF depends on scheduling order of different > threads which might be different in each run. > > Is there any official tool/tests/process to measure performance in webkit? > > I am not sure if there will be a immediate measurable performance improvement with > this single change. > > This change was done with the thought that "If same result can be achieved with lesser > cpu cycle, we should use that approach". I am guessing division by "power of 2 number" > done thru & uses lesser cpu cycle than division by some arbitrary number. > > I think these smaller improvements could slowly adds up to give us better performance > over time. Sorry, that’s not how we do performance on this project. We don’t make changes based on a guess that something might be measurably faster. Further, I think your guess is likely incorrect. We do indeed have many real speedups that result from a collection many smaller improvements, but it is almost always many smaller *measurable* ones. Not changes that we guess are an improvement but can’t measure. There are lots of techniques to measure performance in WebKit but I doubt there is any that will be able to measure this. All the standard techniques, such as writing a targeted micro benchmark that uses a function in a tight loop. If making the table larger to avoid collisions is worth it, you might be able to prove that using collision statistics. But I don’t think any of that is going to work here; this really isn’t a worthwhile improvement. It makes the code harder to read and almost certainly does not provide a commensurate speedup. I think you should choose a more valuable project for your first WebKit patch, perhaps a bug fix. Do you have any other areas of interest? > Source/WTF/wtf/ParkingLot.cpp:220 > + int leadingZeros = size > 1 ? __builtin_clz(size - 1) : __builtin_clz(size); Does not compile on Windows because __builtin_clz is not supported on that platform. 285895 2016-08-11 22:27:02 -0700 2016-08-11 23:33:49 -0700 Patch bug-160799-20160811222524.patch text/plain 2098 rajeevmisraforapple SW5kZXg6IFNvdXJjZS9XVEYvd3RmL1BhcmtpbmdMb3QuY3BwCj09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJj ZS9XVEYvd3RmL1BhcmtpbmdMb3QuY3BwCShyZXZpc2lvbiAyMDQyMzkpCisrKyBTb3VyY2UvV1RG L3d0Zi9QYXJraW5nTG90LmNwcAkod29ya2luZyBjb3B5KQpAQCAtMjE1LDcgKzIxNSwxMSBAQCBz dHJ1Y3QgSGFzaHRhYmxlIHsKICAgICBzdGF0aWMgSGFzaHRhYmxlKiBjcmVhdGUodW5zaWduZWQg c2l6ZSkKICAgICB7CiAgICAgICAgIEFTU0VSVChzaXplID49IDEpOwotICAgICAgICAKKyAgICAg ICAgLy8gSWYgd2UgbWFrZSBzaXplID0gY2xvc2VzdCAiZ3JlYXRlciBvciBlcXVhbCIgdG8gcG93 ZXIgb2YgMiwgbGF0ZXIgb24gJSB3aXRoIHNpemUgb3BlcmF0aW9uIGNvdWxkIGJlIHJlcGxhY2Vk CisgICAgICAgIC8vIGJ5IHNpbXBsZSBiaXR3aXNlICYKKyAgICAgICAgaW50IGxlYWRpbmdaZXJv cyA9IHNpemUgPiAxID8gX19idWlsdGluX2NseihzaXplIC0gMSkgOiBfX2J1aWx0aW5fY2x6KHNp emUpOworICAgICAgICBzaXplID0gMVUgPDwgKChzaXplb2YodW5zaWduZWQpIDw8IDMpICAtIGxl YWRpbmdaZXJvcyk7CisKICAgICAgICAgSGFzaHRhYmxlKiByZXN1bHQgPSBzdGF0aWNfY2FzdDxI YXNodGFibGUqPigKICAgICAgICAgICAgIGZhc3RaZXJvZWRNYWxsb2Moc2l6ZW9mKEhhc2h0YWJs ZSkgKyBzaXplb2YoQXRvbWljPEJ1Y2tldCo+KSAqIChzaXplIC0gMSkpKTsKICAgICAgICAgcmVz dWx0LT5zaXplID0gc2l6ZTsKQEAgLTM4NCw3ICszODgsNyBAQCB2b2lkIGVuc3VyZUhhc2h0YWJs ZVNpemUodW5zaWduZWQgbnVtVGhyCiAgICAgICAgIGlmICh2ZXJib3NlKQogICAgICAgICAgICAg ZGF0YUxvZyh0b1N0cmluZyhjdXJyZW50VGhyZWFkKCksICI6IHJlaGFzaGluZyB0aHJlYWQgZGF0 YSAiLCBSYXdQb2ludGVyKHRocmVhZERhdGEpLCAiIHdpdGggYWRkcmVzcyA9ICIsIFJhd1BvaW50 ZXIodGhyZWFkRGF0YS0+YWRkcmVzcyksICJcbiIpKTsKICAgICAgICAgdW5zaWduZWQgaGFzaCA9 IGhhc2hBZGRyZXNzKHRocmVhZERhdGEtPmFkZHJlc3MpOwotICAgICAgICB1bnNpZ25lZCBpbmRl eCA9IGhhc2ggJSBuZXdIYXNodGFibGUtPnNpemU7CisgICAgICAgIHVuc2lnbmVkIGluZGV4ID0g aGFzaCAmIChuZXdIYXNodGFibGUtPnNpemUgLSAxKTsKICAgICAgICAgaWYgKHZlcmJvc2UpCiAg ICAgICAgICAgICBkYXRhTG9nKHRvU3RyaW5nKGN1cnJlbnRUaHJlYWQoKSwgIjogaW5kZXggPSAi LCBpbmRleCwgIlxuIikpOwogICAgICAgICBCdWNrZXQqIGJ1Y2tldCA9IG5ld0hhc2h0YWJsZS0+ ZGF0YVtpbmRleF0ubG9hZCgpOwpAQCAtNDY1LDcgKzQ2OSw3IEBAIGJvb2wgZW5xdWV1ZShjb25z dCB2b2lkKiBhZGRyZXNzLCBjb25zdCAKIAogICAgIGZvciAoOzspIHsKICAgICAgICAgSGFzaHRh YmxlKiBteUhhc2h0YWJsZSA9IGVuc3VyZUhhc2h0YWJsZSgpOwotICAgICAgICB1bnNpZ25lZCBp bmRleCA9IGhhc2ggJSBteUhhc2h0YWJsZS0+c2l6ZTsKKyAgICAgICAgdW5zaWduZWQgaW5kZXgg PSBoYXNoICYgKG15SGFzaHRhYmxlLT5zaXplIC0gMSk7CiAgICAgICAgIEF0b21pYzxCdWNrZXQq PiYgYnVja2V0UG9pbnRlciA9IG15SGFzaHRhYmxlLT5kYXRhW2luZGV4XTsKICAgICAgICAgQnVj a2V0KiBidWNrZXQ7CiAgICAgICAgIGZvciAoOzspIHsKQEAgLTUxNyw3ICs1MjEsNyBAQCBib29s IGRlcXVldWUoCiAKICAgICBmb3IgKDs7KSB7CiAgICAgICAgIEhhc2h0YWJsZSogbXlIYXNodGFi bGUgPSBlbnN1cmVIYXNodGFibGUoKTsKLSAgICAgICAgdW5zaWduZWQgaW5kZXggPSBoYXNoICUg bXlIYXNodGFibGUtPnNpemU7CisgICAgICAgIHVuc2lnbmVkIGluZGV4ID0gaGFzaCAmIChteUhh c2h0YWJsZS0+c2l6ZSAtIDEpOwogICAgICAgICBBdG9taWM8QnVja2V0Kj4mIGJ1Y2tldFBvaW50 ZXIgPSBteUhhc2h0YWJsZS0+ZGF0YVtpbmRleF07CiAgICAgICAgIEJ1Y2tldCogYnVja2V0ID0g YnVja2V0UG9pbnRlci5sb2FkKCk7CiAgICAgICAgIGlmICghYnVja2V0KSB7Cg== 285899 2016-08-11 23:33:54 -0700 2016-08-15 23:45:43 -0700 Patch bug-160799-20160811233216.patch text/plain 3061 rajeevmisraforapple SW5kZXg6IFNvdXJjZS9XVEYvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XVEYvQ2hh bmdlTG9nCShyZXZpc2lvbiAyMDQ0MDcpCisrKyBTb3VyY2UvV1RGL0NoYW5nZUxvZwkod29ya2lu ZyBjb3B5KQpAQCAtMSwzICsxLDIyIEBACisyMDE2LTA4LTExICBSYWplZXYgTWlzcmEgIDxyYWpl ZXZtaXNyYWZvcmFwcGxlQGdtYWlsLmNvbT4KKworICAgICAgICBOZWVkIGEgc2hvcnQgZGVzY3Jp cHRpb24gKE9PUFMhKS4KKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcu Y2dpP2lkPTE2MDc5OQorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisg ICAgICAgIFJlcGxhY2luZyBtb2Qgb3BlcmF0aW9uIGJ5IGJpdHdpc2UgJgorICAgICAgICBJZiB3 ZSBtYWtlIHNpemUgb2YgSGFzaFRhYmxlIGFzIHBvd2VyCisgICAgICAgIG9mIDIsIHdlIGNvdWxk IHVzZSAmIGR1cmluZyBtb2QgKCUpCisgICAgICAgIG9wZXJhdGlvbi4gV2UgdHJ5IHRvIGZpbmQg dGhlIGNsb3Nlc3QKKyAgICAgICAgImdyZWF0ZXIgb3IgZXF1YWwiIG51bWJlciB3aGljaCBpcyBw b3dlcgorICAgICAgICBvZiAyIG9mIGdpdmVuIHNpemUuIFRoaXMgd2F5IHdlIG92ZXIgZXN0aW1h dGUKKyAgICAgICAgd2hpY2ggbWlnaHQgZ2l2ZSB1cyBib3RoIGV4dHJhIHJvb20gdG8gYXZvaWQK KyAgICAgICAgY29sbGlzaW9uIGFzIHdlbGwgYXMgbW9kIG9wZXJhdGlvbiBzaG91bGQgCisgICAg ICAgIGJlY29tZSBzaW1wbGUuCisKKyAgICAgICAgKiB3dGYvUGFya2luZ0xvdC5jcHA6CisKIDIw MTYtMDgtMTEgIE1hdXJlZW4gRGF1bSAgPG1kYXVtQGFwcGxlLmNvbT4KIAogICAgICAgICBBZGQg TlNCdXR0b25UeXBlIHRvIEFwcEtpdENvbXBhdGliaWxpdHlEZWNsYXJhdGlvbnMuaC4KSW5kZXg6 IFNvdXJjZS9XVEYvd3RmL1BhcmtpbmdMb3QuY3BwCj09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XVEYv d3RmL1BhcmtpbmdMb3QuY3BwCShyZXZpc2lvbiAyMDQyMzkpCisrKyBTb3VyY2UvV1RGL3d0Zi9Q YXJraW5nTG90LmNwcAkod29ya2luZyBjb3B5KQpAQCAtMjE1LDcgKzIxNSwxMSBAQCBzdHJ1Y3Qg SGFzaHRhYmxlIHsKICAgICBzdGF0aWMgSGFzaHRhYmxlKiBjcmVhdGUodW5zaWduZWQgc2l6ZSkK ICAgICB7CiAgICAgICAgIEFTU0VSVChzaXplID49IDEpOwotICAgICAgICAKKyAgICAgICAgLy8g SWYgd2UgbWFrZSBzaXplID0gY2xvc2VzdCAiZ3JlYXRlciBvciBlcXVhbCIgdG8gcG93ZXIgb2Yg MiwgbGF0ZXIgb24gJSB3aXRoIHNpemUgb3BlcmF0aW9uIGNvdWxkIGJlIHJlcGxhY2VkCisgICAg ICAgIC8vIGJ5IHNpbXBsZSBiaXR3aXNlICYKKyAgICAgICAgaW50IGxlYWRpbmdaZXJvcyA9IHNp emUgPiAxID8gX19idWlsdGluX2NseihzaXplIC0gMSkgOiBfX2J1aWx0aW5fY2x6KHNpemUpOwor ICAgICAgICBzaXplID0gMVUgPDwgKChzaXplb2YodW5zaWduZWQpIDw8IDMpICAtIGxlYWRpbmda ZXJvcyk7CisKICAgICAgICAgSGFzaHRhYmxlKiByZXN1bHQgPSBzdGF0aWNfY2FzdDxIYXNodGFi bGUqPigKICAgICAgICAgICAgIGZhc3RaZXJvZWRNYWxsb2Moc2l6ZW9mKEhhc2h0YWJsZSkgKyBz aXplb2YoQXRvbWljPEJ1Y2tldCo+KSAqIChzaXplIC0gMSkpKTsKICAgICAgICAgcmVzdWx0LT5z aXplID0gc2l6ZTsKQEAgLTM4NCw3ICszODgsNyBAQCB2b2lkIGVuc3VyZUhhc2h0YWJsZVNpemUo dW5zaWduZWQgbnVtVGhyCiAgICAgICAgIGlmICh2ZXJib3NlKQogICAgICAgICAgICAgZGF0YUxv Zyh0b1N0cmluZyhjdXJyZW50VGhyZWFkKCksICI6IHJlaGFzaGluZyB0aHJlYWQgZGF0YSAiLCBS YXdQb2ludGVyKHRocmVhZERhdGEpLCAiIHdpdGggYWRkcmVzcyA9ICIsIFJhd1BvaW50ZXIodGhy ZWFkRGF0YS0+YWRkcmVzcyksICJcbiIpKTsKICAgICAgICAgdW5zaWduZWQgaGFzaCA9IGhhc2hB ZGRyZXNzKHRocmVhZERhdGEtPmFkZHJlc3MpOwotICAgICAgICB1bnNpZ25lZCBpbmRleCA9IGhh c2ggJSBuZXdIYXNodGFibGUtPnNpemU7CisgICAgICAgIHVuc2lnbmVkIGluZGV4ID0gaGFzaCAm IChuZXdIYXNodGFibGUtPnNpemUgLSAxKTsKICAgICAgICAgaWYgKHZlcmJvc2UpCiAgICAgICAg ICAgICBkYXRhTG9nKHRvU3RyaW5nKGN1cnJlbnRUaHJlYWQoKSwgIjogaW5kZXggPSAiLCBpbmRl eCwgIlxuIikpOwogICAgICAgICBCdWNrZXQqIGJ1Y2tldCA9IG5ld0hhc2h0YWJsZS0+ZGF0YVtp bmRleF0ubG9hZCgpOwpAQCAtNDY1LDcgKzQ2OSw3IEBAIGJvb2wgZW5xdWV1ZShjb25zdCB2b2lk KiBhZGRyZXNzLCBjb25zdCAKIAogICAgIGZvciAoOzspIHsKICAgICAgICAgSGFzaHRhYmxlKiBt eUhhc2h0YWJsZSA9IGVuc3VyZUhhc2h0YWJsZSgpOwotICAgICAgICB1bnNpZ25lZCBpbmRleCA9 IGhhc2ggJSBteUhhc2h0YWJsZS0+c2l6ZTsKKyAgICAgICAgdW5zaWduZWQgaW5kZXggPSBoYXNo ICYgKG15SGFzaHRhYmxlLT5zaXplIC0gMSk7CiAgICAgICAgIEF0b21pYzxCdWNrZXQqPiYgYnVj a2V0UG9pbnRlciA9IG15SGFzaHRhYmxlLT5kYXRhW2luZGV4XTsKICAgICAgICAgQnVja2V0KiBi dWNrZXQ7CiAgICAgICAgIGZvciAoOzspIHsKQEAgLTUxNyw3ICs1MjEsNyBAQCBib29sIGRlcXVl dWUoCiAKICAgICBmb3IgKDs7KSB7CiAgICAgICAgIEhhc2h0YWJsZSogbXlIYXNodGFibGUgPSBl bnN1cmVIYXNodGFibGUoKTsKLSAgICAgICAgdW5zaWduZWQgaW5kZXggPSBoYXNoICUgbXlIYXNo dGFibGUtPnNpemU7CisgICAgICAgIHVuc2lnbmVkIGluZGV4ID0gaGFzaCAmIChteUhhc2h0YWJs ZS0+c2l6ZSAtIDEpOwogICAgICAgICBBdG9taWM8QnVja2V0Kj4mIGJ1Y2tldFBvaW50ZXIgPSBt eUhhc2h0YWJsZS0+ZGF0YVtpbmRleF07CiAgICAgICAgIEJ1Y2tldCogYnVja2V0ID0gYnVja2V0 UG9pbnRlci5sb2FkKCk7CiAgICAgICAgIGlmICghYnVja2V0KSB7Cg==