diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 7f2b2d0..7d376e3 100644 --- a/.github/workflows/ci.yml +++ b/.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: | diff --git a/Cargo.toml b/Cargo.toml index 96488e3..9802482 100644 --- a/Cargo.toml +++ b/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 diff --git a/README.md b/README.md index 5253fdd..537410c 100644 --- a/README.md +++ b/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")); } diff --git a/integration-tests/Cargo.toml b/integration-tests/Cargo.toml index b1091f3..d4d2631 100644 --- a/integration-tests/Cargo.toml +++ b/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" } diff --git a/integration-tests/src/bench.rs b/integration-tests/src/bench.rs index 04ec795..eedf101 100644 --- a/integration-tests/src/bench.rs +++ b/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; diff --git a/integration-tests/src/lib.rs b/integration-tests/src/lib.rs index 641caaf..ff17e30 100644 --- a/integration-tests/src/lib.rs +++ b/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()); diff --git a/src/atom.rs b/src/atom.rs index 8a2f782..443b2b5 100644 --- a/src/atom.rs +++ b/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::() >= 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 { @@ -166,19 +190,22 @@ impl Atom { 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 Atom { impl Default for Atom { #[inline] fn default() -> Self { - Atom::pack_static(Static::empty_string_index()) + Atom::pack_inline(0, 0) } } @@ -225,16 +252,14 @@ impl Hash for Atom { where H: Hasher, { - state.write_u32(self.get_hash()) + state.write_u64(self.get_hash()) } } impl<'a, Static: StaticAtomSet> From> for Atom { 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> for Atom { } } else { Self::try_static_internal(&string_to_add).unwrap_or_else(|hash| { - let ptr: std::ptr::NonNull = 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 = dynamic_set().insert(string_to_add, hash); let data = ptr.as_ptr().expose_provenance() as u64; debug_assert!(0 == data & TAG_MASK); Atom { diff --git a/src/dynamic_set.rs b/src/dynamic_set.rs index e74fb05..4759b26 100644 --- a/src/dynamic_set.rs +++ b/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>>]>, @@ -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, - 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>>, @@ -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_ALIGNMENT); -} - pub(crate) fn dynamic_set() -> &'static Set { // NOTE: Using const initialization for buckets breaks the small-stack test. static DYNAMIC_SET: OnceLock = OnceLock::new(); @@ -61,7 +51,7 @@ pub(crate) fn dynamic_set() -> &'static Set { } impl Set { - pub(crate) fn insert(&self, string: Cow, hash: u32) -> NonNull { + pub(crate) fn insert(&self, string: Cow, hash: u64) -> NonNull { let bucket_index = (hash & BUCKET_MASK) as usize; let mut linked_list = self.buckets[bucket_index].lock(); @@ -93,7 +83,6 @@ impl Set { ptr = unsafe { entry.next_in_bucket.get().read() }; } } - debug_assert!(mem::align_of::() >= ENTRY_ALIGNMENT); let string = string.into_owned(); let entry = Box::new(Entry { next_in_bucket: UnsafeCell::new(linked_list.take()), @@ -101,10 +90,7 @@ impl Set { 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 } diff --git a/src/lib.rs b/src/lib.rs index 193ab71..c343657 100644 --- a/src/lib.rs +++ b/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; -// 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::(), 8); - assert_eq!(size_of::>(), size_of::(),); -} +const _: () = assert!(std::mem::size_of::() == 8); +const _: () = assert!(std::mem::size_of::>() == 8); diff --git a/src/static_sets.rs b/src/static_sets.rs index f7f1799..10f03bb 100644 --- a/src/static_sets.rs +++ b/src/static_sets.rs @@ -18,8 +18,6 @@ pub trait StaticAtomSet: Ord { /// Get the location of the static string set in the binary. fn get() -> &'static PhfStrSet; - /// Get the index of the empty string, which is in every set and is used for `Atom::default`. - fn empty_string_index() -> u32; } /// A string set created using a [perfect hash function], specifically @@ -37,7 +35,7 @@ pub struct PhfStrSet { #[doc(hidden)] pub atoms: &'static [&'static str], #[doc(hidden)] - pub hashes: &'static [u32], + pub hashes: &'static [u64], } /// An empty static atom set for when only dynamic strings will be added @@ -57,8 +55,4 @@ impl StaticAtomSet for EmptyStaticAtomSet { }; &SET } - - fn empty_string_index() -> u32 { - 0 - } } diff --git a/src/trivial_impls.rs b/src/trivial_impls.rs index e001769..8c25dc9 100644 --- a/src/trivial_impls.rs +++ b/src/trivial_impls.rs @@ -15,7 +15,8 @@ use std::fmt; impl ::precomputed_hash::PrecomputedHash for Atom { fn precomputed_hash(&self) -> u32 { - self.get_hash() + let hash64 = self.get_hash(); + (hash64 >> 32) as u32 ^ hash64 as u32 } } diff --git a/string-cache-codegen/Cargo.toml b/string-cache-codegen/Cargo.toml index eeb2037..f12daab 100644 --- a/string-cache-codegen/Cargo.toml +++ b/string-cache-codegen/Cargo.toml @@ -1,12 +1,12 @@ [package] name = "string_cache_codegen" -version = "0.7.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 codegen library for string-cache, 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_codegen/" -edition = "2018" +edition = "2024" [lib] name = "string_cache_codegen" diff --git a/string-cache-codegen/lib.rs b/string-cache-codegen/lib.rs index 26e7ed3..861dd76 100644 --- a/string-cache-codegen/lib.rs +++ b/string-cache-codegen/lib.rs @@ -28,8 +28,6 @@ //! In `build.rs`: //! //! ```no_run -//! extern crate string_cache_codegen; -//! //! use std::env; //! use std::path::Path; //! @@ -44,8 +42,6 @@ //! In `lib.rs`: //! //! ```ignore -//! extern crate string_cache; -//! //! mod foo { //! include!(concat!(env!("OUT_DIR"), "/foo_atom.rs")); //! } @@ -203,25 +199,22 @@ impl AtomType { #[expect(clippy::wrong_self_convention)] // Doesn’t matter on a private method fn to_tokens(&mut self) -> proc_macro2::TokenStream { - // `impl Default for Atom` requires the empty string to be in the static set. - // This also makes sure the set in non-empty, - // which would cause divisions by zero in rust-phf. + // Make `atom!("")` always work, and ensure the set is non-empty + // to avoid divisions by zero in rust-phf. self.atoms.insert(String::new()); - // Strings over 7 bytes + empty string added to static set. - // Otherwise stored inline. + // Strings over 7 bytes added to static set, otherwise stored inline. let (static_strs, inline_strs): (Vec<_>, Vec<_>) = self .atoms .iter() .map(String::as_str) - .partition(|s| s.len() > 7 || s.is_empty()); + .partition(|s| s.len() > 7); // Static strings let hash_state = phf_generator::generate_hash(&static_strs); let phf_generator::HashState { key, disps, map } = hash_state; let (disps0, disps1): (Vec<_>, Vec<_>) = disps.into_iter().unzip(); let atoms: Vec<&str> = map.iter().map(|&idx| static_strs[idx]).collect(); - let empty_string_index = atoms.iter().position(|s| s.is_empty()).unwrap() as u32; let indices = 0..atoms.len() as u32; fn is_valid_ident(name: &str) -> bool { @@ -248,11 +241,13 @@ impl AtomType { .collect(); let istr_idents: Vec = istrs_for_idents.iter().map(|atom| new_term(atom)).collect(); - let hashes: Vec = atoms + let hashes: Vec = atoms .iter() .map(|string| { let hash = phf_shared::hash(string, &key); - (hash.g ^ hash.f1) as u32 + // Reconstitute 64-bit `Hash128::h1` + // https://docs.rs/phf_shared/0.14.0/src/phf_shared/lib.rs.html#45-54 + (hash.g as u64) << 32 | (hash.f1 as u64) }) .collect(); @@ -338,9 +333,6 @@ impl AtomType { }; &SET } - fn empty_string_index() -> u32 { - #empty_string_index - } } #(