address all review comments

aneubeck · aneubeck · commit 1ecea60155e5 · 2025-03-28T09:44:36.000+01:00
diff --git a/.cargo/config.toml b/.cargo/config.toml
@@ -0,0 +1,14 @@
+# Consider adding "--codegen=link-args=-Wl,--compress-debug-sections=zlib"
+
+[target.x86_64-unknown-linux-gnu]
+# SSE3 is requred by simd-varint.
+# POPCNT makes `count_ones` (which we use in geofilter and bitrank) more efficient.
+rustflags = ["-C", "target-feature=+ssse3,+avx2,+popcnt"]
+
+[target.x86_64-apple-darwin]
+# SSE3 is requred by simd-varint.
+# POPCNT makes `count_ones` (which we use in geofilter and bitrank) more efficient.
+rustflags = ["-C", "target-feature=+ssse3,+avx2,+popcnt"]
+
+[target.aarch64-apple-darwin]
+rustflags = ["-C", "target-feature=+neon"]
diff --git a/crates/string-offsets/src/bitrank.rs b/crates/string-offsets/src/bitrank.rs
@@ -40,9 +40,6 @@ struct Block {
 
 impl Block {
     /// Set a bit without updating `self.sub_blocks`.
-    ///
-    /// This panics if the bit was already set, because that indicates that the original positions
-    /// list is invalid/had duplicates.
     fn set(&mut self, index: usize) {
         debug_assert!(index < BITS_PER_BLOCK);
         let chunk_idx = index / BITS_PER_SUB_BLOCK;
@@ -52,11 +49,7 @@ impl Block {
         self.bits[chunk_idx] |= mask;
     }
 
-    /// The **total rank** of the block relative local index, and the index of the one
-    /// bit that establishes that rank (aka "select") **if** it occurs within that same
-    /// chunk, otherwise ['None'].  The assumption is that if you would have to look back
-    /// through previous chunks it would actually be cheaper to do a lookup in the original
-    /// data structure that the bit vector was created from.
+    /// The **total rank** of the block relative local index.
     fn rank(&self, local_idx: usize) -> usize {
         let mut rank = self.rank as usize;
         let sub_block = local_idx / BITS_PER_SUB_BLOCK;
@@ -65,11 +58,7 @@ impl Block {
         let remainder = local_idx % BITS_PER_SUB_BLOCK;
 
         let last_chunk = local_idx / BITS_PER_SUB_BLOCK;
-        let masked = if remainder == 0 {
-            0
-        } else {
-            self.bits[last_chunk] << (BITS_PER_SUB_BLOCK - remainder)
-        };
+        let masked = self.bits[last_chunk] & !(SubblockBits::MAX << remainder);
         rank + masked.count_ones() as usize
     }
 
@@ -176,42 +165,52 @@ mod tests {
 
     /// Creates a `BitRank` containing the integers in `iter` (which should be strictly
     /// increasing).
-    pub fn bitrank<I: IntoIterator<Item = usize>>(capacity: usize, iter: I) -> BitRank {
-        let mut builder = BitRankBuilder::with_capacity(capacity);
-        for position in iter {
-            builder.push(position);
+    pub fn bitrank<I>(iter: I) -> BitRank
+    where
+        I: IntoIterator<Item = usize>,
+        I::IntoIter: DoubleEndedIterator,
+    {
+        let mut iter = iter.into_iter().rev();
+        if let Some(last) = iter.next() {
+            let mut builder = BitRankBuilder::with_capacity(last + 1);
+            builder.push(last);
+            for position in iter {
+                builder.push(position);
+            }
+            builder.finish()
+        } else {
+            BitRank { blocks: vec![] }
         }
-        builder.finish()
     }
 
     #[test]
     fn test_rank_zero() {
-        let br = bitrank(1, [0]);
+        let br = bitrank([0]);
         assert_eq!(br.rank(0), 0);
         assert_eq!(br.rank(1), 1);
     }
 
     #[test]
     fn test_empty() {
-        let br = bitrank(0, []);
+        let br = bitrank([]);
         assert!(br.blocks.is_empty());
     }
 
     #[test]
     fn test_index_out_of_bounds() {
-        let br = bitrank(BITS_PER_BLOCK, [BITS_PER_BLOCK - 1]);
+        let br = bitrank([BITS_PER_BLOCK - 1]);
         assert_eq!(br.rank(BITS_PER_BLOCK), 1);
     }
 
     #[test]
     #[should_panic]
     fn test_duplicate_position() {
-        bitrank(91, [64, 66, 68, 68, 90]);
+        bitrank([64, 66, 68, 68, 90]);
     }
 
     #[test]
     fn test_rank_exclusive() {
-        let br = bitrank(133, 0..132);
+        let br = bitrank(0..132);
         assert_eq!(br.blocks.len(), 1);
         assert_eq!(br.rank(64), 64);
         assert_eq!(br.rank(132), 132);
@@ -221,37 +220,37 @@ mod tests {
     fn test_rank() {
         let mut positions: Vec<usize> = (0..132).collect();
         positions.append(&mut vec![138usize, 140, 146]);
-        let br = bitrank(146, positions);
+        let br = bitrank(positions);
         assert_eq!(br.rank(135), 132);
 
-        let br2 = bitrank(BITS_PER_BLOCK, 0..BITS_PER_BLOCK - 5);
+        let br2 = bitrank(0..BITS_PER_BLOCK - 5);
         assert_eq!(br2.rank(169), 169);
 
-        let br3 = bitrank(BITS_PER_BLOCK + 6, 0..BITS_PER_BLOCK + 5);
+        let br3 = bitrank(0..BITS_PER_BLOCK + 5);
         assert_eq!(br3.rank(BITS_PER_BLOCK), BITS_PER_BLOCK);
     }
 
     #[test]
     fn test_rank_idx() {
         let mut positions: Vec<usize> = (0..132).collect();
         positions.append(&mut vec![138usize, 140, 146]);
-        let br = bitrank(147, positions);
+        let br = bitrank(positions);
         assert_eq!(br.rank(135), 132);
 
         let bits2: Vec<usize> = (0..BITS_PER_BLOCK - 5).collect();
-        let br2 = bitrank(BITS_PER_BLOCK, bits2);
+        let br2 = bitrank(bits2);
         assert_eq!(br2.rank(169), 169);
 
         let bits3: Vec<usize> = (0..BITS_PER_BLOCK + 5).collect();
-        let br3 = bitrank(BITS_PER_BLOCK + 6, bits3);
+        let br3 = bitrank(bits3);
         assert_eq!(br3.rank(BITS_PER_BLOCK), BITS_PER_BLOCK);
 
         let bits4: Vec<usize> = vec![1, 1000, 7777, BITS_PER_BLOCK + 1];
-        let br4 = bitrank(BITS_PER_BLOCK + 1, bits4);
+        let br4 = bitrank(bits4);
         assert_eq!(br4.rank(8000), 3);
 
         let bits5: Vec<usize> = vec![1, 1000, 7777, BITS_PER_BLOCK + 1];
-        let br5 = bitrank(BITS_PER_BLOCK + 1, bits5);
+        let br5 = bitrank(bits5);
         assert_eq!(br5.rank(BITS_PER_BLOCK), 3);
     }
 
@@ -267,7 +266,7 @@ mod tests {
         // This isn't strictly necessary, given that the bit would just be toggled again, but it
         // ensures that we are meeting the contract.
         random_bits.dedup();
-        let br = bitrank(1_000_000, random_bits.iter().copied());
+        let br = bitrank(random_bits.iter().copied());
         let mut rank = 0;
         for i in 0..random_bits.capacity() {
             assert_eq!(br.rank(i), rank);
@@ -282,7 +281,7 @@ mod tests {
     #[test]
     fn test_rank_out_of_bounds() {
         for i in 1..30 {
-            let br = bitrank(BITS_PER_BLOCK * i, [BITS_PER_BLOCK * i - 1]);
+            let br = bitrank([BITS_PER_BLOCK * i - 1]);
             assert_eq!(br.max_rank(), 1);
             assert_eq!(br.rank(BITS_PER_BLOCK * i - 1), 0);
             for j in 0..10 {
@@ -293,10 +292,7 @@ mod tests {
 
     #[test]
     fn test_large_gap() {
-        let br = bitrank(
-            BITS_PER_BLOCK * 16,
-            (3..4).chain(BITS_PER_BLOCK * 15..BITS_PER_BLOCK * 15 + 17),
-        );
+        let br = bitrank((3..4).chain(BITS_PER_BLOCK * 15..BITS_PER_BLOCK * 15 + 17));
         for i in 1..15 {
             assert_eq!(br.rank(BITS_PER_BLOCK * i), 1);
         }
diff --git a/crates/string-offsets/src/lib.rs b/crates/string-offsets/src/lib.rs
@@ -3,10 +3,10 @@
 //! # Example
 //!
 //! ```
-//! use string_offsets::{StringOffsets, AllConfig};
+//! use string_offsets::StringOffsets;
 //!
 //! let s = "☀️hello\n🗺️world\n";
-//! let offsets = StringOffsets::<AllConfig>::new(s);
+//! let offsets: StringOffsets = StringOffsets::new(s);
 //!
 //! // Find offsets where lines begin and end.
 //! assert_eq!(offsets.line_to_utf8s(0), 0..12);  // note: 0-based line numbers
@@ -93,7 +93,9 @@ pub use config::{AllConfig, OnlyLines};
 /// Most operations run in O(1) time. A few require O(log n) time. The memory consumed by this
 /// data structure is typically less than the memory occupied by the actual content. In the best
 /// case, it requires ~45% of the content space.
-pub struct StringOffsets<C: ConfigType> {
+/// One can reduce memory requirements further by only requesting the necessary features via the
+/// configuration type.
+pub struct StringOffsets<C: ConfigType = AllConfig> {
     /// Vector storing, for every line, the byte position at which the line starts.
     line_begins: Vec<u32>,
 
@@ -488,7 +490,7 @@ mod tests {
         let content = r#"a short line.
 followed by another one.
 no terminating newline!"#;
-        let lines = StringOffsets::<AllConfig>::new(content);
+        let lines: StringOffsets = StringOffsets::new(content);
         assert_eq!(lines.line_to_utf8s(0), 0..14);
         assert_eq!(&content[0..14], "a short line.\n");
         assert_eq!(lines.line_to_utf8s(1), 14..39);
@@ -534,7 +536,7 @@ no terminating newline!"#;
     fn test_convert_ascii() {
         let content = r#"line0
 line1"#;
-        let lines = StringOffsets::<AllConfig>::new(content);
+        let lines: StringOffsets = StringOffsets::new(content);
         assert_eq!(lines.utf8_to_char_pos(0), pos(0, 0));
         assert_eq!(lines.utf8_to_char_pos(1), pos(0, 1));
         assert_eq!(lines.utf8_to_char_pos(6), pos(1, 0));
@@ -547,7 +549,7 @@ line1"#;
         let content = r#"❤️ line0
 line1
 ✅ line2"#;
-        let lines = StringOffsets::<AllConfig>::new(content);
+        let lines: StringOffsets = StringOffsets::new(content);
         assert_eq!(lines.utf8_to_char_pos(0), pos(0, 0)); // ❤️ takes 6 bytes to represent in utf8 (2 code points)
         assert_eq!(lines.utf8_to_char_pos(1), pos(0, 0));
         assert_eq!(lines.utf8_to_char_pos(2), pos(0, 0));
@@ -578,7 +580,7 @@ line1
     fn test_small() {
         // Á - 2 bytes utf8
         let content = r#"❤️ line0 ❤️Á 👋"#;
-        let lines = StringOffsets::<AllConfig>::new(content);
+        let lines: StringOffsets = StringOffsets::new(content);
         let mut utf16_index = 0;
         let mut char_index = 0;
         for (byte_index, char) in content.char_indices() {
@@ -598,7 +600,7 @@ line1
         //                 ^~~~ utf8: 1 char, 1 byte, utf16: 1 code unit
         //                ^~~~~ utf8: 1 char, 2 bytes, utf16: 1 code unit
         //               ^~~~~~ utf8: 2 chars, 3 byte ea., utf16: 2 code units
-        let lines = StringOffsets::<AllConfig>::new(content);
+        let lines: StringOffsets = StringOffsets::new(content);
 
         // UTF-16 positions
         assert_eq!(lines.utf8_to_utf16_pos(0), pos(0, 0)); // ❤️
@@ -644,7 +646,7 @@ line1
     #[test]
     fn test_critical_input_len() {
         let content = [b'a'; 16384];
-        let lines = StringOffsets::<AllConfig>::from_bytes(&content);
+        let lines: StringOffsets = StringOffsets::from_bytes(&content);
         assert_eq!(lines.utf8_to_utf16_pos(16384), pos(1, 0));
     }
 }
diff --git a/crates/string-offsets/src/wasm.rs b/crates/string-offsets/src/wasm.rs
@@ -2,89 +2,76 @@ use wasm_bindgen::prelude::*;
 
 use crate::{AllConfig, Pos, StringOffsets as StringOffsetsImpl};
 
-#[cfg_attr(feature = "wasm", wasm_bindgen)]
+#[wasm_bindgen]
 pub struct StringOffsets(StringOffsetsImpl<AllConfig>);
 
-#[cfg_attr(feature = "wasm", wasm_bindgen)]
+#[wasm_bindgen]
+#[allow(non_snake_case)]
 impl StringOffsets {
-    #[cfg_attr(feature = "wasm", wasm_bindgen(constructor))]
+    #[wasm_bindgen(constructor)]
     pub fn new(content: &str) -> Self {
         Self(StringOffsetsImpl::new(content))
     }
 
     #[allow(unused_variables)]
-    #[cfg_attr(feature = "wasm", wasm_bindgen(static_method_of = StringOffsets))]
+    #[wasm_bindgen(static_method_of = StringOffsets)]
     pub fn from_bytes(content: &[u8]) -> Self {
         Self(StringOffsetsImpl::from_bytes(content))
     }
 
-    #[cfg_attr(feature = "wasm", wasm_bindgen(js_name = lines))]
     pub fn lines(&self) -> usize {
         self.0.lines()
     }
 
-    #[cfg_attr(feature = "wasm", wasm_bindgen(js_name = lineToUtf8Begin))]
-    pub fn line_to_utf8_begin(&self, line_number: usize) -> usize {
+    pub fn lineToUtf8Begin(&self, line_number: usize) -> usize {
         self.0.line_to_utf8_begin(line_number)
     }
 
-    #[cfg_attr(feature = "wasm", wasm_bindgen(js_name = lineToUtf8End))]
-    pub fn line_to_utf8_end(&self, line_number: usize) -> usize {
+    pub fn lineToUtf8End(&self, line_number: usize) -> usize {
         self.0.line_to_utf8_end(line_number)
     }
 
-    #[cfg_attr(feature = "wasm", wasm_bindgen(js_name = utf8ToLine))]
-    pub fn utf8_to_line(&self, byte_number: usize) -> usize {
+    pub fn utf8ToLine(&self, byte_number: usize) -> usize {
         self.0.utf8_to_line(byte_number)
     }
 
-    #[cfg_attr(feature = "wasm", wasm_bindgen(js_name = lineChars))]
-    pub fn line_chars(&self, line_number: usize) -> usize {
+    pub fn lineChars(&self, line_number: usize) -> usize {
         self.0.line_chars(line_number)
     }
 
-    #[cfg_attr(feature = "wasm", wasm_bindgen(js_name = lineToCharBegin))]
-    pub fn line_to_char_begin(&self, line_number: usize) -> usize {
+    pub fn lineToCharBegin(&self, line_number: usize) -> usize {
         self.0.line_to_char_begin(line_number)
     }
 
-    #[cfg_attr(feature = "wasm", wasm_bindgen(js_name = lineToCharEnd))]
-    pub fn line_to_char_end(&self, line_number: usize) -> usize {
+    pub fn lineToCharEnd(&self, line_number: usize) -> usize {
         self.0.line_to_char_end(line_number)
     }
 
-    #[cfg_attr(feature = "wasm", wasm_bindgen(js_name = utf8ToCharPos))]
-    pub fn utf8_to_char_pos(&self, byte_number: usize) -> Pos {
+    pub fn utf8ToCharPos(&self, byte_number: usize) -> Pos {
         self.0.utf8_to_char_pos(byte_number)
     }
 
-    #[cfg_attr(feature = "wasm", wasm_bindgen(js_name = utf8ToChar))]
-    pub fn utf8_to_char(&self, byte_number: usize) -> usize {
+    pub fn utf8ToChar(&self, byte_number: usize) -> usize {
         self.0.utf8_to_char(byte_number)
     }
 
-    #[cfg_attr(feature = "wasm", wasm_bindgen(js_name = charToUtf8))]
-    pub fn char_to_utf8(&self, char_number: usize) -> usize {
+    pub fn charToUtf8(&self, char_number: usize) -> usize {
         self.0.char_to_utf8(char_number)
     }
 
-    #[cfg_attr(feature = "wasm", wasm_bindgen(js_name = utf8ToUtf16))]
-    pub fn utf8_to_utf16(&self, byte_number: usize) -> usize {
+    pub fn utf8ToUtf16(&self, byte_number: usize) -> usize {
         self.0.utf8_to_utf16(byte_number)
     }
 
-    #[cfg_attr(feature = "wasm", wasm_bindgen(js_name = lineToUtf16Begin))]
-    pub fn line_to_utf16_begin(&self, line_number: usize) -> usize {
+    pub fn lineToUtf16Begin(&self, line_number: usize) -> usize {
         self.0.line_to_utf16_begin(line_number)
     }
 
-    #[cfg_attr(feature = "wasm", wasm_bindgen(js_name = lineToUtf16End))]
-    pub fn line_to_utf16_end(&self, line_number: usize) -> usize {
+    pub fn lineToUtf16End(&self, line_number: usize) -> usize {
         self.0.line_to_utf16_end(line_number)
     }
 
-    #[cfg_attr(feature = "wasm", wasm_bindgen(js_name = utf8ToUtf16Pos))]
-    pub fn utf8_to_utf16_pos(&self, byte_number: usize) -> Pos {
+    pub fn utf8ToUtf16Pos(&self, byte_number: usize) -> Pos {
         self.0.utf8_to_utf16_pos(byte_number)
     }
 }
