[webkit-reviews] review granted: [Bug 207426] [WTF] Use SwissTable algorithm for larger HashTable : [Attachment 390734] Patch

[bugzilla-daemon at webkit.org]

Keith Miller <keith_miller at apple.com> has granted Yusuke Suzuki
<ysuzuki at apple.com>'s request for review:
Bug 207426: [WTF] Use SwissTable algorithm for larger HashTable
https://bugs.webkit.org/show_bug.cgi?id=207426

Attachment 390734: Patch

https://bugs.webkit.org/attachment.cgi?id=390734&action=review




--- Comment #17 from Keith Miller <keith_miller at apple.com> ---
Comment on attachment 390734
  --> https://bugs.webkit.org/attachment.cgi?id=390734
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=390734&action=review

r=me if you fix the style errors and rename h1/h2. Can you make sure we have
tests for SwissHashing in testWTF?

> Source/WTF/ChangeLog:13
> +	   to select Group we will search. The key is that we maintains Control
bytes as a side data,

Nit: maintains => maintain and "a side data" => "side data"

> Source/WTF/ChangeLog:16
> +	   For each bucket, we store either of (1) 7 bit hash code, (2) empty
marker, or (3) deleted marker.

Nit: either => one

> Source/WTF/ChangeLog:18
> +	   operation to match 16 bucket hash codes in parallel, to filter out
search candidate buckets in

Nit: operation => operations. no comma after parallel.

> Source/WTF/ChangeLog:19
> +	   one Group. And we can see last empty bucket if any bucket of Group
is empty. So, almost all cases,

Nit: "So, almost" => "So, in almost"

> Source/WTF/wtf/HashTable.h:622
> +	   ALWAYS_INLINE unsigned h1(unsigned hash)

h1 doesn't really convey the meaning of what we are computing here. Can we call
this groupHash?

> Source/WTF/wtf/HashTable.h:628
> +	   inline Control h2(unsigned hash)

h2 doesn't really convey the meaning of what we are computing here. Can we call
this controlHash?

> Source/WTF/wtf/HashTable.h:1033
> +    inline auto HashTable<Key, Value, Extractor, HashFunctions, Traits,
KeyTraits>::lookupForWriting(const T& key, unsigned h) -> LookupType

Can we change h to hash? It's poor style in the old code and now that it's a
parameter is even worse...