Fix Disambiguator- and IdentityMap hashing #682
Conversation
✅ Deploy Preview for salsa-rs canceled.
|
CodSpeed Performance ReportMerging #682 will not alter performanceComparing Summary
|
Veykril
force-pushed
the
veykril/push-vntqlkpyzmqm
branch
3 times, most recently
from
3179952 to
e677179
Compare
| .collect(), | ||
| ); | ||
| let trackeds = batch(&db, inputs); | ||
| for (id, tracked) in trackeds.into_iter().enumerate() { |
There was a problem hiding this comment.
I don't think this test exercises the right code path. We need at lesst two revisions, were both construct roughly the same tracked stucts because the identity map isn't used in a single revision (and we then just end up disambiguating too often)
It might be easier to write a unit test for each map
There was a problem hiding this comment.
I was thinking of writing a unit test, but then we'd still not actually test that disambiguating works as expected (as no test actual hits the disambiguator). I guess the test here + a unit test should cover things though.
Veykril
force-pushed
the
veykril/push-vntqlkpyzmqm
branch
from
e677179 to
547acab
Compare
It is rather sparse but this was entirely my mistake, the hash is part of the identity/eq of our struct here so it doesn't make sense to leave it out when comparing |
Veykril
force-pushed
the
veykril/push-vntqlkpyzmqm
branch
from
547acab to
e725854
Compare
As was raised by @ibraheemdev in #647, the hashmap may consider slots/buckets that do not uniquely correspond to a single hash so the hash itself is required for equality in our case.
Added a test that exerts the disambiguator mapping as we have no test that actually hits it, though even without the changes here that test won't fail. I am not quite sure what a setup for that would be.