Using std maps in datagen
#2152
Conversation
robertbastian
force-pushed
the
hashmap
branch
2 times, most recently
from
455736e
to
c276f4e
Compare
robertbastian
requested review from
a team,
sffc,
Manishearth,
zbraniecki,
gregtatum and
nordzilla
as code owners
robertbastian
removed request for
a team,
zbraniecki,
sffc,
gregtatum,
Manishearth and
nordzilla
| #[allow(clippy::derive_hash_xor_eq)] | ||
|
|
||
| // Custom implementation to avoid hashing the path. | ||
| impl core::hash::Hash for ResourceKey { |
There was a problem hiding this comment.
Issue: I feel that ResourceKeyHash should be used instead of ResourceKey when a hash is needed, which is why there isn't already an impl Hash for ResourceKey.
There was a problem hiding this comment.
But ResourceKeyHash doesn't have the path, which I need for databake. This is like saying you should store f(x) is a hash set instead of x.
There was a problem hiding this comment.
I cannot approve this PR given that my PR #2115 conflicts with the Hash impl. We can't merge both.
There was a problem hiding this comment.
What's the conflict exactly?
There was a problem hiding this comment.
We need to decide on what to do with the ResourceKeyMetadata.
- Does it affect PartialEq? (then it needs to go into Hash)
- Is it ignored for PartialEq? (then match statements get less efficient)
- Does it go into the path string itself? (then it gets into Hash and PartialEq automatically)
I don't want to lock ourselves into having a Hash impl if the impl would be inefficient.
tl;dr, let's reach consensus on #2115 and then we can move on.
There was a problem hiding this comment.
This is independent of #2115. Hashing on hash (aka path) only is valid, even if PartialEq compares more fields. Hash is free to produce collisions as long as it produces equal hashes for PartialEq values. Hash collisions don't affect correctness, they're a performance problem, but that doesn't matter here because a) it's only used in datagen and b) we don't have keys that differ only by metadata anyway.
|
I think the reason was in part to get rid of Ord requirements |
|
One of the reasons you mention in #1341 was that we can add methods to The only valid point in #1341 imho is keeping |
Currently we're using
LiteMapwith no clear benefit but several drawbacks:HashMaporBTreeMapOrdwhereHashwould doHelloWorldDataProvidercan just use a static slice and doesn't have to allocate at all.