Assorted changes and prepare 0.11.0 release

.github/workflows/ci.yml

@@ -44,7 +44,7 @@ jobs:
           args: --workspace
       - name: Run Clippy
         if: ${{ matrix.rust == 'stable' }}
-        run: cargo clippy --workspace
+        run: cargo clippy --workspace --all-targets
       - name: Install Miri
         if: ${{ matrix.rust == 'nightly' }}
         run: |

Cargo.toml

@@ -1,12 +1,12 @@
 [package]
 name = "string_cache"
-version = "0.10.0"  # Also update README.md when making a semver-breaking change
+version = "0.11.0"  # Also update README.md when making a semver-breaking change
 authors = ["The Servo Project Developers"]
 description = "A string interning library for Rust, developed as part of the Servo project."
 license = "MIT OR Apache-2.0"
 repository = "https://github.com/servo/string-cache"
 documentation = "https://docs.rs/string_cache"
-edition = "2018"
+edition = "2024"
 rust-version = "1.85"
 
 # Do not `exclude` ./string-cache-codegen because we want to include

README.md

@@ -12,36 +12,33 @@ In `Cargo.toml`:
 
 ```toml
 [dependencies]
-string_cache = "0.10"
+string_cache = "0.11"
 ```
 
 In `lib.rs`:
 
 ```rust
-extern crate string_cache;
 use string_cache::DefaultAtom as Atom;
 ```
 
 ## With static atoms
 
-In `Cargo.toml`:
+In `Cargo.toml`, corresponding versions of the two crates must be used:
 
 ```toml
 [package]
 build = "build.rs"
 
 [dependencies]
-string_cache = "0.10"
+string_cache = "0.11"
 
 [build-dependencies]
-string_cache_codegen = "0.7"
+string_cache_codegen = "0.11"
 ```
 
 In `build.rs`:
 
 ```rust
-extern crate string_cache_codegen;
-
 use std::env;
 use std::path::Path;
 
@@ -56,8 +53,6 @@ fn main() {
 In `lib.rs`:
 
 ```rust
-extern crate string_cache;
-
 mod foo {
     include!(concat!(env!("OUT_DIR"), "/foo_atom.rs"));
 }

integration-tests/Cargo.toml

@@ -4,7 +4,7 @@ version = "0.0.1"
 authors = [ "The Servo Project Developers" ]
 build = "build.rs"
 publish = false
-edition = "2018"
+edition = "2024"
 
 [lib]
 doctest = false
@@ -16,11 +16,11 @@ test = true
 unstable = []
 
 [dependencies]
-string_cache = { version = "0.10", path = ".." }
+string_cache = { path = ".." }
 
 [dev-dependencies]
 rand = { version = "0.8", features = ["small_rng"] }
-string_cache_codegen = { version = "0.7", path = "../string-cache-codegen" }
+string_cache_codegen = { path = "../string-cache-codegen" }
 
 [build-dependencies]
-string_cache_codegen = { version = "0.7", path = "../string-cache-codegen" }
+string_cache_codegen = { path = "../string-cache-codegen" }

integration-tests/src/bench.rs

@@ -28,7 +28,7 @@ and cheap to move around, which isn't reflected in these tests.
 */
 use crate::TestAtom;
 
-use test::{black_box, Bencher};
+use test::{Bencher, black_box};
 
 // Just shorthand
 fn mk(x: &str) -> TestAtom {
@@ -154,10 +154,8 @@ macro_rules! bench_all (
     );
 );
 
-pub const longer_dynamic_a: &'static str =
-    "Thee Silver Mt. Zion Memorial Orchestra & Tra-La-La Band";
-pub const longer_dynamic_b: &'static str =
-    "Thee Silver Mt. Zion Memorial Orchestra & Tra-La-La Ban!";
+pub const longer_dynamic_a: &str = "Thee Silver Mt. Zion Memorial Orchestra & Tra-La-La Band";
+pub const longer_dynamic_b: &str = "Thee Silver Mt. Zion Memorial Orchestra & Tra-La-La Ban!";
 
 bench_all!([eq ne lt clone_string] for short_string = "e", "f");
 bench_all!([eq ne lt clone_string] for medium_string = "xyzzy01", "xyzzy02");
@@ -194,11 +192,10 @@ bench_all!([ne lt x_inline y_dynamic]
 macro_rules! bench_rand ( ($name:ident, $len:expr) => (
     #[bench]
     fn $name(b: &mut Bencher) {
-        use std::str;
         use rand;
         use rand::{RngCore, SeedableRng};
 
-        let mut gen = rand::rngs::SmallRng::from_entropy();
+        let mut rng = rand::rngs::SmallRng::from_entropy();
         b.iter(|| {
             // We have to generate new atoms on every iter, because
             // the dynamic atom table isn't reset.
@@ -207,7 +204,7 @@ macro_rules! bench_rand ( ($name:ident, $len:expr) => (
             // as about 3-12% at one point.
 
             let mut buf: [u8; $len] = [0; $len];
-            gen.fill_bytes(&mut buf);
+            rng.fill_bytes(&mut buf);
             for n in buf.iter_mut() {
                 // shift into printable ASCII
                 *n = (*n % 0x40) + 0x20;

integration-tests/src/lib.rs

@@ -108,7 +108,7 @@ fn test_as_ref_bytes() {
 
 #[test]
 fn test_types() {
-    assert!(Atom::from("").is_static());
+    assert!(Atom::from("").is_inline());
     assert!(Atom::from("defaults").is_static());
     assert!(Atom::from("font-weight").is_static());
     assert!(Atom::from("id").is_inline());

src/atom.rs

@@ -7,7 +7,7 @@
 // option. This file may not be copied, modified, or distributed
 // except according to those terms.
 
-use crate::dynamic_set::{dynamic_set, Entry};
+use crate::dynamic_set::{Entry, dynamic_set};
 use crate::static_sets::StaticAtomSet;
 use debug_unreachable::debug_unreachable;
 
@@ -27,6 +27,10 @@ const DYNAMIC_TAG: u8 = 0b_00;
 const INLINE_TAG: u8 = 0b_01; // len in upper nybble
 const STATIC_TAG: u8 = 0b_10;
 const TAG_MASK: u64 = 0b_11;
+
+/// With alignment, a `*const Entry` pointer always has zeroes in its lowest `TAG_BITS` bits
+const _: () = assert!(mem::align_of::<Entry>() >= TAG_MASK.next_power_of_two() as usize);
+
 const LEN_OFFSET: u64 = 4;
 const LEN_MASK: u64 = 0xF0;
 
@@ -75,6 +79,26 @@ const STATIC_SHIFT_BITS: usize = 32;
 ///     }
 /// } // atom is dropped here, so it is not kept around in memory
 /// ```
+///
+/// ## Internal representation
+///
+/// An `Atom` is always 64 bits / 8 bytes.
+/// The least-significant two bits form a tag to distinguish three different representations:
+///
+/// * `0b01`: A short string up to 7 bytes, stored inline in most-significant 56 bits.
+///   Bits #4 to #7 (the upper nibble of the lower byte) are the length of the string.
+/// * `0b10`: A string part of a statically-known indexed set with [perfect hashing].
+///   The most-significant 32 bits are the index in the set.
+/// * `0b00`: For other cases, the entire 64 bits are a heap-allocated pointer
+///   to an entry in a global hash map.
+///   Alignment of the allocation ensures the tag bits are indeed zero.
+///   The entry is atomically reference-counted.
+///   It is removed from the map and deallocated when its last `Atom` is dropped.
+///   The map exists so that interning the same string again gives another pointer to the same entry.
+///
+/// In all cases, shallow 64-bit equality is equivalent to string equality.
+///
+/// [perfect hashing]: https://docs.rs/phf/latest/phf/
 #[derive(PartialEq, Eq)]
 // NOTE: Deriving PartialEq requires that a given string must always be interned the same way.
 pub struct Atom<Static> {
@@ -166,19 +190,22 @@ impl<Static: StaticAtomSet> Atom<Static> {
         self.unsafe_data.get() >> STATIC_SHIFT_BITS
     }
 
-    /// Get the hash of the string as it is stored in the set.
-    pub fn get_hash(&self) -> u32 {
+    /// Returns a hash of the string
+    ///
+    /// For static or dynamic atoms, it is a pre-computed high-quality hash.
+    ///
+    /// For inline atoms however (short strings 7 bytes or less),
+    /// the returned value is the literal inline representation
+    /// with string bytes packed directly in the `u64` value,
+    /// which makes it a relatively poor-quality hash if used directly.
+    pub fn get_hash(&self) -> u64 {
         match self.tag() {
             DYNAMIC_TAG => {
                 let entry = self.dynamic_ptr();
                 unsafe { (*entry).hash }
             }
             STATIC_TAG => Static::get().hashes[self.static_index() as usize],
-            INLINE_TAG => {
-                let data = self.unsafe_data.get();
-                // This may or may not be great...
-                ((data >> 32) ^ data) as u32
-            }
+            INLINE_TAG => self.unsafe_data.get(),
             _ => unsafe { debug_unreachable!() },
         }
     }
@@ -215,7 +242,7 @@ impl<Static: StaticAtomSet> Atom<Static> {
 impl<Static: StaticAtomSet> Default for Atom<Static> {
     #[inline]
     fn default() -> Self {
-        Atom::pack_static(Static::empty_string_index())
+        Atom::pack_inline(0, 0)
     }
 }
 
@@ -225,16 +252,14 @@ impl<Static: StaticAtomSet> Hash for Atom<Static> {
     where
         H: Hasher,
     {
-        state.write_u32(self.get_hash())
+        state.write_u64(self.get_hash())
     }
 }
 
 impl<'a, Static: StaticAtomSet> From<Cow<'a, str>> for Atom<Static> {
     fn from(string_to_add: Cow<'a, str>) -> Self {
         let len = string_to_add.len();
-        if len == 0 {
-            Self::pack_static(Static::empty_string_index())
-        } else if len <= MAX_INLINE_LEN {
+        if len <= MAX_INLINE_LEN {
             let mut data: u64 = (INLINE_TAG as u64) | ((len as u64) << LEN_OFFSET);
             {
                 let dest = inline_atom_slice_mut(&mut data);
@@ -247,7 +272,10 @@ impl<'a, Static: StaticAtomSet> From<Cow<'a, str>> for Atom<Static> {
             }
         } else {
             Self::try_static_internal(&string_to_add).unwrap_or_else(|hash| {
-                let ptr: std::ptr::NonNull<Entry> = dynamic_set().insert(string_to_add, hash.g);
+                // Reconstitute 64-bit `Hash128::h1`
+                // https://docs.rs/phf_shared/0.14.0/src/phf_shared/lib.rs.html#45-54
+                let hash = (hash.g as u64) << 32 | (hash.f1 as u64);
+                let ptr: std::ptr::NonNull<Entry> = dynamic_set().insert(string_to_add, hash);
                 let data = ptr.as_ptr().expose_provenance() as u64;
                 debug_assert!(0 == data & TAG_MASK);
                 Atom {

src/dynamic_set.rs

@@ -10,14 +10,13 @@
 use parking_lot::Mutex;
 use std::borrow::Cow;
 use std::cell::UnsafeCell;
-use std::mem;
 use std::ptr::NonNull;
+use std::sync::OnceLock;
 use std::sync::atomic::AtomicIsize;
 use std::sync::atomic::Ordering::SeqCst;
-use std::sync::OnceLock;
 
 const NB_BUCKETS: usize = 1 << 12; // 4096
-const BUCKET_MASK: u32 = (1 << 12) - 1;
+const BUCKET_MASK: u64 = (1 << 12) - 1;
 
 pub(crate) struct Set {
     buckets: Box<[Mutex<Option<NonNull<Entry>>>]>,
@@ -26,7 +25,7 @@ pub(crate) struct Set {
 pub(crate) struct Entry {
     // These fields can be accessed freely by `Atom` methods
     pub(crate) string: Box<str>,
-    pub(crate) hash: u32,
+    pub(crate) hash: u64,
     pub(crate) ref_count: AtomicIsize,
     // This field is protected by a `Mutex` in `Set`
     next_in_bucket: UnsafeCell<Option<NonNull<Entry>>>,
@@ -41,15 +40,6 @@ unsafe impl Sync for Entry {}
 unsafe impl Send for Set {}
 unsafe impl Sync for Set {}
 
-// Addresses are a multiples of this,
-// and therefore have have TAG_MASK bits unset, available for tagging.
-pub(crate) const ENTRY_ALIGNMENT: usize = 4;
-
-#[test]
-fn entry_alignment_is_sufficient() {
-    assert!(mem::align_of::<Entry>() >= ENTRY_ALIGNMENT);
-}
-
 pub(crate) fn dynamic_set() -> &'static Set {
     // NOTE: Using const initialization for buckets breaks the small-stack test.
     static DYNAMIC_SET: OnceLock<Set> = OnceLock::new();
@@ -61,7 +51,7 @@ pub(crate) fn dynamic_set() -> &'static Set {
 }
 
 impl Set {
-    pub(crate) fn insert(&self, string: Cow<str>, hash: u32) -> NonNull<Entry> {
+    pub(crate) fn insert(&self, string: Cow<str>, hash: u64) -> NonNull<Entry> {
         let bucket_index = (hash & BUCKET_MASK) as usize;
         let mut linked_list = self.buckets[bucket_index].lock();
 
@@ -93,18 +83,14 @@ impl Set {
                 ptr = unsafe { entry.next_in_bucket.get().read() };
             }
         }
-        debug_assert!(mem::align_of::<Entry>() >= ENTRY_ALIGNMENT);
         let string = string.into_owned();
         let entry = Box::new(Entry {
             next_in_bucket: UnsafeCell::new(linked_list.take()),
             hash,
             ref_count: AtomicIsize::new(1),
             string: string.into_boxed_str(),
         });
-        // TODO: use `Box::into_non_null` when MSRV has it:
-        // https://github.com/rust-lang/rust/issues/130364
-        // SAFETY: `Box::into_raw` always returns a non-null pointer
-        let ptr = unsafe { NonNull::new_unchecked(Box::into_raw(entry)) };
+        let ptr = NonNull::from(Box::leak(entry));
         *linked_list = Some(ptr);
         ptr
     }

src/lib.rs

@@ -34,8 +34,6 @@
 //! In `build.rs`:
 //!
 //! ```ignore
-//! extern crate string_cache_codegen;
-//!
 //! use std::env;
 //! use std::path::Path;
 //!
@@ -50,8 +48,6 @@
 //! In `lib.rs`:
 //!
 //! ```ignore
-//! extern crate string_cache;
-//!
 //! mod foo {
 //!     include!(concat!(env!("OUT_DIR"), "/foo_atom.rs"));
 //! }
@@ -73,7 +69,6 @@
 //! ## No compile-time atoms
 //!
 //! ```
-//! # extern crate string_cache;
 //! use string_cache::DefaultAtom;
 //!
 //! # fn main() {
@@ -114,13 +109,5 @@ pub use static_sets::{EmptyStaticAtomSet, PhfStrSet, StaticAtomSet};
 /// Use this if you don’t care about static atoms.
 pub type DefaultAtom = Atom<EmptyStaticAtomSet>;
 
-// Some minor tests of internal layout here.
-// See ../integration-tests for much more.
-
-/// Guard against accidental changes to the sizes of things.
-#[test]
-fn assert_sizes() {
-    use std::mem::size_of;
-    assert_eq!(size_of::<DefaultAtom>(), 8);
-    assert_eq!(size_of::<Option<DefaultAtom>>(), size_of::<DefaultAtom>(),);
-}
+const _: () = assert!(std::mem::size_of::<DefaultAtom>() == 8);
+const _: () = assert!(std::mem::size_of::<Option<DefaultAtom>>() == 8);