Make HTTPFields hashing consistent with equality - #143
Conversation
nerdsupremacist
left a comment
nerdsupremacist
left a comment
There was a problem hiding this comment.
Thank you for fixing this. Please check if you can simplify the part where we're combining the hash results from all the field names
| var entryHasher = Hasher() | ||
| entryHasher.combine(name) | ||
| entryHasher.combine(nameHasher.finalize()) |
There was a problem hiding this comment.
Each field contains the name already. So the name is already included when we hash the field itself. I think you can just avoid this intermediate hasher here and use the nameHashers result directly
| // Equality ignores the order of differently named fields, so hashing | ||
| // must too. Combine each name's sequence (order of same-named fields | ||
| // still matters), then mix those group hashes commutatively. | ||
| var grouped = [String: Hasher]() |
There was a problem hiding this comment.
nit: I wonder if there's a way to perform this change without a dictionary.
The dictionary will cause extra allocations, which might not be the fastest when doing basic hashing
That being said, I don't know off the top of my head, all the contexts in which we hash this struct and how critical performance is. So happy to continue with this implementation and we look for a performance improvement later. Correctness is the top priority
There was a problem hiding this comment.
we should at least reserve enough capacity here.
There was a problem hiding this comment.
Reserved grouped.reserveCapacity(_fields.count) so the hasher dictionary does not reallocate as names are inserted.
| let key = field.name.canonicalName | ||
| var nameHasher = grouped[key] ?? Hasher() | ||
| nameHasher.combine(field) | ||
| grouped[key] = nameHasher |
There was a problem hiding this comment.
nit: can we use default here?
| let key = field.name.canonicalName | |
| var nameHasher = grouped[key] ?? Hasher() | |
| nameHasher.combine(field) | |
| grouped[key] = nameHasher | |
| let key = field.name.canonicalName | |
| grouped[key, default: Hasher()].combine(field) |
|
Simplified the hasher as suggested. Each field already includes its name, so the extra per entry hasher is gone, and grouping uses the default Hasher subscript. Left the dictionary in place for correctness; happy to follow up on allocations if hashing this type shows up hot. |
7085c1a to
f58a651
Compare
3 participants
Summary
HTTPFields equality does not depend on the order of differently named fields. Hashing used storage order, so equal values could hash differently.
This hashes each name group in relative order, then mixes those group hashes without depending on name order. The existing hash test now asserts equal hashes.
Fixes #139
Test plan
Run the package tests on macOS. Confirm hashMatchesEqualityForDifferentOrder passes without a known issue wrapper.