From 2fe27f510df465ddd663086d0420f01e121886b4 Mon Sep 17 00:00:00 2001 From: David Freifeld Date: Wed, 2 Jul 2025 11:57:34 -0700 Subject: [PATCH] Make neon-shmem tests thread-safe and report errno in panics --- libs/neon-shmem/benches/hmap_resize.rs | 16 ++++++---------- libs/neon-shmem/src/hash/tests.rs | 24 +++++++++++++----------- libs/neon-shmem/src/sync.rs | 14 ++++++++------ 3 files changed, 27 insertions(+), 27 deletions(-) diff --git a/libs/neon-shmem/benches/hmap_resize.rs b/libs/neon-shmem/benches/hmap_resize.rs index 30a3dca296..6b86e7ed27 100644 --- a/libs/neon-shmem/benches/hmap_resize.rs +++ b/libs/neon-shmem/benches/hmap_resize.rs @@ -1,9 +1,7 @@ -use std::hint::black_box; use criterion::{criterion_group, criterion_main, BatchSize, Criterion, BenchmarkId}; use neon_shmem::hash::HashMapAccess; use neon_shmem::hash::HashMapInit; use neon_shmem::hash::entry::Entry; -use neon_shmem::shmem::ShmemHandle; use rand::prelude::*; use rand::distr::{Distribution, StandardUniform}; use std::hash::BuildHasher; @@ -65,14 +63,13 @@ fn apply_op( op: TestOp, map: &mut HashMapAccess, ) { - let hash = map.get_hash_value(&op.0); - let entry = map.entry_with_hash(op.0, hash); + let entry = map.entry(op.0); match op.1 { Some(new) => { match entry { Entry::Occupied(mut e) => Some(e.insert(new)), - Entry::Vacant(e) => { e.insert(new).unwrap(); None }, + Entry::Vacant(e) => { _ = e.insert(new).unwrap(); None }, } }, None => { @@ -184,15 +181,14 @@ fn real_benchs(c: &mut Criterion) { let mut rng = rand::rng(); b.iter_batched( || HashMapInit::new_resizeable(size, size * 2).attach_writer(), - |mut writer| { - for i in 0..ideal_filled { + |writer| { + for _ in 0..ideal_filled { let key: FileCacheKey = rng.random(); let val = FileCacheEntry::dummy(); - let hash = writer.get_hash_value(&key); - let entry = writer.entry_with_hash(key, hash); + let entry = writer.entry(key); std::hint::black_box(match entry { Entry::Occupied(mut e) => { e.insert(val); }, - Entry::Vacant(e) => { e.insert(val).unwrap(); }, + Entry::Vacant(e) => { _ = e.insert(val).unwrap(); }, }) } }, diff --git a/libs/neon-shmem/src/hash/tests.rs b/libs/neon-shmem/src/hash/tests.rs index 0c760c56b7..d7b8d19050 100644 --- a/libs/neon-shmem/src/hash/tests.rs +++ b/libs/neon-shmem/src/hash/tests.rs @@ -114,7 +114,7 @@ fn apply_op( Some(new) => { match entry { Entry::Occupied(mut e) => Some(e.insert(new)), - Entry::Vacant(e) => { e.insert(new).unwrap(); None }, + Entry::Vacant(e) => { _ = e.insert(new).unwrap(); None }, } }, None => { @@ -277,13 +277,13 @@ fn test_idx_get() { do_random_ops(2000, 1500, 0.25, &mut writer, &mut shadow, &mut rng); for _ in 0..100 { let idx = (rng.next_u32() % 1500) as usize; - if let Some(mut e) = writer.entry_at_bucket(idx) { + if let Some(pair) = writer.get_at_bucket(idx) { { - let v: *const usize = e.get(); + let v: *const usize = &pair.1; assert_eq!(writer.get_bucket_for_value(v), idx); } { - let v: *const usize = e.get_mut(); + let v: *const usize = &pair.1; assert_eq!(writer.get_bucket_for_value(v), idx); } } @@ -339,7 +339,7 @@ fn test_bucket_ops() { ).attach_writer(); match writer.entry(1.into()) { Entry::Occupied(mut e) => { e.insert(2); }, - Entry::Vacant(e) => { e.insert(2).unwrap(); }, + Entry::Vacant(e) => { _ = e.insert(2).unwrap(); }, } assert_eq!(writer.get_num_buckets_in_use(), 1); assert_eq!(writer.get_num_buckets(), 1000); @@ -348,14 +348,16 @@ fn test_bucket_ops() { Entry::Occupied(e) => { assert_eq!(e._key, 1.into()); let pos = e.bucket_pos as usize; - assert_eq!(writer.entry_at_bucket(pos).unwrap()._key, 1.into()); - assert_eq!(*writer.get_at_bucket(pos).unwrap(), (1.into(), 2)); pos }, Entry::Vacant(_) => { panic!("Insert didn't affect entry"); }, }; - let ptr: *const usize = &*writer.get(&1.into()).unwrap(); - assert_eq!(writer.get_bucket_for_value(ptr), pos); + assert_eq!(writer.entry_at_bucket(pos).unwrap()._key, 1.into()); + assert_eq!(*writer.get_at_bucket(pos).unwrap(), (1.into(), 2)); + { + let ptr: *const usize = &*writer.get(&1.into()).unwrap(); + assert_eq!(writer.get_bucket_for_value(ptr), pos); + } writer.remove(&1.into()); assert!(writer.get(&1.into()).is_none()); } @@ -390,7 +392,7 @@ fn test_shrink_zero() { #[test] #[should_panic] fn test_grow_oom() { - let mut writer = HashMapInit::::new_resizeable_named( + let writer = HashMapInit::::new_resizeable_named( 1500, 2000, "test_grow_oom" ).attach_writer(); writer.grow(20000).unwrap(); @@ -408,7 +410,7 @@ fn test_shrink_bigger() { #[test] #[should_panic] fn test_shrink_early_finish() { - let mut writer = HashMapInit::::new_resizeable_named( + let writer = HashMapInit::::new_resizeable_named( 1500, 2500, "test_shrink_early_finish" ).attach_writer(); writer.finish_shrink().unwrap(); diff --git a/libs/neon-shmem/src/sync.rs b/libs/neon-shmem/src/sync.rs index 8887299a92..fc39df9100 100644 --- a/libs/neon-shmem/src/sync.rs +++ b/libs/neon-shmem/src/sync.rs @@ -3,6 +3,8 @@ use std::mem::MaybeUninit; use std::ptr::NonNull; +use nix::errno::Errno; + pub type RwLock = lock_api::RwLock; pub(crate) type RwLockReadGuard<'a, T> = lock_api::RwLockReadGuard<'a, PthreadRwLock, T>; pub type RwLockWriteGuard<'a, T> = lock_api::RwLockWriteGuard<'a, PthreadRwLock, T>; @@ -48,7 +50,7 @@ unsafe impl lock_api::RawRwLock for PthreadRwLock { unsafe { let res = libc::pthread_rwlock_rdlock(self.inner().as_ptr()); if res != 0 { - panic!("rdlock failed with {res}"); + panic!("rdlock failed with {}", Errno::from_raw(res)); } } } @@ -59,7 +61,7 @@ unsafe impl lock_api::RawRwLock for PthreadRwLock { match res { 0 => true, libc::EAGAIN => false, - o => panic!("try_rdlock failed with {o}") + o => panic!("try_rdlock failed with {}", Errno::from_raw(res)), } } } @@ -68,7 +70,7 @@ unsafe impl lock_api::RawRwLock for PthreadRwLock { unsafe { let res = libc::pthread_rwlock_wrlock(self.inner().as_ptr()); if res != 0 { - panic!("wrlock failed with {res}"); + panic!("wrlock failed with {}", Errno::from_raw(res)); } } } @@ -79,7 +81,7 @@ unsafe impl lock_api::RawRwLock for PthreadRwLock { match res { 0 => true, libc::EAGAIN => false, - o => panic!("try_wrlock failed with {o}") + o => panic!("try_wrlock failed with {}", Errno::from_raw(res)), } } } @@ -88,7 +90,7 @@ unsafe impl lock_api::RawRwLock for PthreadRwLock { unsafe { let res = libc::pthread_rwlock_unlock(self.inner().as_ptr()); if res != 0 { - panic!("unlock failed with {res}"); + panic!("unlock failed with {}", Errno::from_raw(res)); } } } @@ -96,7 +98,7 @@ unsafe impl lock_api::RawRwLock for PthreadRwLock { unsafe { let res = libc::pthread_rwlock_unlock(self.inner().as_ptr()); if res != 0 { - panic!("unlock failed with {res}"); + panic!("unlock failed with {}", Errno::from_raw(res)); } } }