From 99aa4510b60fe8320b8dc6300c826deceb176474 Mon Sep 17 00:00:00 2001 From: Eliad Peller Date: Wed, 1 Jan 2025 15:16:59 +0200 Subject: [PATCH 1/4] ArcIntern: add from_str() specialization for ArcIntern This is useful in order to allow looking for interned str without having to create owned value first. The internal BorrowStr trait was used since we can't just have blanket implementation for T: Borrow due to conflicting implementations (since T: Borrow) Signed-off-by: Eliad Peller --- src/arc.rs | 73 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 73 insertions(+) diff --git a/src/arc.rs b/src/arc.rs index 3f23f25..eb9ff59 100644 --- a/src/arc.rs +++ b/src/arc.rs @@ -123,6 +123,14 @@ impl Borrow> for BoxRefCount { &self.0 } } + +impl Borrow for BoxRefCount { + #[inline(always)] + fn borrow(&self) -> &str { + &self.0.data.borrow() + } +} + impl Deref for BoxRefCount { type Target = T; #[inline(always)] @@ -262,6 +270,60 @@ impl ArcIntern { } } +/// internal trait that allows us to specialize the `Borrow` trait for `str` +/// avoid the need to create an owned value first. +pub trait BorrowStr: Borrow {} + +impl BorrowStr for String {} + +/// BorrowStr specialization +impl ArcIntern { + /// Intern a value from a reference with atomic reference counting. + /// + /// this is a fast-path for str, as it avoids the need to create owned + /// value first. + pub fn from_str>(val: Q) -> ArcIntern + where + T: BorrowStr + for<'a> From<&'a str>, + { + // No reference only fast-path as + // the trait `std::borrow::Borrow` is not implemented for `Arc` + Self::new_from_str(val.as_ref()) + } + + /// Intern a value from a reference with atomic reference counting. + /// + /// If this value has not previously been + /// interned, then `new` will allocate a spot for the value on the + /// heap and generate that value using `T::from(val)`. + fn new_from_str<'a>(val: &'a str) -> ArcIntern + where + T: BorrowStr + From<&'a str>, + { + let m = Self::get_container(); + if let Some(b) = m.get(val) { + let b = b.key(); + // First increment the count. We are holding the write mutex here. + // Has to be the write mutex to avoid a race + let oldval = b.0.count.fetch_add(1, Ordering::SeqCst); + if oldval != 0 { + // we can only use this value if the value is not about to be freed + return ArcIntern { + pointer: std::ptr::NonNull::from(b.0.borrow()), + }; + } else { + // we have encountered a race condition here. + // we will just wait for the object to finish + // being freed. + b.0.count.fetch_sub(1, Ordering::SeqCst); + } + } + + // start over with the new value + Self::new(val.into()) + } +} + impl Clone for ArcIntern { fn clone(&self) -> Self { // First increment the count. Using a relaxed ordering is @@ -626,3 +688,14 @@ fn test_shrink_to_fit() { ArcIntern::::shrink_to_fit(); assert_eq!(0, ArcIntern::::get_container().capacity()); } + +#[test] +fn test_from_str() { + let x = ArcIntern::new("hello".to_string()); + let y = ArcIntern::::from_str("world"); + assert_ne!(x, y); + assert_eq!(x, ArcIntern::from_ref("hello")); + assert_eq!(x, ArcIntern::from_str("hello")); + assert_eq!(y, ArcIntern::from_ref("world")); + assert_eq!(&*x, "hello"); +} From f018d00348c2309aeffdd82fc676fef845ce5040 Mon Sep 17 00:00:00 2001 From: Eliad Peller Date: Wed, 1 Jan 2025 15:18:05 +0200 Subject: [PATCH 2/4] benches: define Vec with the correct capacity (the alternative is using HashSet) Signed-off-by: Eliad Peller --- benches/get_container.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/benches/get_container.rs b/benches/get_container.rs index 36b5d41..088b0ff 100644 --- a/benches/get_container.rs +++ b/benches/get_container.rs @@ -19,7 +19,7 @@ fn bench_get_container(c: &mut Criterion) { b.iter_batched( || {}, |_| { - let mut ans = Vec::with_capacity(RANGE); + let mut ans = Vec::with_capacity(ITER); for idx in 0..ITER { let s = ArcIntern::::new(format!("short-{}", idx % RANGE)); ans.push(s); @@ -36,7 +36,7 @@ fn bench_get_container(c: &mut Criterion) { b.iter_batched( || {}, |_| { - let mut ans = Vec::with_capacity(RANGE); + let mut ans = Vec::with_capacity(ITER); for idx in 0..ITER { let s = ArcIntern::>::new(NewType(format!( "short-{}", @@ -54,7 +54,7 @@ fn bench_get_container(c: &mut Criterion) { b.iter_batched( || {}, |_| { - let mut ans = Vec::with_capacity(RANGE); + let mut ans = Vec::with_capacity(ITER); for idx in 0..ITER { let s = ArcIntern::::new(idx % RANGE); ans.push(s); @@ -68,7 +68,7 @@ fn bench_get_container(c: &mut Criterion) { b.iter_batched( || {}, |_| { - let mut ans = Vec::with_capacity(RANGE); + let mut ans = Vec::with_capacity(ITER); for idx in 0..ITER { let s = ArcIntern::>::new(NewType(idx % RANGE)); ans.push(s); From aa2d075ee3db6eb4724d45b5e5715d6851fa89fe Mon Sep 17 00:00:00 2001 From: Eliad Peller Date: Wed, 1 Jan 2025 15:57:34 +0200 Subject: [PATCH 3/4] benches: build strings outside of the test the string building is not part of the test, and taking them out will allow benchmarking against other scenarios more easily Signed-off-by: Eliad Peller --- benches/get_container.rs | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/benches/get_container.rs b/benches/get_container.rs index 088b0ff..da11c62 100644 --- a/benches/get_container.rs +++ b/benches/get_container.rs @@ -14,6 +14,8 @@ struct NewType(T); fn bench_get_container(c: &mut Criterion) { let mut group = c.benchmark_group("cached"); + let vals: Vec<_> = (0..RANGE).map(|idx| format!("short-{}", idx)).collect(); + // time: [17.635 ms 17.707 ms 17.782 ms] group.bench_function(BenchmarkId::new("String", "short"), |b| { b.iter_batched( @@ -21,7 +23,7 @@ fn bench_get_container(c: &mut Criterion) { |_| { let mut ans = Vec::with_capacity(ITER); for idx in 0..ITER { - let s = ArcIntern::::new(format!("short-{}", idx % RANGE)); + let s = ArcIntern::::new(vals[idx % RANGE].clone()); ans.push(s); } }, @@ -30,6 +32,9 @@ fn bench_get_container(c: &mut Criterion) { }); group.finish(); + let new_vals: Vec<_> = (0..RANGE) + .map(|idx| NewType(format!("short-{}", idx))) + .collect(); let mut group = c.benchmark_group("uncached"); // time: [22.209 ms 22.294 ms 22.399 ms] => that's 26% faster! group.bench_function(BenchmarkId::new("NewType", "short"), |b| { @@ -38,10 +43,7 @@ fn bench_get_container(c: &mut Criterion) { |_| { let mut ans = Vec::with_capacity(ITER); for idx in 0..ITER { - let s = ArcIntern::>::new(NewType(format!( - "short-{}", - idx % RANGE - ))); + let s = ArcIntern::>::new(new_vals[idx % RANGE].clone()); ans.push(s); } }, From b6a74d62347e9f308f652766259cd12adf5fc9ca Mon Sep 17 00:00:00 2001 From: Eliad Peller Date: Wed, 1 Jan 2025 16:05:40 +0200 Subject: [PATCH 4/4] benches: add from_ref / from_str benchmarks using the from_str benchmark we can see that it indeed gives performance boost if the strings were already interned, as it avoids a redundant string allocation. cached/String/short time: [13.598 ms 13.783 ms 13.994 ms] change: [-0.5323% +1.5518% +3.7562%] (p = 0.15 > 0.05) No change in performance detected. Found 14 outliers among 100 measurements (14.00%) 7 (7.00%) high mild 7 (7.00%) high severe cached/String/short-from_ref time: [13.582 ms 13.757 ms 13.955 ms] change: [-1.5349% +0.4010% +2.5499%] (p = 0.70 > 0.05) No change in performance detected. Found 12 outliers among 100 measurements (12.00%) 3 (3.00%) high mild 9 (9.00%) high severe cached/String/short-from_str time: [9.3047 ms 9.4529 ms 9.6175 ms] change: [-1.9867% +0.4488% +2.9423%] (p = 0.72 > 0.05) No change in performance detected. Found 15 outliers among 100 measurements (15.00%) Signed-off-by: Eliad Peller --- benches/get_container.rs | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/benches/get_container.rs b/benches/get_container.rs index da11c62..9db3736 100644 --- a/benches/get_container.rs +++ b/benches/get_container.rs @@ -30,6 +30,35 @@ fn bench_get_container(c: &mut Criterion) { criterion::BatchSize::PerIteration, ); }); + group.bench_function(BenchmarkId::new("String", "short-from_ref"), |b| { + b.iter_batched( + || {}, + |_| { + let mut ans = Vec::with_capacity(ITER); + for idx in 0..ITER { + let s = ArcIntern::::from_ref(&vals[idx % RANGE]); + + ans.push(s); + } + }, + criterion::BatchSize::PerIteration, + ); + }); + + group.bench_function(BenchmarkId::new("String", "short-from_str"), |b| { + b.iter_batched( + || {}, + |_| { + let mut ans = Vec::with_capacity(ITER); + for idx in 0..ITER { + let s = ArcIntern::::from_str(&vals[idx % RANGE]); + + ans.push(s); + } + }, + criterion::BatchSize::PerIteration, + ); + }); group.finish(); let new_vals: Vec<_> = (0..RANGE)