140858 2015-01-24 11:25:22 -0800 Provide more efficient implementation for WTF::DefaultHash<bool> 2015-01-30 18:45:58 -0800 1 1 1 Unclassified WebKit Web Template Framework 528+ (Nightly build) Unspecified Unspecified RESOLVED INVALID P2 Normal --- 140848 1 cdumez cdumez andersca benjamin cmarcelo commit-queue darin koivisto oldest_to_newest 1063929 0 cdumez 2015-01-24 11:25:22 -0800 Provide more efficient implementation for WTF::DefaultHash<bool>. Right now, it merely calls intHash(uint8_t) which is a bit wasteful. 1063930 1 245287 cdumez 2015-01-24 11:27:56 -0800 Created attachment 245287 Patch 1063931 2 commit-queue 2015-01-24 11:29:41 -0800 Attachment 245287 did not pass style-queue: ERROR: Source/WTF/wtf/HashFunctions.h:182: More than one command on the same line [whitespace/newline] [4] Total errors found: 1 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style. 1063935 3 245287 darin 2015-01-24 15:06:38 -0800 Comment on attachment 245287 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=245287&action=review Not sure this optimization is important. But this patch doesn’t successfully apply it. > Source/WTF/wtf/HashFunctions.h:41 > + return key1 ? 0x62baf5a0 : 0x4636b9c9; WebKit mostly uses uppercase hex; this is lowercase. How did you choose the branching version of this over the arithmetic version? > Source/WTF/wtf/HashFunctions.h:182 > + template<> struct DefaultHash<bool> { typedef IntHash<bool> Hash; }; This is not enough to get intHash(bool) called. The problem is that the IntHash struct template casts to IntTypes<sizeof(T)>::UnsignedType, so it’s going to choose some kind of integer type based on sizeof(bool). That’s not what we want. I think we’ll need: struct BoolHash { static unsigned hash(bool key) { return intHash(key); } static bool equal(bool a, bool b) { return a == b; } static const bool safeToCompareToEmptyOrDeleted = true; }; You can test by setting a breakpoint in a debug build and seeing the new function isn’t called. 1063936 4 245287 cdumez 2015-01-24 15:15:50 -0800 Comment on attachment 245287 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=245287&action=review I am not sure this optimization is very important either, but since you commented, I figured I would write a quick patch. >> Source/WTF/wtf/HashFunctions.h:41 >> + return key1 ? 0x62baf5a0 : 0x4636b9c9; > > WebKit mostly uses uppercase hex; this is lowercase. How did you choose the branching version of this over the arithmetic version? I only considered simplicity / readability. I haven't actually measured if one version is faster than the others. Wouldn't be difficult to benchmark those though if we want. >> Source/WTF/wtf/HashFunctions.h:182 >> + template<> struct DefaultHash<bool> { typedef IntHash<bool> Hash; }; > > This is not enough to get intHash(bool) called. The problem is that the IntHash struct template casts to IntTypes<sizeof(T)>::UnsignedType, so it’s going to choose some kind of integer type based on sizeof(bool). That’s not what we want. I think we’ll need: > > struct BoolHash { > static unsigned hash(bool key) { return intHash(key); } > static bool equal(bool a, bool b) { return a == b; } > static const bool safeToCompareToEmptyOrDeleted = true; > }; > > You can test by setting a breakpoint in a debug build and seeing the new function isn’t called. Right, sorry about that. I didn't see IntHash was using typename IntTypes<sizeof(T)>::UnsignedType. 245287 2015-01-24 11:27:56 -0800 2015-01-24 15:06:38 -0800 Patch bug-140858-20150124112757.patch text/plain 2156 cdumez U3VidmVyc2lvbiBSZXZpc2lvbjogMTc5MDYxCmRpZmYgLS1naXQgYS9Tb3VyY2UvV1RGL0NoYW5n ZUxvZyBiL1NvdXJjZS9XVEYvQ2hhbmdlTG9nCmluZGV4IGY4Yjk1OTcyOTgyMzliMmNmODYyYzU2 MjZhMzFmMjJlZjg4NDYzNzMuLjUwZmUwZjgyZWI0NzE3N2IwNWU1NTNiNDc3ODA4NDFhYTYyODZm MWQgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XVEYvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XVEYvQ2hh bmdlTG9nCkBAIC0xLDUgKzEsMjEgQEAKIDIwMTUtMDEtMjQgIENocmlzIER1bWV6ICA8Y2R1bWV6 QGFwcGxlLmNvbT4KIAorICAgICAgICBQcm92aWRlIG1vcmUgZWZmaWNpZW50IGltcGxlbWVudGF0 aW9uIGZvciBXVEY6OkRlZmF1bHRIYXNoPGJvb2w+CisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJr aXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xNDA4NTgKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JP RFkgKE9PUFMhKS4KKworICAgICAgICBQcm92aWRlIG1vcmUgZWZmaWNpZW50IGltcGxlbWVudGF0 aW9uIGZvciBXVEY6OkRlZmF1bHRIYXNoPGJvb2w+LgorICAgICAgICBQcmV2aW91c2x5LCBpdCB3 YXMgbWVyZWx5IGNhbGxpbmcgaW50SGFzaCh1aW50OF90KSB3aGljaCB3YXMgYSBiaXQKKyAgICAg ICAgd2FzdGVmdWwuIFRoZSBuZXcgaW50SGFzaChib29sKSByZXR1cm5zIHRoZSBzYW1lIHJlc3Vs dCBhcworICAgICAgICBpbnRIYXNoKHVpbnQ4X3QpIGZvciBib29sZWFuIGlucHV0IHZhbHVlcyBi dXQgd2l0aCBhIGxvdCBsZXNzCisgICAgICAgIG1hbmlwdWxhdGlvbnMuCisKKyAgICAgICAgKiB3 dGYvSGFzaEZ1bmN0aW9ucy5oOgorICAgICAgICAoV1RGOjppbnRIYXNoKToKKworMjAxNS0wMS0y NCAgQ2hyaXMgRHVtZXogIDxjZHVtZXpAYXBwbGUuY29tPgorCiAgICAgICAgIFByb3ZpZGUgaW1w bGVtZW50YXRpb24gZm9yIFdURjo6RGVmYXVsdEhhc2g8Ym9vbD4KICAgICAgICAgaHR0cHM6Ly9i dWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTE0MDg0OAogCmRpZmYgLS1naXQgYS9Tb3Vy Y2UvV1RGL3d0Zi9IYXNoRnVuY3Rpb25zLmggYi9Tb3VyY2UvV1RGL3d0Zi9IYXNoRnVuY3Rpb25z LmgKaW5kZXggYzQwOTdhZGFmZDQ4ZmM4NDkyMjRlYzRhZjg1OTBmN2MwNDUwNjMwNi4uZDlkZDhj ZDAxOTBjNzI2MjdlOWY5MzZmYTEyMDU3M2EyYTg2MGNkYyAxMDA2NDQKLS0tIGEvU291cmNlL1dU Ri93dGYvSGFzaEZ1bmN0aW9ucy5oCisrKyBiL1NvdXJjZS9XVEYvd3RmL0hhc2hGdW5jdGlvbnMu aApAQCAtMzUsNiArMzUsMTIgQEAgbmFtZXNwYWNlIFdURiB7CiAKICAgICAvLyBpbnRlZ2VyIGhh c2ggZnVuY3Rpb24KIAorICAgIC8vIFNpbXBsaWZpZWQgdmVyc2lvbiBvZiBpbnRIYXNoIGZvciBi b29sZWFuIGlucHV0LCByZXR1cm5pbmcgdGhlIHNhbWUgcmVzdWx0LgorICAgIGlubGluZSB1bnNp Z25lZCBpbnRIYXNoKGJvb2wga2V5MSkKKyAgICB7CisgICAgICAgIHJldHVybiBrZXkxID8gMHg2 MmJhZjVhMCA6IDB4NDYzNmI5Yzk7CisgICAgfQorCiAgICAgLy8gVGhvbWFzIFdhbmcncyAzMiBC aXQgTWl4IEZ1bmN0aW9uOiBodHRwOi8vd3d3LmNyaXMuY29tL35UdHdhbmcvdGVjaC9pbnRoYXNo Lmh0bQogICAgIGlubGluZSB1bnNpZ25lZCBpbnRIYXNoKHVpbnQ4X3Qga2V5OCkKICAgICB7CkBA IC0xNzMsNyArMTc5LDcgQEAgbmFtZXNwYWNlIFdURiB7CiAKICAgICAvLyBtYWtlIEludEhhc2gg dGhlIGRlZmF1bHQgaGFzaCBmdW5jdGlvbiBmb3IgbWFueSBpbnRlZ2VyIHR5cGVzCiAKLSAgICB0 ZW1wbGF0ZTw+IHN0cnVjdCBEZWZhdWx0SGFzaDxib29sPiB7IHR5cGVkZWYgSW50SGFzaDx1aW50 OF90PiBIYXNoOyB9OworICAgIHRlbXBsYXRlPD4gc3RydWN0IERlZmF1bHRIYXNoPGJvb2w+IHsg dHlwZWRlZiBJbnRIYXNoPGJvb2w+IEhhc2g7IH07CiAgICAgdGVtcGxhdGU8PiBzdHJ1Y3QgRGVm YXVsdEhhc2g8c2hvcnQ+IHsgdHlwZWRlZiBJbnRIYXNoPHVuc2lnbmVkPiBIYXNoOyB9OwogICAg IHRlbXBsYXRlPD4gc3RydWN0IERlZmF1bHRIYXNoPHVuc2lnbmVkIHNob3J0PiB7IHR5cGVkZWYg SW50SGFzaDx1bnNpZ25lZD4gSGFzaDsgfTsKICAgICB0ZW1wbGF0ZTw+IHN0cnVjdCBEZWZhdWx0 SGFzaDxpbnQ+IHsgdHlwZWRlZiBJbnRIYXNoPHVuc2lnbmVkPiBIYXNoOyB9Owo=