[RUN-1730] hashtable: make it ordered #616

oridb · 2023-05-02T02:58:45Z

https://linear.app/replay/issue/RUN-1730/make-all-chromium-hashset-and-hashmap-iteration-order-deterministic

Domiii

Leaving a few comments. Will leave approval to Brian and/or others.

Domiii · 2023-05-02T14:23:27Z

third_party/blink/renderer/platform/wtf/hash_table.h

  }
  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_);


I think, it would be better to either use CHECK_NE or we could create a new set of REPLAY_CHECK defines which simply point to their CHECK counterparts to avoid unnecessary code changes.

Domiii · 2023-05-02T14:23:36Z

third_party/blink/renderer/platform/wtf/hash_table.h

@@ -376,7 +378,8 @@ class HashTableConstIterator final {

  const_iterator& operator--() {
 #if DCHECK_IS_ON()
-    DCHECK_NE(position_, begin_position_);
+    assert(position_ != end_position_);


Ditto CHECK_NE

Domiii · 2023-05-02T14:25:16Z

third_party/blink/renderer/platform/wtf/hash_table.h

@@ -1103,6 +1129,55 @@ HashTable<Key, Value, Extractor, HashFunctions, Traits, KeyTraits, Allocator>::
      const_cast<const HashTable*>(this)->Lookup<HashTranslator>(key));
 }

+template <typename Key,


Did you write this yourself?
Do you have a reference you can point to for this or can you explain it?

Looks legit to me. It's a template method specialization on a templated class.

Domiii · 2023-05-02T14:26:02Z

third_party/blink/renderer/platform/wtf/hash_table.h

@@ -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)){


space before {

Domiii · 2023-05-02T14:27:30Z

third_party/blink/renderer/platform/wtf/hash_table.h


      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)){


missing space

Domiii · 2023-05-02T14:30:31Z

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?

The branch name is missing a dash so Linear probably won't pick up the PR. Can fix by adding the correct id into the title.
GitHub won't ever auto-link to Linear, so we always provide the link manually in the description.

kannanvijayan

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?

kannanvijayan · 2023-05-02T16:05:20Z

third_party/blink/renderer/platform/wtf/hash_table.h

+inline ssize_t
+HashTable<Key, Value, Extractor, HashFunctions, Traits, KeyTraits, Allocator>::
+    LookupIdx(const T& key) const {
+  DCHECK(!AccessForbidden());


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.

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.

Please also see my other comment regarding DCHECK here

kannanvijayan

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.

kannanvijayan · 2023-05-10T17:28:58Z

third_party/blink/renderer/platform/wtf/hash_table.h

@@ -1103,6 +1129,55 @@ HashTable<Key, Value, Extractor, HashFunctions, Traits, KeyTraits, Allocator>::
      const_cast<const HashTable*>(this)->Lookup<HashTranslator>(key));
 }

+template <typename Key,


Looks legit to me. It's a template method specialization on a templated class.

kannanvijayan · 2023-05-10T20:23:21Z

third_party/blink/renderer/platform/wtf/hash_table.h

@@ -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_);


Nit: stale comments to remove.

…able-hash-order

oridb marked this pull request as draft May 2, 2023 14:03

oridb requested review from kannanvijayan, klochek and Domiii May 2, 2023 14:03

Domiii reviewed May 2, 2023

View reviewed changes

kannanvijayan approved these changes May 2, 2023

View reviewed changes

hashtable: make it ordered

e5de1a7

oridb force-pushed the run1730-stable-hash-order branch from 6755139 to e5de1a7 Compare May 4, 2023 17:05

oridb added 2 commits May 4, 2023 16:01

fixes

33207f5

Move back to vector based backing

364ffb0

oridb force-pushed the run1730-stable-hash-order branch from 182a3ce to 364ffb0 Compare May 10, 2023 03:15

fix hashset swap

517e871

oridb changed the title ~~[DRAFT] hashtable: make it ordered~~ [RUN-1730] hashtable: make it ordered May 10, 2023

oridb added 3 commits May 10, 2023 11:00

compress the ordering vector if it grows too much

01e56d3

revert test changes

fb32195

missed revert

39e0b2d

kannanvijayan approved these changes May 10, 2023

View reviewed changes

oridb and others added 3 commits May 11, 2023 12:32

revert to gc'ed arrays

c322427

fix some missing count copies

d002b51

Merge branch 'master' of github.com:replayio/chromium into run1730-st…

e1bdc6d

…able-hash-order

kannanvijayan mentioned this pull request Jun 12, 2023

Order iteration over all chromium hash tables. #717

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RUN-1730] hashtable: make it ordered #616

[RUN-1730] hashtable: make it ordered #616

oridb commented May 2, 2023 •

edited

Loading

Domiii left a comment •

edited

Loading

Domiii May 2, 2023

Domiii May 2, 2023

Domiii May 2, 2023

kannanvijayan May 10, 2023

Domiii May 2, 2023

Domiii May 2, 2023

Domiii commented May 2, 2023 •

edited

Loading

kannanvijayan left a comment

kannanvijayan May 2, 2023

oridb May 2, 2023

Domiii May 2, 2023

kannanvijayan left a comment

kannanvijayan May 10, 2023

kannanvijayan May 10, 2023

[RUN-1730] hashtable: make it ordered #616

Are you sure you want to change the base?

[RUN-1730] hashtable: make it ordered #616

Conversation

oridb commented May 2, 2023 • edited Loading

Domiii left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Domiii commented May 2, 2023 • edited Loading

kannanvijayan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kannanvijayan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oridb commented May 2, 2023 •

edited

Loading

Domiii left a comment •

edited

Loading

Domiii commented May 2, 2023 •

edited

Loading