Further sequester `Group`/`Tag` code #568

clarfonthey · 2024-10-06T01:31:26Z

Was originally going to make this part of a larger change, but decided this bit is enough for its own PR.

Essentially, this completely moves the Group and Tag code outside of the raw module into a separate control module. This module will also eventually be the future home for stuff like RawIterHash and ProbeSeq, but I decided that that would be better as a separate PR, since moving and modifying the files makes it harder to review.

clarfonthey · 2024-10-06T01:33:15Z

src/control/group/mod.rs

@@ -0,0 +1,35 @@
+cfg_if! {


This is just copied directly from the raw module, without any additional changes.

clarfonthey · 2024-10-06T01:34:47Z

src/control/tag.rs

+/// Extension trait for slices of tags.
+pub(crate) trait TagSliceExt {
+    /// Fills the control with the given tag.
+    fn fill_tag(&mut self, tag: Tag);
+
+    /// Clears out the control.
+    fn fill_empty(&mut self) {
+        self.fill_tag(Tag::EMPTY)
+    }
+}
+impl TagSliceExt for [Tag] {
+    fn fill_tag(&mut self, tag: Tag) {
+        // SAFETY: We have access to the entire slice, so, we can write to the entire slice.
+        unsafe { self.as_mut_ptr().write_bytes(tag.0, self.len()) }
+    }
+}


This is new code.

The TagSliceExt was a mostly impulse decision to be able to do slice.fill_empty() instead of Tag::EMPTY.fill(slice), and since it's solely an internal API anyway it isn't a big deal. The most import part is that this lets us make the inside of a tag private relative to the raw module.

We're not able to just do slice.fill(Tag::EMPTY) and have it optimise into memset by itself, which is why this exists.

clarfonthey · 2024-10-06T01:36:00Z

src/control/tag.rs

+impl fmt::Debug for Tag {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        if self.is_special() {
+            if self.special_is_empty() {
+                f.pad("EMPTY")
+            } else {
+                f.pad("DELETED")
+            }
+        } else {
+            f.debug_tuple("full").field(&(self.0 & 0x7F)).finish()
+        }
+    }
+}


I added this to replace the derived debug to help me debug the code I'm going to add in a future PR, so it's a bit easier to read. I figure that if we care about the additional compile time added by this one debug impl so much we can cfg(debug_assertions) gate it later.

This isn't a problem for compilation time since it's not a generic function.

clarfonthey · 2024-10-06T01:37:20Z

src/util.rs

@@ -0,0 +1,14 @@
+// FIXME: Branch prediction hint. This is currently only available on nightly


I wasn't feeling particularly creative and will need this in the control module too in the future, so, I decided to separate them into their own module.

Maybe nightly would be a better name, but I don't think it matters that much.

It's actually broken at the moment on nightly until rust-lang/rust#120370 is merged, so it doesn't even do anything. But it's probably fine to keep for now as a reminder to fix this later.

bors · 2024-10-14T10:36:50Z

☔ The latest upstream changes (presumably #572) made this pull request unmergeable. Please resolve the merge conflicts.

clarfonthey · 2024-10-15T00:30:28Z

src/control/bitmask.rs

@@ -102,7 +102,7 @@ impl IntoIterator for BitMask {

 /// Iterator over the contents of a `BitMask`, returning the indices of set
 /// bits.
-#[derive(Copy, Clone)]
+#[derive(Clone)]


Iterators in general shouldn't implement Copy, for better or worse.

clarfonthey · 2024-10-15T00:35:38Z

So, I figured out why my probing logic wasn't working: it wasn't because it wasn't working, but because my debug-assert checking that it was working wasn't working, and giving me false positives. So, #578 exists to demonstrate the fuller version of the control module so you can get an idea where I'm coming from with this change, even though this should be merged first.

Amanieu · 2024-11-06T22:47:51Z

src/control/tag.rs

+    }
+}
+impl TagSliceExt for [Tag] {
+    fn fill_tag(&mut self, tag: Tag) {


This needs #[inline].

Amanieu · 2024-11-06T22:47:54Z

src/control/tag.rs

+    fn fill_tag(&mut self, tag: Tag);
+
+    /// Clears out the control.
+    fn fill_empty(&mut self) {


This needs #[inline].

Amanieu · 2024-11-06T22:48:25Z

src/control/tag.rs

+impl fmt::Debug for Tag {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        if self.is_special() {
+            if self.special_is_empty() {
+                f.pad("EMPTY")
+            } else {
+                f.pad("DELETED")
+            }
+        } else {
+            f.debug_tuple("full").field(&(self.0 & 0x7F)).finish()
+        }
+    }
+}


This isn't a problem for compilation time since it's not a generic function.

Amanieu · 2024-11-06T22:50:16Z

src/util.rs

@@ -0,0 +1,14 @@
+// FIXME: Branch prediction hint. This is currently only available on nightly


It's actually broken at the moment on nightly until rust-lang/rust#120370 is merged, so it doesn't even do anything. But it's probably fine to keep for now as a reminder to fix this later.

clarfonthey commented Oct 6, 2024

View reviewed changes

clarfonthey force-pushed the control-module-p1 branch 3 times, most recently from 3df96dd to 11f2cf9 Compare October 15, 2024 00:29

clarfonthey commented Oct 15, 2024

View reviewed changes

clarfonthey force-pushed the control-module-p1 branch from 11f2cf9 to f7aa64a Compare October 15, 2024 00:31

clarfonthey mentioned this pull request Oct 15, 2024

Move probing logic into control module #578

Draft

Amanieu reviewed Nov 6, 2024

View reviewed changes

clarfonthey added 2 commits November 6, 2024 18:45

Further sequester Group/Tag code

cc0947a

Add inlines per feedback

4f75078

clarfonthey force-pushed the control-module-p1 branch from f7aa64a to 4f75078 Compare November 6, 2024 23:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Further sequester `Group`/`Tag` code #568

Further sequester `Group`/`Tag` code #568

clarfonthey commented Oct 6, 2024

clarfonthey Oct 6, 2024

clarfonthey Oct 6, 2024 •

edited

Loading

clarfonthey Oct 6, 2024 •

edited

Loading

Amanieu Nov 6, 2024

clarfonthey Oct 6, 2024

Amanieu Nov 6, 2024

bors commented Oct 14, 2024

clarfonthey Oct 15, 2024

clarfonthey commented Oct 15, 2024

Amanieu Nov 6, 2024

Amanieu Nov 6, 2024

Amanieu Nov 6, 2024

Amanieu Nov 6, 2024

		@@ -0,0 +1,14 @@
		// FIXME: Branch prediction hint. This is currently only available on nightly

Further sequester Group/Tag code #568

Are you sure you want to change the base?

Further sequester Group/Tag code #568

Conversation

clarfonthey commented Oct 6, 2024

Choose a reason for hiding this comment

clarfonthey Oct 6, 2024 • edited Loading

Choose a reason for hiding this comment

clarfonthey Oct 6, 2024 • 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

bors commented Oct 14, 2024

Choose a reason for hiding this comment

clarfonthey commented Oct 15, 2024

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

Further sequester `Group`/`Tag` code #568

Further sequester `Group`/`Tag` code #568

clarfonthey Oct 6, 2024 •

edited

Loading

clarfonthey Oct 6, 2024 •

edited

Loading