-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[RUN-1730] hashtable: make it ordered #616
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leaving a few comments. Will leave approval to Brian and/or others.
} | ||
typename Traits::IteratorConstReferenceType operator*() const { | ||
return Traits::GetToReferenceConstConversion(Get()); | ||
} | ||
GetType operator->() const { return Get(); } | ||
|
||
const_iterator& operator++() { | ||
DCHECK_NE(position_, end_position_); | ||
assert(position_ != end_position_); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think, it would be better to either use CHECK_NE
or we could create a new set of REPLAY_CHECK
define
s which simply point to their CHECK
counterparts to avoid unnecessary code changes.
@@ -376,7 +378,8 @@ class HashTableConstIterator final { | |||
|
|||
const_iterator& operator--() { | |||
#if DCHECK_IS_ON() | |||
DCHECK_NE(position_, begin_position_); | |||
assert(position_ != end_position_); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto CHECK_NE
@@ -1103,6 +1129,55 @@ HashTable<Key, Value, Extractor, HashFunctions, Traits, KeyTraits, Allocator>:: | |||
const_cast<const HashTable*>(this)->Lookup<HashTranslator>(key)); | |||
} | |||
|
|||
template <typename Key, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you write this yourself?
Do you have a reference you can point to for this or can you explain it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks legit to me. It's a template method specialization on a templated class.
@@ -1383,22 +1458,29 @@ HashTable<Key, Value, Extractor, HashFunctions, Traits, KeyTraits, Allocator>:: | |||
break; | |||
|
|||
if (HashFunctions::safe_to_compare_to_empty_or_deleted) { | |||
if (HashTranslator::Equal(Extractor::Extract(*entry), key)) | |||
if (HashTranslator::Equal(Extractor::Extract(*entry), key)){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
space before {
|
||
if (IsDeletedBucket(*entry) && can_reuse_deleted_entry) | ||
deleted_entry = entry; | ||
} else { | ||
if (IsDeletedBucket(*entry) && can_reuse_deleted_entry) | ||
deleted_entry = entry; | ||
else if (HashTranslator::Equal(Extractor::Extract(*entry), key)) | ||
else if (HashTranslator::Equal(Extractor::Extract(*entry), key)){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing space
One more thing: Can you add linkage from https://linear.app/replay/issue/RUN-1730/make-all-chromium-hashset-and-hashmap-iteration-order-deterministic to here and vice versa?
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks reasonable overall, but I didn't rigorously check the logic or anything.
Only question I have is how this works during rehashing? Does it come automatically because the resized hashtable is inserted into using the existing routines, and those are already modified to build the ordering vectors?
inline ssize_t | ||
HashTable<Key, Value, Extractor, HashFunctions, Traits, KeyTraits, Allocator>:: | ||
LookupIdx(const T& key) const { | ||
DCHECK(!AccessForbidden()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DCHECK compiles to no-op in our builds, so you may want to use something better for this if you want it to be actually checked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
During rehashing, we build a new hash table, and then steal its guts, replacing the old table.
There's a comment in there that worries me, about how this works with the concurrent collector. I need to go find the code that scans the table, and make sure it only scans the table_ member before this can land.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also see my other comment regarding DCHECK
here
6755139
to
e5de1a7
Compare
182a3ce
to
364ffb0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good mostly. But see notes about using CHECK
instead of DCHECK
. It's probably good to fail fast more often just to be safe with this code at the start. I don't think the perf-impact of it would be huge.
@@ -1103,6 +1129,55 @@ HashTable<Key, Value, Extractor, HashFunctions, Traits, KeyTraits, Allocator>:: | |||
const_cast<const HashTable*>(this)->Lookup<HashTranslator>(key)); | |||
} | |||
|
|||
template <typename Key, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks legit to me. It's a template method specialization on a templated class.
@@ -1870,16 +1973,22 @@ HashTable<Key, Value, Extractor, HashFunctions, Traits, KeyTraits, Allocator>:: | |||
Allocator::TraceBackingStoreIfMarked(new_hash_table.table_); | |||
|
|||
ValueType* old_table = table_; | |||
unsigned old_table_size = table_size_; | |||
auto old_table_size = table_size_; | |||
//auto old_table_order = std::move(idxorder_); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: stale comments to remove.
https://linear.app/replay/issue/RUN-1730/make-all-chromium-hashset-and-hashmap-iteration-order-deterministic