From 5c9f168fdafd6aeaf0591098fa1176eff814766a Mon Sep 17 00:00:00 2001 From: Levi Morrison Date: Thu, 13 Nov 2025 17:57:49 -0700 Subject: [PATCH 01/20] feat(profiling): parallel set and string set --- Cargo.lock | 2 + libdd-profiling/Cargo.toml | 2 + .../src/profiles/collections/arc.rs | 311 +++++++++++++++ .../src/profiles/collections/error.rs | 2 + .../src/profiles/collections/mod.rs | 4 + .../src/profiles/collections/parallel/mod.rs | 12 + .../src/profiles/collections/parallel/set.rs | 180 +++++++++ .../profiles/collections/parallel/sharded.rs | 188 +++++++++ .../collections/parallel/slice_set.rs | 369 ++++++++++++++++++ .../collections/parallel/string_set.rs | 232 +++++++++++ .../src/profiles/collections/set.rs | 8 +- 11 files changed, 1306 insertions(+), 4 deletions(-) create mode 100644 libdd-profiling/src/profiles/collections/arc.rs create mode 100644 libdd-profiling/src/profiles/collections/parallel/mod.rs create mode 100644 libdd-profiling/src/profiles/collections/parallel/set.rs create mode 100644 libdd-profiling/src/profiles/collections/parallel/sharded.rs create mode 100644 libdd-profiling/src/profiles/collections/parallel/slice_set.rs create mode 100644 libdd-profiling/src/profiles/collections/parallel/string_set.rs diff --git a/Cargo.lock b/Cargo.lock index 9c6b210fe7..46b4cc026b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2776,6 +2776,7 @@ dependencies = [ "bytes", "chrono", "criterion", + "crossbeam-utils", "futures", "hashbrown 0.16.0", "http", @@ -2788,6 +2789,7 @@ dependencies = [ "libdd-profiling-protobuf", "lz4_flex", "mime", + "parking_lot", "proptest", "prost", "rustc-hash 1.1.0", diff --git a/libdd-profiling/Cargo.toml b/libdd-profiling/Cargo.toml index c9d1938ad4..bc8eb70928 100644 --- a/libdd-profiling/Cargo.toml +++ b/libdd-profiling/Cargo.toml @@ -27,6 +27,7 @@ bitmaps = "3.2.0" byteorder = { version = "1.5", features = ["std"] } bytes = "1.1" chrono = {version = "0.4", default-features = false, features = ["std", "clock"]} +crossbeam-utils = { version = "0.8.21" } libdd-alloc = { version = "1.0.0", path = "../libdd-alloc" } libdd-profiling-protobuf = { version = "1.0.0", path = "../libdd-profiling-protobuf", features = ["prost_impls"] } libdd-common = { version = "1.0.0", path = "../libdd-common" } @@ -39,6 +40,7 @@ hyper-multipart-rfc7578 = "0.9.0" indexmap = "2.11" lz4_flex = { version = "0.9", default-features = false, features = ["std", "safe-encode", "frame"] } mime = "0.3.16" +parking_lot = { version = "0.12", default-features = false } prost = "0.13.5" rustc-hash = { version = "1.1", default-features = false } serde = {version = "1.0", features = ["derive"]} diff --git a/libdd-profiling/src/profiles/collections/arc.rs b/libdd-profiling/src/profiles/collections/arc.rs new file mode 100644 index 0000000000..a0a7c8f445 --- /dev/null +++ b/libdd-profiling/src/profiles/collections/arc.rs @@ -0,0 +1,311 @@ +// Copyright 2025-Present Datadog, Inc. https://www.datadoghq.com/ +// SPDX-License-Identifier: Apache-2.0 +// This is heavily inspired by the standard library's `Arc` implementation, +// which is dual-licensed as Apache-2.0 or MIT. + +use allocator_api2::alloc::{AllocError, Allocator, Global}; +use allocator_api2::boxed::Box; +use core::sync::atomic::{fence, AtomicUsize, Ordering}; +use core::{alloc::Layout, fmt, mem::ManuallyDrop, ptr}; +use core::{marker::PhantomData, ops::Deref, ptr::NonNull}; +use crossbeam_utils::CachePadded; + +/// A thread-safe reference-counting pointer with only strong references. +/// +/// This type is similar to `std::sync::Arc` but intentionally omits APIs that +/// can panic or abort the process. In particular: +/// - There are no weak references. +/// - Cloning uses [`Arc::try_clone`], which returns an error on reference-count overflow instead of +/// aborting the process. +/// - Construction uses fallible allocation via [`Arc::try_new`]. +/// +/// Deref gives shared access to the inner value; mutation should use interior +/// mutability primitives as with `std::sync::Arc`. +#[repr(C)] +#[derive(Debug)] +pub struct Arc { + ptr: NonNull>, + alloc: A, + phantom: PhantomData>, +} + +// repr(C) prevents field reordering that could affect raw-pointer helpers. +#[repr(C)] +struct ArcInner { + refcount: CachePadded, + data: CachePadded, +} + +impl ArcInner { + fn from_ptr<'a>(ptr: *const T) -> &'a Self { + let data = ptr.cast::(); + let data_offset = Arc::::data_offset(); + let bytes_ptr = unsafe { data.sub(data_offset) }; + let arc_ptr = bytes_ptr as *mut ArcInner; + unsafe { &*arc_ptr } + } + + fn try_clone(&self) -> Result<(), ArcOverflow> { + if self.refcount.fetch_add(1, Ordering::Relaxed) > MAX_REFCOUNT { + self.refcount.fetch_sub(1, Ordering::Relaxed); + return Err(ArcOverflow); + } + Ok(()) + } +} + +impl Arc { + pub fn try_new(data: T) -> Result, AllocError> { + Self::try_new_in(data, Global) + } + + /// Tries to increment the reference count using only a pointer to the + /// inner `T`. This does not create an `Arc` instance. + /// + /// # Safety + /// - `ptr` must be a valid pointer to the `T` inside an `Arc` allocation produced by this + /// module. Passing any other pointer is undefined behavior. + /// - There must be at least one existing reference alive when called. + pub unsafe fn try_increment_count(ptr: *const T) -> Result<(), ArcOverflow> { + let inner = ArcInner::from_ptr(ptr); + inner.try_clone() + } +} + +impl Arc { + /// Constructs a new `Arc` in the provided allocator, returning an + /// error if allocation fails. + pub fn try_new_in(data: T, alloc: A) -> Result, AllocError> { + let inner = ArcInner { + refcount: CachePadded::new(AtomicUsize::new(1)), + data: CachePadded::new(data), + }; + let boxed = Box::try_new_in(inner, alloc)?; + let (ptr, alloc) = Box::into_non_null(boxed); + Ok(Arc { + ptr, + alloc, + phantom: PhantomData, + }) + } + + /// Returns the inner value, if the `Arc` has exactly one reference. + /// + /// Otherwise, an [`Err`] is returned with the same `Arc` that was passed + /// in. + /// + /// It is strongly recommended to use [`Arc::into_inner`] instead if you + /// don't keep the `Arc` in the [`Err`] case. + pub fn try_unwrap(this: Self) -> Result { + // Attempt to take unique ownership by transitioning strong: 1 -> 0 + let inner_ref = unsafe { this.ptr.as_ref() }; + if inner_ref + .refcount + .compare_exchange(1, 0, Ordering::Acquire, Ordering::Relaxed) + .is_ok() + { + // We have unique ownership; move out T and deallocate without dropping T. + let this = ManuallyDrop::new(this); + let ptr = this.ptr.as_ptr(); + let alloc: A = unsafe { ptr::read(&this.alloc) }; + // Reconstruct a Box to ArcInner and convert into inner to avoid double-drop of T + let boxed: Box, A> = unsafe { Box::from_raw_in(ptr, alloc) }; + let ArcInner { refcount: _, data } = Box::into_inner(boxed); + // We moved out `data` above, so do not use `data` here; free already done via + // into_inner + Ok(CachePadded::into_inner(data)) + } else { + Err(this) + } + } + + pub fn into_inner(this: Self) -> Option { + // Prevent running Drop; we will manage the refcount and allocation manually. + let this = ManuallyDrop::new(this); + let inner = unsafe { this.ptr.as_ref() }; + if inner.refcount.fetch_sub(1, Ordering::Release) != 1 { + return None; + } + fence(Ordering::Acquire); + + // We are the last strong reference. Move out T and free the allocation + // without dropping T. + let ptr = this.ptr.as_ptr(); + let alloc: A = unsafe { ptr::read(&this.alloc) }; + let boxed: Box, A> = unsafe { Box::from_raw_in(ptr, alloc) }; + let ArcInner { refcount: _, data } = Box::into_inner(boxed); + Some(CachePadded::into_inner(data)) + } + + /// Returns a raw non-null pointer to the inner value. The pointer is valid + /// as long as there is at least one strong reference alive. + #[inline] + pub fn as_ptr(&self) -> NonNull { + let ptr = NonNull::as_ptr(self.ptr); + // SAFETY: `ptr` points to a valid `ArcInner` allocation. Taking the + // address of the `data` field preserves provenance unlike going + // through Deref. + let data = unsafe { ptr::addr_of_mut!((*ptr).data) }; + // SAFETY: data field address is derived from a valid NonNull. + unsafe { NonNull::new_unchecked(data as *mut T) } + } + + /// Converts the Arc into a non-null pointer to the inner value, without + /// decreasing the reference count. + /// + /// The caller must later call `Arc::from_raw` with the same pointer exactly + /// once to avoid leaking the allocation. + #[inline] + #[must_use = "losing the pointer will leak memory"] + pub fn into_raw(this: Self) -> NonNull { + let this = ManuallyDrop::new(this); + // Reuse as_ptr logic without dropping `this`. + Arc::as_ptr(&this) + } +} + +// SAFETY: `Arc` is Send and Sync iff `T` is Send and Sync. +unsafe impl Send for Arc {} +unsafe impl Sync for Arc {} + +impl Arc { + #[inline] + fn inner(&self) -> &ArcInner { + // SAFETY: `ptr` is a valid, live allocation managed by this Arc + unsafe { self.ptr.as_ref() } + } +} + +/// Error returned when the reference count would overflow. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct ArcOverflow; + +impl fmt::Display for ArcOverflow { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.write_str("arc: reference count overflow") + } +} + +impl core::error::Error for ArcOverflow {} + +/// A limit on the amount of references that may be made to an `Arc`. +const MAX_REFCOUNT: usize = isize::MAX as usize; + +impl Arc { + /// Fallible clone that increments the strong reference count. + /// + /// Returns an error if the reference count would exceed `isize::MAX`. + pub fn try_clone(&self) -> Result { + let inner = self.inner(); + inner.try_clone()?; + Ok(Arc { + ptr: self.ptr, + alloc: self.alloc.clone(), + phantom: PhantomData, + }) + } +} + +impl Drop for Arc { + fn drop(&mut self) { + let inner = self.inner(); + if inner.refcount.fetch_sub(1, Ordering::Release) == 1 { + // Synchronize with other threads that might have modified the data + // before dropping the last strong reference. + // Raymond Chen wrote a little blog article about it: + // https://devblogs.microsoft.com/oldnewthing/20251015-00/?p=111686 + fence(Ordering::Acquire); + // SAFETY: this was the last strong reference; reclaim allocation + let ptr = self.ptr.as_ptr(); + // Move out allocator for deallocation, but prevent double-drop of `alloc` + let alloc: A = unsafe { ptr::read(&self.alloc) }; + unsafe { drop(Box::, A>::from_raw_in(ptr, alloc)) }; + } + } +} + +impl Deref for Arc { + type Target = T; + + fn deref(&self) -> &Self::Target { + // SAFETY: The allocation outlives `self` while any strong refs exist. + unsafe { &self.ptr.as_ref().data } + } +} + +impl Arc { + #[inline] + fn data_offset() -> usize { + // Layout of ArcInner is repr(C): [CachePadded, CachePadded] + let header = Layout::new::>(); + match header.extend(Layout::new::>()) { + Ok((_layout, offset)) => offset, + Err(_) => { + // Fallback: compute padding manually to avoid unwrap. This should + // not fail in practice for valid types. + let align = Layout::new::>().align(); + let size = header.size(); + let padding = (align - (size % align)) % align; + size + padding + } + } + } + + /// Recreates an `Arc` from a raw pointer produced by `Arc::into_raw`. + /// + /// # Safety + /// - `ptr` must have been returned by a previous call to `Arc::::into_raw`. + /// - if `ptr` has been cast, it needs to be to a compatible repr. + /// - It must not be used to create multiple owning `Arc`s without corresponding `into_raw` + /// calls; otherwise the refcount will be decremented multiple times. + #[inline] + pub unsafe fn from_raw_in(ptr: NonNull, alloc: A) -> Self { + let data = ptr.as_ptr() as *const u8; + let arc_ptr_u8 = data.sub(Self::data_offset()); + let arc_ptr = arc_ptr_u8 as *mut ArcInner; + Arc { + ptr: NonNull::new_unchecked(arc_ptr), + alloc, + phantom: PhantomData, + } + } +} + +impl Arc { + /// Recreates an `Arc` in the `Global` allocator from a raw pointer + /// produced by `Arc::into_raw`. + /// + /// # Safety + /// See [`Arc::from_raw_in`] for requirements. + #[inline] + pub unsafe fn from_raw(ptr: NonNull) -> Self { + Arc::from_raw_in(ptr, Global) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn try_new_and_unwrap_unique() { + let arc = Arc::try_new(123u32).unwrap(); + let v = Arc::try_unwrap(arc).ok().unwrap(); + assert_eq!(v, 123); + } + + #[test] + fn try_unwrap_fails_when_shared() { + let arc = Arc::try_new(5usize).unwrap(); + let arc2 = arc.try_clone().unwrap(); + assert!(Arc::try_unwrap(arc).is_err()); + assert_eq!(*arc2, 5); + } + + #[test] + fn deref_access() { + let arc = Arc::try_new("abc").unwrap(); + assert_eq!(arc.len(), 3); + assert_eq!(*arc, "abc"); + } +} diff --git a/libdd-profiling/src/profiles/collections/error.rs b/libdd-profiling/src/profiles/collections/error.rs index c35b772a26..57d3da1bef 100644 --- a/libdd-profiling/src/profiles/collections/error.rs +++ b/libdd-profiling/src/profiles/collections/error.rs @@ -8,6 +8,8 @@ pub enum SetError { InvalidArgument, #[error("set error: out of memory")] OutOfMemory, + #[error("set error: reference count overflow")] + ReferenceCountOverflow, } impl From for SetError { diff --git a/libdd-profiling/src/profiles/collections/mod.rs b/libdd-profiling/src/profiles/collections/mod.rs index 514402f7c4..bfce216e5d 100644 --- a/libdd-profiling/src/profiles/collections/mod.rs +++ b/libdd-profiling/src/profiles/collections/mod.rs @@ -1,7 +1,9 @@ // Copyright 2025-Present Datadog, Inc. https://www.datadoghq.com/ // SPDX-License-Identifier: Apache-2.0 +mod arc; mod error; +mod parallel; mod set; mod slice_set; mod string_set; @@ -9,7 +11,9 @@ mod thin_str; pub type SetHasher = core::hash::BuildHasherDefault; +pub use arc::*; pub use error::*; +pub use parallel::*; pub use set::*; pub use slice_set::*; pub use string_set::*; diff --git a/libdd-profiling/src/profiles/collections/parallel/mod.rs b/libdd-profiling/src/profiles/collections/parallel/mod.rs new file mode 100644 index 0000000000..4c32464df8 --- /dev/null +++ b/libdd-profiling/src/profiles/collections/parallel/mod.rs @@ -0,0 +1,12 @@ +// Copyright 2025-Present Datadog, Inc. https://www.datadoghq.com/ +// SPDX-License-Identifier: Apache-2.0 + +mod set; +mod sharded; +mod slice_set; +mod string_set; + +pub use set::*; +pub use sharded::*; +pub use slice_set::*; +pub use string_set::*; diff --git a/libdd-profiling/src/profiles/collections/parallel/set.rs b/libdd-profiling/src/profiles/collections/parallel/set.rs new file mode 100644 index 0000000000..b4e2e4e544 --- /dev/null +++ b/libdd-profiling/src/profiles/collections/parallel/set.rs @@ -0,0 +1,180 @@ +// Copyright 2025-Present Datadog, Inc. https://www.datadoghq.com/ +// SPDX-License-Identifier: Apache-2.0 + +use crate::profiles::collections::parallel::sharded::Sharded; +use crate::profiles::collections::{Arc, Set, SetError, SetId, SET_MIN_CAPACITY}; +use core::hash; +use libdd_alloc::Global; +use std::ffi::c_void; +use std::hash::BuildHasher; +use std::ptr; + +#[derive(Debug)] +#[repr(C)] +pub struct ParallelSet { + pub(crate) storage: Arc, N>>, +} + +impl ParallelSet { + const fn is_power_of_two_gt1() -> bool { + N.is_power_of_two() && N > 1 + } + + pub fn try_new() -> Result { + if !Self::is_power_of_two_gt1() { + return Err(SetError::InvalidArgument); + } + let storage = Sharded::, N>::try_new_with_min_capacity(SET_MIN_CAPACITY)?; + let storage = Arc::try_new(storage)?; + Ok(Self { storage }) + } + + #[inline] + fn storage(&self) -> &Sharded, N> { + &self.storage + } + + pub fn try_clone(&self) -> Result { + let storage = self + .storage + .try_clone() + .map_err(|_| SetError::ReferenceCountOverflow)?; + Ok(Self { storage }) + } + + #[inline] + fn select_shard(hash: u64) -> usize { + (hash as usize) & (N - 1) + } + + pub fn try_insert(&self, value: T) -> Result, SetError> { + let hash = crate::profiles::collections::SetHasher::default().hash_one(&value); + let idx = Self::select_shard(hash); + let lock = &self.storage().shards[idx]; + + let read_len = { + let guard = lock.read(); + // SAFETY: `hash` was computed using this set's hasher over `&value`. + if let Some(id) = unsafe { guard.find_with_hash(hash, &value) } { + return Ok(id); + } + guard.len() + }; + + let mut guard = lock.write(); + if guard.len() != read_len { + // SAFETY: `hash` was computed using this set's hasher over `&value`. + if let Some(id) = unsafe { guard.find_with_hash(hash, &value) } { + return Ok(id); + } + } + + // SAFETY: `hash` was computed using this set's hasher over `&value`, + // and uniqueness has been enforced by the preceding read/write checks. + unsafe { guard.insert_unique_uncontended_with_hash(hash, value) } + .map_err(|_| SetError::OutOfMemory) + } + + /// Returns the `SetId` for `value` if it exists in the parallel set, without inserting. + /// Intended for tests and debugging; typical usage should prefer `try_insert`. + pub fn find(&self, value: &T) -> Option> { + let hash = crate::profiles::collections::SetHasher::default().hash_one(value); + let idx = Self::select_shard(hash); + let lock = &self.storage().shards[idx]; + let guard = lock.read(); + // SAFETY: `hash` computed using this set's hasher over `&value`. + unsafe { guard.find_with_hash(hash, value) } + } + + /// Returns a shared reference to the value for a given `SetId`. + /// + /// # Safety + /// - The `id` must have been obtained from this exact `ParallelSet` (and shard) instance, and + /// thus point to a live `T` stored in its arena. + /// # Safety + /// - `id` must come from this exact `ParallelSet` instance (same shard) and still refer to a + /// live element in its arena. + /// - The returned reference is immutable; do not concurrently mutate the same element via + /// interior mutability. + pub unsafe fn get(&self, id: SetId) -> &T { + // We do not need to lock to read the value; storage is arena-backed and + // values are immutable once inserted. Caller guarantees `id` belongs here. + unsafe { id.0.as_ref() } + } + + pub fn into_raw(self) -> ptr::NonNull { + Arc::into_raw(self.storage).cast() + } + + /// # Safety + /// - `this` must be produced by `into_raw` for a `ParallelSet` with matching `T`, `N`, + /// and allocator. + /// - After calling, do not use the original raw pointer again. + pub unsafe fn from_raw(this: ptr::NonNull) -> Self { + let storage = unsafe { Arc::from_raw_in(this.cast(), Global) }; + Self { storage } + } +} + +// SAFETY: uses `RwLock>` to synchronize access. All reads/writes in +// this wrapper go through those locks. All non-mut methods of +// `ParallelSetStorage` and `Set` are safe to call under a read-lock, and all +// mut methods are safe to call under a write-lock. +unsafe impl Send for ParallelSet {} +unsafe impl Sync for ParallelSet {} + +#[cfg(test)] +mod tests { + use super::*; + use proptest::prelude::*; + use std::collections::HashSet as StdHashSet; + + proptest! { + #![proptest_config(ProptestConfig { + cases: if cfg!(miri) { 4 } else { 64 }, + .. ProptestConfig::default() + })] + + #[test] + fn proptest_parallel_set_matches_std_hashset( + values in proptest::collection::vec(any::(), 0..if cfg!(miri) { 32 } else { 512 }) + ) { + type PSet = ParallelSet; + let set = PSet::try_new().unwrap(); + let mut shadow = StdHashSet::::new(); + + for v in &values { + shadow.insert(*v); + let _ = set.try_insert(*v).unwrap(); + } + + // Compare lengths + let len_pset = { + let s = set.storage(); + let mut acc = 0usize; + for shard in &s.shards { acc += shard.read().len(); } + acc + }; + prop_assert_eq!(len_pset, shadow.len()); + + // Each shadow value must be present and equal + for &v in &shadow { + let id = set.find(&v); + prop_assert!(id.is_some()); + let id = id.unwrap(); + // SAFETY: id just obtained from this set + let fetched = unsafe { set.get(id) }; + prop_assert_eq!(*fetched, v); + } + } + } + + #[test] + fn auto_traits_send_sync() { + fn require_send() {} + fn require_sync() {} + type PSet = ParallelSet; + require_send::(); + require_sync::(); + } +} diff --git a/libdd-profiling/src/profiles/collections/parallel/sharded.rs b/libdd-profiling/src/profiles/collections/parallel/sharded.rs new file mode 100644 index 0000000000..c5a08a3f30 --- /dev/null +++ b/libdd-profiling/src/profiles/collections/parallel/sharded.rs @@ -0,0 +1,188 @@ +// Copyright 2025-Present Datadog, Inc. https://www.datadoghq.com/ +// SPDX-License-Identifier: Apache-2.0 + +use crate::profiles::collections::{ + Set, SetError, SetHasher as Hasher, SetId, SliceSet, ThinSlice, +}; +use core::hash::Hash; +use core::mem::MaybeUninit; +use crossbeam_utils::CachePadded; +use parking_lot::RwLock; + +/// Operations a set must provide for so that a sharded set can be built on +/// top of it. +/// +/// # Safety +/// +/// Implementors must ensure that all methods which take `&self` are safe to +/// call under a read-lock, and all `&mut self` methods are safe to call under +/// a write-lock, and are safe for `Send` and `Sync`. +pub unsafe trait SetOps { + type Lookup<'a>: Copy + where + Self: 'a; + + /// Owned payload used for insertion. For some containers (e.g. slice-backed + /// sets) this can be a borrowed view like `&'a [T]` because the container + /// copies data into its own arena during insertion. + type Owned<'a> + where + Self: 'a; + + type Id: Copy; + + fn try_with_capacity(capacity: usize) -> Result + where + Self: Sized; + + fn len(&self) -> usize; + + #[inline] + fn is_empty(&self) -> bool { + self.len() == 0 + } + + /// # Safety + /// Same safety contract as the underlying container's find_with_hash. + unsafe fn find_with_hash(&self, hash: u64, key: Self::Lookup<'_>) -> Option; + + /// # Safety + /// Same safety contract as the underlying container's insert_unique_uncontended_with_hash. + unsafe fn insert_unique_uncontended_with_hash( + &mut self, + hash: u64, + key: Self::Owned<'_>, + ) -> Result; +} + +#[derive(Debug)] +pub struct Sharded { + pub(crate) shards: [CachePadded>; N], +} + +impl Sharded { + #[inline] + pub const fn is_power_of_two_gt1() -> bool { + N.is_power_of_two() && N > 1 + } + + #[inline] + pub fn select_shard(hash: u64) -> usize { + (hash as usize) & (N - 1) + } + + pub fn try_new_with_min_capacity(min_capacity: usize) -> Result { + if !Self::is_power_of_two_gt1() { + return Err(SetError::InvalidArgument); + } + let mut shards_uninit: [MaybeUninit>>; N] = + unsafe { MaybeUninit::uninit().assume_init() }; + let mut i = 0usize; + while i < N { + match S::try_with_capacity(min_capacity) { + Ok(inner) => { + shards_uninit[i].write(CachePadded::new(RwLock::new(inner))); + i += 1; + } + Err(e) => { + for j in (0..i).rev() { + unsafe { shards_uninit[j].assume_init_drop() }; + } + return Err(e); + } + } + } + let shards: [CachePadded>; N] = + unsafe { core::mem::transmute_copy(&shards_uninit) }; + // If N=0, then we error at the very top of the function, so we know + // there's at least one. + Ok(Self { shards }) + } + + pub fn try_insert_common<'a>( + &self, + lookup: S::Lookup<'a>, + owned: S::Owned<'a>, + ) -> Result + where + S::Lookup<'a>: Hash + PartialEq, + { + use std::hash::BuildHasher; + let hash = Hasher::default().hash_one(lookup); + let idx = Self::select_shard(hash); + let lock = &self.shards[idx]; + + let read_len = { + let guard = lock.read(); + if let Some(id) = unsafe { guard.find_with_hash(hash, lookup) } { + return Ok(id); + } + guard.len() + }; + + let mut guard = lock.write(); + if guard.len() != read_len { + if let Some(id) = unsafe { guard.find_with_hash(hash, lookup) } { + return Ok(id); + } + } + + unsafe { guard.insert_unique_uncontended_with_hash(hash, owned) } + } +} + +// SAFETY: relies on safety requirements of `SetOps`. +unsafe impl Send for Sharded {} +unsafe impl Sync for Sharded {} + +unsafe impl SetOps for Set { + type Lookup<'a> = &'a T; + type Owned<'a> = T; + type Id = SetId; + + fn try_with_capacity(capacity: usize) -> Result { + Set::try_with_capacity(capacity) + } + + fn len(&self) -> usize { + self.len() + } + + unsafe fn find_with_hash(&self, hash: u64, key: Self::Lookup<'_>) -> Option { + self.find_with_hash(hash, key) + } + + unsafe fn insert_unique_uncontended_with_hash( + &mut self, + hash: u64, + value: Self::Owned<'_>, + ) -> Result { + self.insert_unique_uncontended_with_hash(hash, value) + } +} + +unsafe impl SetOps for SliceSet { + type Lookup<'a> = &'a [T]; + type Owned<'a> = &'a [T]; + type Id = ThinSlice<'static, T>; + + fn try_with_capacity(capacity: usize) -> Result { + SliceSet::try_with_capacity(capacity) + } + + fn len(&self) -> usize { + self.len() + } + + unsafe fn find_with_hash(&self, hash: u64, key: Self::Lookup<'_>) -> Option { + unsafe { self.find_with_hash(hash, key) } + } + + unsafe fn insert_unique_uncontended_with_hash( + &mut self, + hash: u64, + key: Self::Owned<'_>, + ) -> Result { + unsafe { self.insert_unique_uncontended_with_hash(hash, key) } + } +} diff --git a/libdd-profiling/src/profiles/collections/parallel/slice_set.rs b/libdd-profiling/src/profiles/collections/parallel/slice_set.rs new file mode 100644 index 0000000000..0fedce7915 --- /dev/null +++ b/libdd-profiling/src/profiles/collections/parallel/slice_set.rs @@ -0,0 +1,369 @@ +// Copyright 2025-Present Datadog, Inc. https://www.datadoghq.com/ +// SPDX-License-Identifier: Apache-2.0 + +use crate::profiles::collections::{ + Arc, ArcOverflow, SetError, SetHasher as Hasher, SliceSet, ThinSlice, +}; +use std::hash::{self, BuildHasher}; +use std::ops::Deref; + +/// Number of shards used by the parallel slice set and (by extension) +/// the string-specific parallel set. Kept as a constant so tests and +/// related code can refer to the same value. +pub const N_SHARDS: usize = 16; + +/// The initial capacities for Rust's hash map (and set) currently go +/// like this: 3, 7, 14, 28. We want to avoid some of the smaller sizes so +/// that there's less frequent re-allocation, which is the most expensive +/// part of the set's operations. +const HASH_TABLE_MIN_CAPACITY: usize = 28; + +pub type ParallelSliceStorage = super::Sharded, N_SHARDS>; + +/// A slice set which can have parallel read and write operations. It works +/// by sharding the set and using read-write locks local to each shard. Items +/// cannot be removed from the set; the implementation relies on this for a +/// variety of optimizations. +/// +/// This is a fairly naive implementation. Unfortunately, dashmap and other +/// off-the-shelf implementations I looked at don't have adequate APIs for +/// avoiding panics, including handling allocation failures. Since we're +/// rolling our own, we can get some benefits like having wait-free lookups +/// when fetching the value associated to an ID. +/// +/// Also unfortunately, even parking_lot's RwLock doesn't handle allocation +/// failures. But I'm not going to go _that_ far to avoid allocation failures +/// today. We're very unlikely to run out of memory while adding a waiter to +/// its queue, because the amount of memory used is bounded by the number of +/// threads, which is small. +#[repr(transparent)] +pub struct ParallelSliceSet { + pub(crate) arc: Arc>, +} + +// SAFETY: uses `RwLock>` to synchronize access. All reads/writes +// in this wrapper go through those locks. All non-mut methods of +// `ParallelSliceStorage` and `Set` are safe to call under a read-lock, and all +// mut methods are safe to call under a write-lock. +unsafe impl Send for ParallelSliceSet {} +unsafe impl Sync for ParallelSliceSet {} + +impl Deref for ParallelSliceSet { + type Target = ParallelSliceStorage; + fn deref(&self) -> &Self::Target { + &self.arc + } +} + +impl ParallelSliceSet { + pub fn try_clone(&self) -> Result, ArcOverflow> { + let ptr = self.arc.try_clone().map_err(|_| ArcOverflow)?; + Ok(ParallelSliceSet { arc: ptr }) + } + + pub const fn select_shard(hash: u64) -> usize { + // Use lower bits for shard selection to avoid interfering with + // Swiss tables' internal SIMD comparisons that use upper 7 bits. + // Using 4 bits provides resilience against hash function deficiencies + // and optimal scaling for low thread counts. + (hash & 0b1111) as usize + } + + /// Tries to create a new parallel slice set. + pub fn try_new() -> Result { + let storage = ParallelSliceStorage::try_new_with_min_capacity(HASH_TABLE_MIN_CAPACITY)?; + let ptr = Arc::try_new(storage)?; + Ok(Self { arc: ptr }) + } + + /// # Safety + /// The slice must not have been inserted yet, as it skips checking if + /// the slice is already present. + pub unsafe fn insert_unique_uncontended( + &self, + slice: &[T], + ) -> Result, SetError> + where + T: hash::Hash, + { + let hash = Hasher::default().hash_one(slice); + let shard_idx = Self::select_shard(hash); + let lock = &self.shards[shard_idx]; + let mut guard = lock.write(); + guard.insert_unique_uncontended(slice) + } + + /// Adds the slice to the slice set if it isn't present already, and + /// returns a handle to the slice that can be used to retrieve it later. + pub fn try_insert(&self, slice: &[T]) -> Result, SetError> + where + T: hash::Hash + PartialEq, + { + // Hash once and reuse it for all operations. + // Do this without holding any locks. + let hash = Hasher::default().hash_one(slice); + let shard_idx = Self::select_shard(hash); + let lock = &self.shards[shard_idx]; + + let read_len = { + let guard = lock.read(); + // SAFETY: the slice's hash is correct, we use the same hasher as + // SliceSet uses. + if let Some(id) = unsafe { guard.deref().find_with_hash(hash, slice) } { + return Ok(id); + } + guard.len() + }; + + let mut write_guard = lock.write(); + let write_len = write_guard.slices.len(); + // This is an ABA defense. It's simple because we only support insert. + if write_len != read_len { + // SAFETY: the slice's hash is correct, we use the same hasher as + // SliceSet uses. + if let Some(id) = unsafe { write_guard.find_with_hash(hash, slice) } { + return Ok(id); + } + } + + // SAFETY: we just checked above that the slice isn't in the set. + let id = unsafe { write_guard.insert_unique_uncontended_with_hash(hash, slice)? }; + Ok(id) + } +} + +#[cfg(test)] +mod tests { + use crate::profiles::collections::string_set::{StringRef, UnsyncStringSet}; + use std::collections::hash_map::DefaultHasher; + use std::hash::{Hash, Hasher}; + + /// A test struct representing a function with file and function names. + /// This tests that the generic slice infrastructure works with composite types. + #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] + struct Function { + file_name: StringRef, + function_name: StringRef, + } + + impl Function { + fn new(file_name: StringRef, function_name: StringRef) -> Self { + Self { + file_name, + function_name, + } + } + } + + #[test] + fn test_function_deduplication() { + // Create string set for the string data + let mut string_set = UnsyncStringSet::try_new().unwrap(); + + // Create some strings + let file1 = string_set.try_insert("src/main.rs").unwrap(); + let file2 = string_set.try_insert("src/lib.rs").unwrap(); + let func1 = string_set.try_insert("main").unwrap(); + let func2 = string_set.try_insert("process_data").unwrap(); + + // Create function objects + let fn1 = Function::new(file1, func1); // main in src/main.rs + let fn2 = Function::new(file1, func2); // process_data in src/main.rs + let fn3 = Function::new(file2, func1); // main in src/lib.rs + let fn4 = Function::new(file1, func1); // main in src/main.rs (duplicate of fn1) + + // Test that the functions are equal/different as expected + assert_eq!(fn1, fn4, "Same function should be equal"); + assert_ne!(fn1, fn2, "Different functions should not be equal"); + assert_ne!(fn1, fn3, "Different functions should not be equal"); + assert_ne!(fn2, fn3, "Different functions should not be equal"); + + // Test that we can distinguish them by their components + unsafe { + assert_eq!(string_set.get_string(fn1.file_name), "src/main.rs"); + assert_eq!(string_set.get_string(fn1.function_name), "main"); + assert_eq!(string_set.get_string(fn2.function_name), "process_data"); + assert_eq!(string_set.get_string(fn3.file_name), "src/lib.rs"); + } + } + + #[test] + fn auto_traits_send_sync() { + fn require_send() {} + fn require_sync() {} + require_send::>(); + require_sync::>(); + } + + #[test] + fn test_function_hashing() { + let mut string_set = UnsyncStringSet::try_new().unwrap(); + + let file1 = string_set.try_insert("src/main.rs").unwrap(); + let func1 = string_set.try_insert("main").unwrap(); + let func2 = string_set.try_insert("process_data").unwrap(); + + let fn1 = Function::new(file1, func1); + let fn2 = Function::new(file1, func2); + let fn1_copy = Function::new(file1, func1); + + // Test hash consistency + let hash1 = { + let mut hasher = DefaultHasher::new(); + fn1.hash(&mut hasher); + hasher.finish() + }; + + let hash1_copy = { + let mut hasher = DefaultHasher::new(); + fn1_copy.hash(&mut hasher); + hasher.finish() + }; + + let hash2 = { + let mut hasher = DefaultHasher::new(); + fn2.hash(&mut hasher); + hasher.finish() + }; + + // Same function should have same hash + assert_eq!(hash1, hash1_copy, "Same function should hash consistently"); + + // Different functions should have different hashes (with high probability) + assert_ne!( + hash1, hash2, + "Different functions should have different hashes" + ); + } + + #[test] + fn test_function_composition() { + let mut string_set = UnsyncStringSet::try_new().unwrap(); + + let file1 = string_set.try_insert("src/utils.rs").unwrap(); + let func1 = string_set.try_insert("calculate_hash").unwrap(); + + let function = Function::new(file1, func1); + + // Test that we can access the components + assert_eq!(function.file_name, file1); + assert_eq!(function.function_name, func1); + + // Test that the string data is preserved + unsafe { + assert_eq!(string_set.get_string(function.file_name), "src/utils.rs"); + assert_eq!( + string_set.get_string(function.function_name), + "calculate_hash" + ); + } + } + + #[test] + fn test_many_functions() { + let mut string_set = UnsyncStringSet::try_new().unwrap(); + + // Create a variety of file and function names + let files = [ + "src/main.rs", + "src/lib.rs", + "src/utils.rs", + "src/parser.rs", + "src/codegen.rs", + ]; + let functions = [ + "main", "new", "process", "parse", "generate", "validate", "cleanup", "init", + ]; + + let mut file_ids = Vec::new(); + let mut func_ids = Vec::new(); + + // Create string IDs + for &file in &files { + file_ids.push(string_set.try_insert(file).unwrap()); + } + for &func in &functions { + func_ids.push(string_set.try_insert(func).unwrap()); + } + + let mut functions_created = Vec::new(); + + // Create many function combinations + for &file_id in &file_ids { + for &func_id in &func_ids { + let function = Function::new(file_id, func_id); + functions_created.push(function); + } + } + + // Should have files.len() * functions.len() unique functions + assert_eq!(functions_created.len(), files.len() * functions.len()); + + // Test that all functions are different (no duplicates) + for i in 0..functions_created.len() { + for j in i + 1..functions_created.len() { + assert_ne!( + functions_created[i], functions_created[j], + "All function combinations should be different" + ); + } + } + + // Test that we can retrieve the original strings + for function in &functions_created { + unsafe { + let file_str = string_set.get_string(function.file_name); + let func_str = string_set.get_string(function.function_name); + + // Verify the strings are in our original arrays + assert!( + files.contains(&file_str), + "File name should be from our test set" + ); + assert!( + functions.contains(&func_str), + "Function name should be from our test set" + ); + } + } + } + + #[test] + fn test_function_edge_cases() { + let mut string_set = UnsyncStringSet::try_new().unwrap(); + + // Test with empty strings + let empty_file = string_set.try_insert("").unwrap(); + let empty_func = string_set.try_insert("").unwrap(); + let normal_file = string_set.try_insert("normal.rs").unwrap(); + let normal_func = string_set.try_insert("normal_function").unwrap(); + + let fn1 = Function::new(empty_file, empty_func); // Both empty + let fn2 = Function::new(empty_file, normal_func); // Empty file, normal function + let fn3 = Function::new(normal_file, empty_func); // Normal file, empty function + let fn4 = Function::new(normal_file, normal_func); // Both normal + + // All should be different + let functions = [fn1, fn2, fn3, fn4]; + for i in 0..functions.len() { + for j in i + 1..functions.len() { + assert_ne!( + functions[i], functions[j], + "Functions with different components should not be equal" + ); + } + } + + // Test that we can retrieve the correct strings + unsafe { + assert_eq!(string_set.get_string(fn1.file_name), ""); + assert_eq!(string_set.get_string(fn1.function_name), ""); + assert_eq!(string_set.get_string(fn2.file_name), ""); + assert_eq!(string_set.get_string(fn2.function_name), "normal_function"); + assert_eq!(string_set.get_string(fn3.file_name), "normal.rs"); + assert_eq!(string_set.get_string(fn3.function_name), ""); + assert_eq!(string_set.get_string(fn4.file_name), "normal.rs"); + assert_eq!(string_set.get_string(fn4.function_name), "normal_function"); + } + } +} diff --git a/libdd-profiling/src/profiles/collections/parallel/string_set.rs b/libdd-profiling/src/profiles/collections/parallel/string_set.rs new file mode 100644 index 0000000000..edee55dc77 --- /dev/null +++ b/libdd-profiling/src/profiles/collections/parallel/string_set.rs @@ -0,0 +1,232 @@ +// Copyright 2025-Present Datadog, Inc. https://www.datadoghq.com/ +// SPDX-License-Identifier: Apache-2.0 + +use crate::profiles::collections::parallel::slice_set::{ParallelSliceSet, ParallelSliceStorage}; +use crate::profiles::collections::{Arc, ArcOverflow, SetError, StringRef, WELL_KNOWN_STRING_REFS}; +use core::ptr; +use std::ffi::c_void; +use std::ops::Deref; + +/// A string set which can have parallel read and write operations. +/// This is a newtype wrapper around ParallelSliceSet that adds +/// string-specific functionality like well-known strings. +#[repr(transparent)] +pub struct ParallelStringSet { + pub(crate) inner: ParallelSliceSet, +} + +impl Deref for ParallelStringSet { + type Target = ParallelSliceSet; + fn deref(&self) -> &Self::Target { + &self.inner + } +} + +impl ParallelStringSet { + /// Consumes the `ParallelStringSet`, returning a non-null pointer to the + /// inner storage. This storage should not be mutated--it only exists to + /// be passed across FFI boundaries, which is why its type has been erased. + #[inline] + pub fn into_raw(self) -> ptr::NonNull { + Arc::into_raw(self.inner.arc).cast() + } + + /// Recreates a `ParallelStringSet` from a raw pointer produced by + /// [`ParallelStringSet::into_raw`]. + /// + /// # Safety + /// + /// The pointer must have been produced by [`ParallelStringSet::into_raw`] + /// and be returned unchanged. + #[inline] + pub unsafe fn from_raw(raw: ptr::NonNull) -> Self { + let arc = Arc::from_raw(raw.cast::>()); + Self { + inner: ParallelSliceSet { arc }, + } + } + + pub fn try_clone(&self) -> Result { + Ok(ParallelStringSet { + inner: self.inner.try_clone()?, + }) + } + + /// Tries to create a new parallel string set that contains the well-known + /// strings, including the empty string. + pub fn try_new() -> Result { + let inner = ParallelSliceSet::try_new()?; + let set = Self { inner }; + + for id in WELL_KNOWN_STRING_REFS.iter() { + // SAFETY: the well-known strings are unique, and we're in the + // constructor where other threads don't have access to it yet. + _ = unsafe { set.insert_unique_uncontended(id.0.deref())? }; + } + Ok(set) + } + + /// # Safety + /// The string must not have been inserted yet, as it skips checking if + /// the string is already present. + pub unsafe fn insert_unique_uncontended(&self, str: &str) -> Result { + let thin_slice = self.inner.insert_unique_uncontended(str.as_bytes())?; + Ok(StringRef(thin_slice.into())) + } + + /// Adds the string to the string set if it isn't present already, and + /// returns a handle to the string that can be used to retrieve it later. + pub fn try_insert(&self, str: &str) -> Result { + let thin_slice = self.inner.try_insert(str.as_bytes())?; + Ok(StringRef(thin_slice.into())) + } + + /// Selects which shard a hash should go to (0-3 for 4 shards). + pub fn select_shard(hash: u64) -> usize { + ParallelSliceSet::::select_shard(hash) + } + + /// # Safety + /// The caller must ensure that the StringId is valid for this set. + pub unsafe fn get(&self, id: StringRef) -> &str { + // SAFETY: safe as long as caller respects this function's safety. + unsafe { core::mem::transmute::<&str, &str>(id.0.deref()) } + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::profiles::collections::parallel::slice_set::N_SHARDS; + use crate::profiles::collections::SetHasher as Hasher; + use std::hash::BuildHasher; + + #[test] + fn test_well_known_strings() { + let strs: [&str; WELL_KNOWN_STRING_REFS.len()] = [ + "", + "end_timestamp_ns", + "local root span id", + "trace endpoint", + "span id", + ]; + for (expected, id) in strs.iter().copied().zip(WELL_KNOWN_STRING_REFS) { + let actual: &str = id.0.deref(); + assert_eq!(expected, actual); + } + + let mut selected = [0; WELL_KNOWN_STRING_REFS.len()]; + for (id, dst) in WELL_KNOWN_STRING_REFS.iter().zip(selected.iter_mut()) { + *dst = ParallelStringSet::select_shard(Hasher::default().hash_one(id.0.deref())); + } + } + + #[test] + fn test_parallel_set() { + let set = ParallelStringSet::try_new().unwrap(); + // SAFETY: these are all well-known strings. + unsafe { + let str = set.get(StringRef::EMPTY); + assert_eq!(str, ""); + + let str = set.get(StringRef::END_TIMESTAMP_NS); + assert_eq!(str, "end_timestamp_ns"); + + let str = set.get(StringRef::LOCAL_ROOT_SPAN_ID); + assert_eq!(str, "local root span id"); + + let str = set.get(StringRef::TRACE_ENDPOINT); + assert_eq!(str, "trace endpoint"); + + let str = set.get(StringRef::SPAN_ID); + assert_eq!(str, "span id"); + }; + + let id = set.try_insert("").unwrap(); + assert_eq!(&*id.0, &*StringRef::EMPTY.0); + + let id = set.try_insert("end_timestamp_ns").unwrap(); + assert_eq!(&*id.0, &*StringRef::END_TIMESTAMP_NS.0); + + let id = set.try_insert("local root span id").unwrap(); + assert_eq!(&*id.0, &*StringRef::LOCAL_ROOT_SPAN_ID.0); + + let id = set.try_insert("trace endpoint").unwrap(); + assert_eq!(&*id.0, &*StringRef::TRACE_ENDPOINT.0); + + let id = set.try_insert("span id").unwrap(); + assert_eq!(&*id.0, &*StringRef::SPAN_ID.0); + } + + #[test] + fn test_hash_distribution() { + let test_strings: Vec = (0..100).map(|i| format!("test_string_{}", i)).collect(); + + let mut shard_counts = [0; N_SHARDS]; + + for s in &test_strings { + let hash = Hasher::default().hash_one(s); + let shard = ParallelStringSet::select_shard(hash); + shard_counts[shard] += 1; + } + + // Verify that distribution is not completely degenerate + // (both shards should get at least some strings) + assert!(shard_counts[0] > 0, "Shard 0 got no strings"); + assert!(shard_counts[1] > 0, "Shard 1 got no strings"); + + // Print distribution for manual inspection + println!("Shard distribution: {:?}", shard_counts); + } + + #[test] + fn test_parallel_set_shard_selection() { + let set = ParallelStringSet::try_new().unwrap(); + + // Test with realistic strings that would appear in profiling + let test_strings = [ + // .NET method signatures + "System.String.Concat(System.Object)", + "Microsoft.Extensions.DependencyInjection.ServiceProvider.GetService(System.Type)", + "System.Text.Json.JsonSerializer.Deserialize(System.String)", + "MyNamespace.MyClass.MyMethod(Int32 id, String name)", + // File paths and URLs + "/usr/lib/x86_64-linux-gnu/libc.so.6", + "/var/run/datadog/apm.socket", + "https://api.datadoghq.com/api/v1/traces", + "/home/user/.local/share/applications/myapp.desktop", + "C:\\Program Files\\MyApp\\bin\\myapp.exe", + // Short common strings + "f", + "g", + ]; + + let mut ids = Vec::new(); + for &test_str in &test_strings { + let id = set.try_insert(test_str).unwrap(); + ids.push((test_str, id)); + } + + // Verify all strings can be retrieved correctly + for (original_str, id) in ids { + unsafe { + assert_eq!(set.get(id), original_str); + } + } + + // Test that inserting the same strings again returns the same IDs + for &test_str in &test_strings { + let id1 = set.try_insert(test_str).unwrap(); + let id2 = set.try_insert(test_str).unwrap(); + assert_eq!(&*id1.0, &*id2.0); + } + } + + #[test] + fn auto_traits_send_sync() { + fn require_send() {} + fn require_sync() {} + require_send::(); + require_sync::(); + } +} diff --git a/libdd-profiling/src/profiles/collections/set.rs b/libdd-profiling/src/profiles/collections/set.rs index ef378e9da0..6504f0c0f0 100644 --- a/libdd-profiling/src/profiles/collections/set.rs +++ b/libdd-profiling/src/profiles/collections/set.rs @@ -61,7 +61,7 @@ pub struct Set { } impl Set { - const SIZE_HINT: usize = 1024 * 1024; + pub const SIZE_HINT: usize = 1024 * 1024; pub fn try_new() -> Result { Self::try_with_capacity(SET_MIN_CAPACITY) @@ -145,7 +145,7 @@ impl Drop for Set { } impl Set { - fn try_with_capacity(capacity: usize) -> Result { + pub(crate) fn try_with_capacity(capacity: usize) -> Result { let arena = ChainAllocator::new_in(Self::SIZE_HINT, VirtualAllocator {}); let mut table = HashTable::new(); @@ -154,7 +154,7 @@ impl Set { Ok(Self { arena, table }) } - unsafe fn find_with_hash(&self, hash: u64, key: &T) -> Option> { + pub(crate) unsafe fn find_with_hash(&self, hash: u64, key: &T) -> Option> { let found = self .table // SAFETY: NonNull inside table points to live, properly aligned Ts. @@ -162,7 +162,7 @@ impl Set { Some(SetId(*found)) } - unsafe fn insert_unique_uncontended_with_hash( + pub(crate) unsafe fn insert_unique_uncontended_with_hash( &mut self, hash: u64, value: T, From 2f6bdcf3740887f8b63bfb60490a8f7fc7592872 Mon Sep 17 00:00:00 2001 From: Levi Morrison Date: Fri, 14 Nov 2025 11:05:20 -0700 Subject: [PATCH 02/20] test: exercise thread safety --- .../collections/parallel/string_set.rs | 105 ++++++++++++++++++ 1 file changed, 105 insertions(+) diff --git a/libdd-profiling/src/profiles/collections/parallel/string_set.rs b/libdd-profiling/src/profiles/collections/parallel/string_set.rs index edee55dc77..232cd874f4 100644 --- a/libdd-profiling/src/profiles/collections/parallel/string_set.rs +++ b/libdd-profiling/src/profiles/collections/parallel/string_set.rs @@ -229,4 +229,109 @@ mod tests { require_send::(); require_sync::(); } + + #[test] + fn test_thread_safety() { + use std::sync::{Arc, Barrier}; + use std::thread; + + // Create set and add some strings before sharing across threads + let set = ParallelStringSet::try_new().unwrap(); + let pre_inserted_strings = vec![ + "pre_inserted_1", + "pre_inserted_2", + "pre_inserted_3", + "pre_inserted_4", + ]; + let mut pre_inserted_ids = Vec::new(); + for s in &pre_inserted_strings { + let id = set.try_insert(s).unwrap(); + pre_inserted_ids.push((s.to_string(), id)); + } + + // Share the set across threads using try_clone (which clones the internal Arc) + let num_threads = 4; + let operations_per_thread = 50; + + // Keep a clone of the original set for final verification + let original_set = set.try_clone().unwrap(); + + // Create a barrier to ensure all threads start work simultaneously + let barrier = Arc::new(Barrier::new(num_threads)); + + // Spawn threads that will both read pre-existing strings and insert new ones + let handles: Vec<_> = (0..num_threads) + .map(|thread_id| { + let set = original_set.try_clone().unwrap(); + let pre_ids = pre_inserted_ids.clone(); + let barrier = Arc::clone(&barrier); + thread::spawn(move || { + // Wait for all threads to be spawned before starting work + barrier.wait(); + + // Read pre-existing strings (should be safe to read concurrently) + for (expected_str, id) in &pre_ids { + unsafe { + let actual_str = set.get(*id); + assert_eq!( + actual_str, + expected_str.as_str(), + "Pre-inserted string should be readable" + ); + } + } + + // Concurrently insert new strings + for i in 0..operations_per_thread { + let new_str = format!("thread_{}_string_{}", thread_id, i); + let id = set.try_insert(&new_str).unwrap(); + unsafe { + let retrieved = set.get(id); + assert_eq!(retrieved, new_str, "Inserted string should be retrievable"); + } + } + + // Try inserting strings that other threads might have inserted + for i in 0..operations_per_thread { + let shared_str = format!("shared_string_{}", i); + let id1 = set.try_insert(&shared_str).unwrap(); + let id2 = set.try_insert(&shared_str).unwrap(); + // Both should return the same ID (deduplication) + assert_eq!(&*id1.0, &*id2.0, "Duplicate inserts should return same ID"); + unsafe { + assert_eq!(set.get(id1), shared_str); + } + } + }) + }) + .collect(); + + // Wait for all threads to complete + for handle in handles { + handle.join().expect("Thread should not panic"); + } + + // Verify final state: all pre-inserted strings should still be readable + for (expected_str, id) in &pre_inserted_ids { + unsafe { + let actual_str = original_set.get(*id); + assert_eq!( + actual_str, + expected_str.as_str(), + "Pre-inserted strings should remain readable after concurrent operations" + ); + } + } + + // Verify that shared strings inserted by multiple threads are deduplicated + for i in 0..operations_per_thread { + let shared_str = format!("shared_string_{}", i); + let id1 = original_set.try_insert(&shared_str).unwrap(); + let id2 = original_set.try_insert(&shared_str).unwrap(); + assert_eq!( + &*id1.0, &*id2.0, + "Shared strings should be deduplicated correctly" + ); + } + } } From 7e5ae4b70684657ccf3e81f1aac7f72a6bfda01a Mon Sep 17 00:00:00 2001 From: Levi Morrison Date: Fri, 14 Nov 2025 15:24:28 -0700 Subject: [PATCH 03/20] feat(profiling): add ProfilesDictionary --- .../collections/parallel/string_set.rs | 2 +- .../src/profiles/datatypes/function.rs | 76 +++++++ .../src/profiles/datatypes/mapping.rs | 84 ++++++++ libdd-profiling/src/profiles/datatypes/mod.rs | 12 ++ .../profiles/datatypes/profiles_dictionary.rs | 193 ++++++++++++++++++ .../src/profiles/datatypes/string.rs | 53 +++++ libdd-profiling/src/profiles/mod.rs | 1 + 7 files changed, 420 insertions(+), 1 deletion(-) create mode 100644 libdd-profiling/src/profiles/datatypes/function.rs create mode 100644 libdd-profiling/src/profiles/datatypes/mapping.rs create mode 100644 libdd-profiling/src/profiles/datatypes/mod.rs create mode 100644 libdd-profiling/src/profiles/datatypes/profiles_dictionary.rs create mode 100644 libdd-profiling/src/profiles/datatypes/string.rs diff --git a/libdd-profiling/src/profiles/collections/parallel/string_set.rs b/libdd-profiling/src/profiles/collections/parallel/string_set.rs index 232cd874f4..ceab5d5f17 100644 --- a/libdd-profiling/src/profiles/collections/parallel/string_set.rs +++ b/libdd-profiling/src/profiles/collections/parallel/string_set.rs @@ -87,7 +87,7 @@ impl ParallelStringSet { } /// # Safety - /// The caller must ensure that the StringId is valid for this set. + /// The caller must ensure that the StringRef is valid for this set. pub unsafe fn get(&self, id: StringRef) -> &str { // SAFETY: safe as long as caller respects this function's safety. unsafe { core::mem::transmute::<&str, &str>(id.0.deref()) } diff --git a/libdd-profiling/src/profiles/datatypes/function.rs b/libdd-profiling/src/profiles/datatypes/function.rs new file mode 100644 index 0000000000..8e37829a84 --- /dev/null +++ b/libdd-profiling/src/profiles/datatypes/function.rs @@ -0,0 +1,76 @@ +// Copyright 2025-Present Datadog, Inc. https://www.datadoghq.com/ +// SPDX-License-Identifier: Apache-2.0 + +use crate::profiles::collections::StringRef; +use crate::profiles::datatypes::StringId2; + +/// A representation of a function that is an intersection of the Otel and +/// Pprof representations. Omits the start line to save space because Datadog +/// doesn't use this in any way. +#[derive(Copy, Clone, Debug, Default, Eq, PartialEq, Hash)] +#[repr(C)] +pub struct Function { + pub name: StringRef, + pub system_name: StringRef, + pub file_name: StringRef, +} + +/// An FFI-safe version of the Function. +#[repr(C)] +#[derive(Copy, Clone, Debug, Default)] +pub struct Function2 { + pub name: StringId2, + pub system_name: StringId2, + pub file_name: StringId2, +} + +impl From for Function2 { + fn from(f: Function) -> Function2 { + Function2 { + name: f.name.into(), + system_name: f.system_name.into(), + file_name: f.file_name.into(), + } + } +} + +impl From for Function { + fn from(f2: Function2) -> Function { + Function { + name: f2.name.into(), + system_name: f2.system_name.into(), + file_name: f2.file_name.into(), + } + } +} + +#[derive(Clone, Copy, Debug)] +#[repr(transparent)] +pub struct FunctionId2(pub(crate) *mut Function2); + +// todo: when MSRV is 1.88.0+, derive Default +impl Default for FunctionId2 { + fn default() -> Self { + Self(core::ptr::null_mut()) + } +} + +impl FunctionId2 { + pub fn is_empty(self) -> bool { + self.0.is_null() + } + + /// Converts the `FunctionId2` into an `Option` where an empty + /// `FunctionId2` converts to a `None`. + /// + /// # Safety + /// The pointer object must still be alive. In general this means the + /// profiles dictionary it came from must be alive. + pub unsafe fn read(self) -> Option { + if self.is_empty() { + None + } else { + Some(self.0.read()) + } + } +} diff --git a/libdd-profiling/src/profiles/datatypes/mapping.rs b/libdd-profiling/src/profiles/datatypes/mapping.rs new file mode 100644 index 0000000000..941bb3ac0a --- /dev/null +++ b/libdd-profiling/src/profiles/datatypes/mapping.rs @@ -0,0 +1,84 @@ +// Copyright 2025-Present Datadog, Inc. https://www.datadoghq.com/ +// SPDX-License-Identifier: Apache-2.0 + +use crate::profiles::collections::StringRef; +use crate::profiles::datatypes::StringId2; + +/// A representation of a mapping that is an intersection of the Otel and Pprof +/// representations. Omits boolean attributes because Datadog doesn't use them +/// in any way. +#[repr(C)] +#[derive(Copy, Clone, Debug, Default, Eq, PartialEq, Hash)] +pub struct Mapping { + pub memory_start: u64, + pub memory_limit: u64, + pub file_offset: u64, + pub filename: StringRef, + pub build_id: StringRef, // missing in Otel, is it made into an attribute? +} + +/// An FFI-safe version of the Mapping. +#[repr(C)] +#[derive(Copy, Clone, Debug, Default)] +pub struct Mapping2 { + pub memory_start: u64, + pub memory_limit: u64, + pub file_offset: u64, + pub filename: StringId2, + pub build_id: StringId2, // missing in Otel, is it made into an attribute? +} + +impl From for Mapping { + fn from(m2: Mapping2) -> Self { + Self { + memory_start: m2.memory_start, + memory_limit: m2.memory_limit, + file_offset: m2.file_offset, + filename: m2.filename.into(), + build_id: m2.build_id.into(), + } + } +} + +impl From for Mapping2 { + fn from(m: Mapping) -> Self { + Self { + memory_start: m.memory_start, + memory_limit: m.memory_limit, + file_offset: m.file_offset, + filename: m.filename.into(), + build_id: m.build_id.into(), + } + } +} + +#[derive(Clone, Copy, Debug)] +#[repr(transparent)] +pub struct MappingId2(pub(crate) *mut Mapping2); + +// todo: when MSRV is 1.88.0+, derive Default +impl Default for MappingId2 { + fn default() -> Self { + Self(core::ptr::null_mut()) + } +} + +impl MappingId2 { + pub fn is_empty(self) -> bool { + self.0.is_null() + } + + /// Converts the `MappingId2` into an `Option` where an empty + /// `MappingId2` converts to a `None`. + /// + /// # Safety + /// The pointer object must still be alive. In general this means the + /// profiles dictionary it came from must be alive. + pub unsafe fn read(self) -> Option { + if self.is_empty() { + None + } else { + Some(self.0.read()) + } + } +} diff --git a/libdd-profiling/src/profiles/datatypes/mod.rs b/libdd-profiling/src/profiles/datatypes/mod.rs new file mode 100644 index 0000000000..9706609582 --- /dev/null +++ b/libdd-profiling/src/profiles/datatypes/mod.rs @@ -0,0 +1,12 @@ +// Copyright 2025-Present Datadog, Inc. https://www.datadoghq.com/ +// SPDX-License-Identifier: Apache-2.0 + +mod function; +mod mapping; +mod profiles_dictionary; +mod string; + +pub use function::*; +pub use mapping::*; +pub use profiles_dictionary::*; +pub use string::*; diff --git a/libdd-profiling/src/profiles/datatypes/profiles_dictionary.rs b/libdd-profiling/src/profiles/datatypes/profiles_dictionary.rs new file mode 100644 index 0000000000..f2ada4ef24 --- /dev/null +++ b/libdd-profiling/src/profiles/datatypes/profiles_dictionary.rs @@ -0,0 +1,193 @@ +// Copyright 2025-Present Datadog, Inc. https://www.datadoghq.com/ +// SPDX-License-Identifier: Apache-2.0 + +use crate::profiles::collections::{ParallelSet, ParallelStringSet, SetError, SetId}; +use crate::profiles::datatypes::{ + Function, Function2, FunctionId2, Mapping, Mapping2, MappingId2, StringId2, +}; + +pub type FunctionSet = ParallelSet; +pub type MappingSet = ParallelSet; + +pub struct ProfilesDictionary { + functions: FunctionSet, + mappings: MappingSet, + strings: ParallelStringSet, +} + +impl ProfilesDictionary { + /// Creates a new dictionary, returning an error if it cannot allocate + /// memory for one of its member sets. + pub fn try_new() -> Result { + let dictionary = ProfilesDictionary { + functions: ParallelSet::try_new()?, + mappings: ParallelSet::try_new()?, + strings: ParallelStringSet::try_new()?, + }; + dictionary.mappings.try_insert(Mapping::default())?; + dictionary.functions.try_insert(Function::default())?; + Ok(dictionary) + } + + /// Adds the function to the function set in the dictionary, and returns a + /// `FunctionId2` which represents it. Returns an error if it cannot + /// allocate memory. + pub fn try_insert_function2(&self, function: Function2) -> Result { + let function = Function { + name: function.name.into(), + system_name: function.system_name.into(), + file_name: function.file_name.into(), + }; + let set_id = self.functions.try_insert(function)?; + + // SAFETY: the function that SetId points to is layout compatible with + // the one that FunctionId2 points to. The reverse is not true for the + // null StringId cases. + let function_id = unsafe { core::mem::transmute::, FunctionId2>(set_id) }; + Ok(function_id) + } + + /// Adds the mapping to the mapping set in the dictionary, and returns a + /// `MappingId2` which represents it. Returns an error if it cannot + /// allocate memory. + pub fn try_insert_mapping2(&self, mapping: Mapping2) -> Result { + let mapping = Mapping { + memory_start: mapping.memory_start, + memory_limit: mapping.memory_limit, + file_offset: mapping.file_offset, + filename: mapping.filename.into(), + build_id: mapping.build_id.into(), + }; + let set_id = self.mappings.try_insert(mapping)?; + + // SAFETY: the mapping that SetId points to is layout compatible with + // the one that MappingId2 points to. The reverse is not true for the + // null StringId cases. + let mapping_id = unsafe { core::mem::transmute::, MappingId2>(set_id) }; + Ok(mapping_id) + } + + /// Adds the string to the string set in the dictionary, and returns a + /// `StringId2` which represents it. Returns an error if it cannot + /// allocate memory. + pub fn try_insert_str2(&self, str: &str) -> Result { + // Choosing not to add a fast-path for the empty string. It falls out + // naturally, and if the caller feels it's useful for their use-case, + // then they can always wrap this to do it. + let string_ref = self.strings.try_insert(str)?; + Ok(string_ref.into()) + } + + pub fn functions(&self) -> &FunctionSet { + &self.functions + } + + pub fn mappings(&self) -> &MappingSet { + &self.mappings + } + + pub fn strings(&self) -> &ParallelStringSet { + &self.strings + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::profiles::collections::StringRef; + use proptest::prelude::*; + + #[track_caller] + fn assert_string_id_eq(a: StringId2, b: StringId2) { + assert_eq!(StringRef::from(a), StringRef::from(b)); + } + + fn assert_string_value(dict: &ProfilesDictionary, id: StringId2, expected: &str) { + unsafe { + assert_eq!(dict.strings().get(id.into()), expected); + } + } + + proptest! { + #![proptest_config(ProptestConfig { + cases: if cfg!(miri) { 8 } else { 64 }, + ..ProptestConfig::default() + })] + + #[test] + fn proptest_function_round_trip_and_deduplication( + name in ".*", + system_name in ".*", + file_name in ".*", + ) { + let dict = ProfilesDictionary::try_new().unwrap(); + + // Insert strings + let name_id = dict.try_insert_str2(&name).unwrap(); + let system_name_id = dict.try_insert_str2(&system_name).unwrap(); + let file_name_id = dict.try_insert_str2(&file_name).unwrap(); + + let function2 = Function2 { name: name_id, system_name: system_name_id, file_name: file_name_id }; + + // Test insert and read back (exercises unsafe transmute) + let id1 = dict.try_insert_function2(function2).unwrap(); + prop_assert!(!id1.is_empty()); + + let read1 = unsafe { id1.read() }.unwrap(); + assert_string_id_eq(read1.name, name_id); + assert_string_id_eq(read1.system_name, system_name_id); + assert_string_id_eq(read1.file_name, file_name_id); + + // Test deduplication + let id2 = dict.try_insert_function2(function2).unwrap(); + prop_assert_eq!(id1.0, id2.0); + + // Test round-trip conversion + let function: Function = read1.into(); + assert_string_value(&dict, function.name.into(), &name); + assert_string_value(&dict, function.system_name.into(), &system_name); + assert_string_value(&dict, function.file_name.into(), &file_name); + } + + #[test] + fn proptest_mapping_round_trip_and_deduplication( + memory_start in 0..u64::MAX, + memory_limit in 0..u64::MAX, + file_offset in 0..u64::MAX, + filename in ".*", + build_id in ".*", + ) { + let dict = ProfilesDictionary::try_new().unwrap(); + + let filename_id = dict.try_insert_str2(&filename).unwrap(); + let build_id_id = dict.try_insert_str2(&build_id).unwrap(); + + let mapping2 = Mapping2 { + memory_start, + memory_limit, + file_offset, + filename: filename_id, + build_id: build_id_id, + }; + + // Test insert and read back (exercises unsafe transmute) + let id1 = dict.try_insert_mapping2(mapping2).unwrap(); + prop_assert!(!id1.is_empty()); + + let read1 = unsafe { id1.read() }.unwrap(); + prop_assert_eq!(read1.memory_start, memory_start); + prop_assert_eq!(read1.memory_limit, memory_limit); + prop_assert_eq!(read1.file_offset, file_offset); + assert_string_id_eq(read1.filename, filename_id); + assert_string_id_eq(read1.build_id, build_id_id); + + // Test deduplication + let id2 = dict.try_insert_mapping2(mapping2).unwrap(); + prop_assert_eq!(id1.0, id2.0); + + // Verify string values + assert_string_value(&dict, read1.filename, &filename); + assert_string_value(&dict, read1.build_id, &build_id); + } + } +} diff --git a/libdd-profiling/src/profiles/datatypes/string.rs b/libdd-profiling/src/profiles/datatypes/string.rs new file mode 100644 index 0000000000..9260f8144a --- /dev/null +++ b/libdd-profiling/src/profiles/datatypes/string.rs @@ -0,0 +1,53 @@ +// Copyright 2025-Present Datadog, Inc. https://www.datadoghq.com/ +// SPDX-License-Identifier: Apache-2.0 + +use crate::profiles::collections::StringRef; + +/// An FFI-safe string ID where a null StringId2 maps to `StringRef::EMPTY`. +#[repr(transparent)] +#[derive(Clone, Copy, Debug)] +pub struct StringId2(*mut StringHeader); + +/// Represents a pointer to a string's header. Its definition is intentionally obscured. +#[derive(Clone, Copy, Debug)] +#[repr(C)] +pub struct StringHeader(u8); + +unsafe impl Send for StringId2 {} + +unsafe impl Sync for StringId2 {} + +// todo: when MSRV is 1.88.0+, derive Default +impl Default for StringId2 { + fn default() -> Self { + Self::EMPTY + } +} + +impl StringId2 { + pub const EMPTY: StringId2 = StringId2(core::ptr::null_mut()); + + pub fn is_empty(&self) -> bool { + self.0.is_null() + } +} + +impl From for StringId2 { + fn from(s: StringRef) -> Self { + // SAFETY: every StringRef is a valid StringId2 (but not the other way + // because of null). + unsafe { core::mem::transmute::(s) } + } +} + +impl From for StringRef { + fn from(id: StringId2) -> Self { + if id.0.is_null() { + StringRef::EMPTY + } else { + // SAFETY: every non-null StringId2 is supposed to be a valid + // StringRef. + unsafe { core::mem::transmute::(id) } + } + } +} diff --git a/libdd-profiling/src/profiles/mod.rs b/libdd-profiling/src/profiles/mod.rs index 24780566b1..2ed0ebe8ea 100644 --- a/libdd-profiling/src/profiles/mod.rs +++ b/libdd-profiling/src/profiles/mod.rs @@ -3,5 +3,6 @@ pub mod collections; mod compressor; +pub mod datatypes; pub use compressor::*; From 2f8c573aff46f982740d697603c73074a6d6efa8 Mon Sep 17 00:00:00 2001 From: Levi Morrison Date: Mon, 17 Nov 2025 16:41:06 -0700 Subject: [PATCH 04/20] feat(profiling): Profile::try_add_sample2 --- libdd-profiling-ffi/src/lib.rs | 6 +- libdd-profiling-ffi/src/profiles/datatypes.rs | 3 +- libdd-profiling/benches/add_samples.rs | 169 ++++++++++++++++++ libdd-profiling/benches/main.rs | 3 +- libdd-profiling/src/api2.rs | 78 ++++++++ .../src/internal/owned_types/mod.rs | 28 +++ libdd-profiling/src/internal/profile/mod.rs | 165 ++++++++++++++++- .../profile/profiles_dictionary_translator.rs | 119 ++++++++++++ libdd-profiling/src/lib.rs | 1 + libdd-profiling/src/profiles/datatypes/mod.rs | 2 + .../src/profiles/datatypes/value_type.rs | 17 ++ 11 files changed, 583 insertions(+), 8 deletions(-) create mode 100644 libdd-profiling/benches/add_samples.rs create mode 100644 libdd-profiling/src/api2.rs create mode 100644 libdd-profiling/src/internal/profile/profiles_dictionary_translator.rs create mode 100644 libdd-profiling/src/profiles/datatypes/value_type.rs diff --git a/libdd-profiling-ffi/src/lib.rs b/libdd-profiling-ffi/src/lib.rs index 1a820e033c..62ea6890a3 100644 --- a/libdd-profiling-ffi/src/lib.rs +++ b/libdd-profiling-ffi/src/lib.rs @@ -7,13 +7,13 @@ #![cfg_attr(not(test), deny(clippy::todo))] #![cfg_attr(not(test), deny(clippy::unimplemented))] -#[cfg(all(feature = "symbolizer", not(target_os = "windows")))] -pub use symbolizer_ffi::*; - mod exporter; mod profiles; mod string_storage; +#[cfg(all(feature = "symbolizer", not(target_os = "windows")))] +pub use symbolizer_ffi::*; + // re-export crashtracker ffi #[cfg(feature = "crashtracker-ffi")] pub use libdd_crashtracker_ffi::*; diff --git a/libdd-profiling-ffi/src/profiles/datatypes.rs b/libdd-profiling-ffi/src/profiles/datatypes.rs index 97f40c3476..51f4142e5a 100644 --- a/libdd-profiling-ffi/src/profiles/datatypes.rs +++ b/libdd-profiling-ffi/src/profiles/datatypes.rs @@ -6,8 +6,7 @@ use anyhow::Context; use function_name::named; use libdd_common_ffi::slice::{AsBytes, ByteSlice, CharSlice, Slice}; use libdd_common_ffi::{wrap_with_ffi_result, Error, Handle, Timespec, ToInner}; -use libdd_profiling::api; -use libdd_profiling::api::ManagedStringId; +use libdd_profiling::api::{self, ManagedStringId}; use libdd_profiling::internal; use std::num::NonZeroI64; use std::str::Utf8Error; diff --git a/libdd-profiling/benches/add_samples.rs b/libdd-profiling/benches/add_samples.rs new file mode 100644 index 0000000000..ae4062fde1 --- /dev/null +++ b/libdd-profiling/benches/add_samples.rs @@ -0,0 +1,169 @@ +// Copyright 2025-Present Datadog, Inc. +// SPDX-License-Identifier: Apache-2.0 + +use criterion::*; +use libdd_profiling::api2::Location2; +use libdd_profiling::profiles::collections::SetId; +use libdd_profiling::profiles::datatypes::{Function, FunctionId2, MappingId2}; +use libdd_profiling::{self as profiling, api, api2}; + +fn make_sample_types() -> Vec> { + vec![api::ValueType::new("samples", "count")] +} + +fn make_stack_api(frames: &[Frame]) -> (Vec>, Vec) { + // No mappings in Ruby, but the v1 API requires it. + let mapping = api::Mapping::default(); + let mut locations = Vec::with_capacity(frames.len()); + for frame in frames { + locations.push(api::Location { + mapping, + function: api::Function { + name: frame.function_name, + filename: frame.function_name, + ..Default::default() + }, + line: frame.line_number as i64, + ..Default::default() + }); + } + let values = vec![1i64]; + (locations, values) +} + +fn make_stack_api2(frames: &[Frame2]) -> (Vec, Vec) { + let mut locations = Vec::with_capacity(frames.len()); + + for frame in frames { + locations.push(Location2 { + mapping: MappingId2::default(), + function: frame.function, + address: 0, + line: frame.line_number as i64, + }); + } + + let values = vec![1i64]; + (locations, values) +} + +#[derive(Clone, Copy)] +struct Frame { + file_name: &'static str, + line_number: u32, + function_name: &'static str, +} +impl Frame { + pub const fn new( + file_name: &'static str, + line_number: u32, + function_name: &'static str, + ) -> Self { + Self { + file_name, + line_number, + function_name, + } + } +} + +#[derive(Clone, Copy)] +struct Frame2 { + function: FunctionId2, + line_number: u32, +} + +pub fn bench_add_sample_vs_add2(c: &mut Criterion) { + let sample_types = make_sample_types(); + + // Create dictionary for both sample types and frames + let dict = profiling::profiles::datatypes::ProfilesDictionary::try_new().unwrap(); + + // Convert sample_types to ValueType2 by inserting strings into dictionary + let sample_types2: Vec<_> = sample_types + .iter() + .map(|st| { + let type_id = dict.try_insert_str2(st.r#type).unwrap(); + let unit_id = dict.try_insert_str2(st.unit).unwrap(); + api2::ValueType2 { type_id, unit_id } + }) + .collect(); + + // This is root-to-leaf, instead of leaf-to-root. We'll reverse it below. + // Taken from a Ruby app, everything here is source-available. + let mut frames = [ + Frame::new("/usr/local/bundle/gems/logging-2.4.0/lib/logging/diagnostic_context.rb", 474, "create_with_logging_context"), + Frame::new("/usr/local/bundle/gems/puma-6.4.3/lib/puma/thread_pool.rb", 155, "spawn_thread"), + Frame::new("/usr/local/bundle/gems/puma-6.4.3/lib/puma/server.rb", 245, "run"), + Frame::new("/usr/local/bundle/gems/puma-6.4.3/lib/puma/server.rb", 464, "process_client"), + Frame::new("/usr/local/bundle/gems/puma-6.4.3/lib/puma/request.rb", 99, "handle_request"), + Frame::new("/usr/local/bundle/gems/puma-6.4.3/lib/puma/thread_pool.rb", 378, "with_force_shutdown"), + Frame::new("/usr/local/bundle/gems/puma-6.4.3/lib/puma/request.rb", 100, "handle_request"), + Frame::new("/usr/local/bundle/gems/puma-6.4.3/lib/puma/configuration.rb", 272, "call"), + Frame::new("/usr/local/bundle/gems/railties-7.0.8.7/lib/rails/engine.rb", 530, "call"), + Frame::new("/usr/local/bundle/gems/datadog-2.18.0/lib/datadog/tracing/contrib/rack/middlewares.rb", 474, "call"), + Frame::new("/usr/local/bundle/gems/datadog-2.18.0/lib/datadog/tracing/contrib/rack/trace_proxy_middleware.rb", 17, "call"), + Frame::new("/usr/local/bundle/gems/datadog-2.18.0/lib/datadog/tracing/contrib/rack/middlewares.rb", 70, "call"), + Frame::new("/usr/local/bundle/gems/datadog-2.18.0/lib/datadog/appsec/contrib/rack/request_middleware.rb", 82, "call"), + Frame::new("/usr/local/lib/libruby.so.3.3", 0, "catch"), + Frame::new("/usr/local/bundle/gems/datadog-2.18.0/lib/datadog/appsec/contrib/rack/request_middleware.rb", 85, "catch"), + Frame::new("/usr/local/bundle/gems/datadog-2.18.0/lib/datadog/appsec/instrumentation/gateway.rb", 41, "push"), + Frame::new("/usr/local/bundle/gems/datadog-2.18.0/lib/datadog/appsec/instrumentation/gateway.rb", 37, "push"), + Frame::new("/usr/local/bundle/gems/datadog-2.18.0/lib/datadog/appsec/instrumentation/gateway/middleware.rb", 18, "call"), + ]; + frames.reverse(); + + let strings = dict.strings(); + let functions = dict.functions(); + + let frames2 = frames.map(|f| { + let set_id = functions + .try_insert(Function { + name: strings.try_insert(f.file_name).unwrap(), + system_name: Default::default(), + file_name: strings.try_insert(f.file_name).unwrap(), + }) + .unwrap(); + Frame2 { + function: unsafe { core::mem::transmute::, FunctionId2>(set_id) }, + line_number: f.line_number, + } + }); + let dict = profiling::profiles::collections::Arc::try_new(dict).unwrap(); + + c.bench_function("profile_add_sample_frames_x1000", |b| { + b.iter(|| { + let mut profile = profiling::internal::Profile::try_new(&sample_types, None).unwrap(); + let (locations, values) = make_stack_api(frames.as_slice()); + for _ in 0..1000 { + let sample = api::Sample { + locations: locations.clone(), + values: &values, + labels: vec![], + }; + black_box(profile.try_add_sample(sample, None)).unwrap(); + } + black_box(profile.only_for_testing_num_aggregated_samples()) + }) + }); + + c.bench_function("profile_add_sample2_frames_x1000", |b| { + b.iter(|| { + let mut profile = profiling::internal::Profile::try_new2( + dict.try_clone().unwrap(), + &sample_types2, + None, + ) + .unwrap(); + let (locations, values) = make_stack_api2(frames2.as_slice()); + for _ in 0..1000 { + // Provide an empty iterator for labels conversion path + let labels_iter = std::iter::empty::>(); + black_box(profile.try_add_sample2(&locations, &values, labels_iter, None)).unwrap(); + } + black_box(profile.only_for_testing_num_aggregated_samples()) + }) + }); +} + +criterion_group!(benches, bench_add_sample_vs_add2); diff --git a/libdd-profiling/benches/main.rs b/libdd-profiling/benches/main.rs index e2d220f767..3c29e7fef6 100644 --- a/libdd-profiling/benches/main.rs +++ b/libdd-profiling/benches/main.rs @@ -3,6 +3,7 @@ use criterion::criterion_main; +mod add_samples; mod interning_strings; -criterion_main!(interning_strings::benches); +criterion_main!(interning_strings::benches, add_samples::benches); diff --git a/libdd-profiling/src/api2.rs b/libdd-profiling/src/api2.rs new file mode 100644 index 0000000000..601e691142 --- /dev/null +++ b/libdd-profiling/src/api2.rs @@ -0,0 +1,78 @@ +// Copyright 2025-Present Datadog, Inc. https://www.datadoghq.com/ +// SPDX-License-Identifier: Apache-2.0 + +use crate::profiles::datatypes::{self, FunctionId2, MappingId2, StringId2}; + +#[derive(Copy, Clone, Debug, Default)] +#[repr(C)] +pub struct Location2 { + pub mapping: MappingId2, + pub function: FunctionId2, + + /// The instruction address for this location, if available. It + /// should be within [Mapping.memory_start...Mapping.memory_limit] + /// for the corresponding mapping. A non-leaf address may be in the + /// middle of a call instruction. It is up to display tools to find + /// the beginning of the instruction if necessary. + pub address: u64, + pub line: i64, +} + +#[derive(Copy, Clone, Debug, Default)] +pub struct Label<'a> { + pub key: StringId2, + + /// At most one of `.str` and `.num` should not be empty. + pub str: &'a str, + pub num: i64, + + /// Should only be present when num is present. + /// Specifies the units of num. + /// Use arbitrary string (for example, "requests") as a custom count unit. + /// If no unit is specified, consumer may apply heuristic to deduce the unit. + /// Consumers may also interpret units like "bytes" and "kilobytes" as memory + /// units and units like "seconds" and "nanoseconds" as time units, + /// and apply appropriate unit conversions to these. + pub num_unit: &'a str, +} + +impl<'a> Label<'a> { + pub const fn str(key: StringId2, str: &'a str) -> Label<'a> { + Label { + key, + str, + num: 0, + num_unit: "", + } + } + + pub const fn num(key: StringId2, num: i64, num_unit: &'a str) -> Label<'a> { + Label { + key, + str: "", + num, + num_unit, + } + } +} +#[repr(C)] +#[derive(Clone, Copy, Debug)] +pub struct ValueType2 { + pub type_id: StringId2, + pub unit_id: StringId2, +} + +impl From for datatypes::ValueType { + fn from(value: ValueType2) -> datatypes::ValueType { + datatypes::ValueType { + type_id: value.type_id.into(), + unit_id: value.unit_id.into(), + } + } +} + +#[derive(Copy, Clone, Debug)] +pub struct Period2 { + pub r#type: ValueType2, + pub value: i64, +} diff --git a/libdd-profiling/src/internal/owned_types/mod.rs b/libdd-profiling/src/internal/owned_types/mod.rs index a828f0639c..9db99f2507 100644 --- a/libdd-profiling/src/internal/owned_types/mod.rs +++ b/libdd-profiling/src/internal/owned_types/mod.rs @@ -2,6 +2,9 @@ // SPDX-License-Identifier: Apache-2.0 use crate::api; +use crate::api2::{Period2, ValueType2}; +use crate::profiles::collections::StringRef; +use std::ops::Deref; #[cfg_attr(test, derive(bolero::generator::TypeGenerator))] #[derive(Clone, Debug)] @@ -20,6 +23,22 @@ impl<'a> From<&'a api::ValueType<'a>> for ValueType { } } +impl From for ValueType { + fn from(value_type2: ValueType2) -> ValueType { + let typ: StringRef = value_type2.type_id.into(); + let unit: StringRef = value_type2.unit_id.into(); + ValueType { + typ: Box::from(typ.0.deref()), + unit: Box::from(unit.0.deref()), + } + } +} +impl From<&ValueType2> for ValueType { + fn from(value_type2: &ValueType2) -> ValueType { + ValueType::from(*value_type2) + } +} + impl<'a> From<&'a ValueType> for api::ValueType<'a> { fn from(value: &'a ValueType) -> Self { Self::new(&value.typ, &value.unit) @@ -42,3 +61,12 @@ impl<'a> From<&'a api::Period<'a>> for Period { } } } + +impl From for Period { + fn from(period2: Period2) -> Period { + Period { + typ: ValueType::from(period2.r#type), + value: 0, + } + } +} diff --git a/libdd-profiling/src/internal/profile/mod.rs b/libdd-profiling/src/internal/profile/mod.rs index 78dbdddf71..247417f756 100644 --- a/libdd-profiling/src/internal/profile/mod.rs +++ b/libdd-profiling/src/internal/profile/mod.rs @@ -5,27 +5,38 @@ mod fuzz_tests; pub mod interning_api; +mod profiles_dictionary_translator; + +pub use profiles_dictionary_translator::*; use self::api::UpscalingInfo; use super::*; -use crate::api; use crate::api::ManagedStringId; +use crate::api2::{Period2, ValueType2}; use crate::collections::identifiable::*; use crate::collections::string_storage::{CachedProfileId, ManagedStringStorage}; use crate::collections::string_table::{self, StringTable}; +use crate::internal::owned_types; use crate::iter::{IntoLendingIterator, LendingIterator}; +use crate::profiles::collections::ArcOverflow; +use crate::profiles::datatypes::ProfilesDictionary; use crate::profiles::{Compressor, DefaultProfileCodec}; +use crate::{api, api2}; use anyhow::Context; use interning_api::Generation; use libdd_profiling_protobuf::{self as protobuf, Record, Value, NO_OPT_ZERO, OPT_ZERO}; use std::borrow::Cow; use std::collections::HashMap; use std::io; +use std::ops::Deref; use std::sync::atomic::AtomicU64; use std::sync::{Arc, Mutex}; use std::time::{Duration, SystemTime}; pub struct Profile { + /// Translates from the new Id2 APIs to the older internal APIs. Long-term, + /// this should probably use the dictionary directly. + profiles_dictionary_translator: Option, /// When profiles are reset, the sample-types need to be preserved. This /// maintains them in a way that does not depend on the string table. The /// Option part is this is taken from the old profile and moved to the new @@ -162,6 +173,88 @@ impl Profile { self.try_add_sample_internal(sample.values, labels, locations, timestamp) } + /// Tries to add a sample using `api2` structures. + pub fn try_add_sample2<'a, L: ExactSizeIterator>>>( + &mut self, + locations: &[api2::Location2], + values: &[i64], + labels: L, + timestamp: Option, + ) -> anyhow::Result<()> { + let Some(translator) = &mut self.profiles_dictionary_translator else { + anyhow::bail!("profiles dictionary not set"); + }; + + // In debug builds, we iterate over the labels twice. That's not + // something the trait bounds support, so we collect into a vector. + // Since this is debug-only, the performance is fine. + #[cfg(debug_assertions)] + let labels = labels.collect::>(); + #[cfg(debug_assertions)] + { + Self::validate_sample_labels2(labels.as_slice())?; + } + + let string_table = &mut self.strings; + let functions = &mut self.functions; + let mappings = &mut self.mappings; + let locations_set = &mut self.locations; + let labels_set = &mut self.labels; + + let labels = { + let mut lbls = Vec::new(); + lbls.try_reserve_exact(labels.len())?; + for label in labels { + let label = label.context("profile label failed to convert")?; + let key = translator.translate_string(string_table, label.key)?; + let internal_label = if !label.str.is_empty() { + let str = string_table.try_intern(label.str)?; + Label::str(key, str) + } else { + let num = label.num; + let num_unit = string_table.try_intern(label.num_unit)?; + Label::num(key, num, num_unit) + }; + + let id = labels_set.try_dedup(internal_label)?; + lbls.push(id); + } + lbls.into_boxed_slice() + }; + + let mut internal_locations = Vec::new(); + internal_locations + .try_reserve_exact(locations.len()) + .context("failed to reserve memory for sample locations")?; + for location in locations { + let l = Location { + mapping_id: translator.translate_mapping( + mappings, + string_table, + location.mapping, + )?, + function_id: translator.translate_function( + functions, + string_table, + location.function, + )?, + address: location.address, + line: location.line, + }; + let location_id = locations_set.checked_dedup(l)?; + internal_locations.push(location_id); + } + + self.try_add_sample_internal(values, labels, internal_locations, timestamp) + } + + /// Gets the profiles dictionary, needed for `api2` operations. + pub fn get_profiles_dictionary(&self) -> Option<&ProfilesDictionary> { + self.profiles_dictionary_translator + .as_ref() + .map(|p| p.profiles_dictionary.deref()) + } + pub fn add_string_id_sample( &mut self, sample: api::StringIdSample, @@ -283,12 +376,15 @@ impl Profile { } /// Tries to create a profile with the given `period`. - /// Initializes the string table to hold: + /// Initializes the string table to hold common strings such as: /// - "" (the empty string) /// - "local root span id" /// - "trace endpoint" + /// - "end_timestamp_ns" /// /// All other fields are default. + /// + /// It's recommended to use [`Profile::try_new2`] instead. pub fn try_new( sample_types: &[api::ValueType], period: Option, @@ -297,6 +393,29 @@ impl Profile { Self::backup_period(period), Self::backup_sample_types(sample_types), None, + None, + ) + } + + /// Tries to create a profile with the given period and sample types. The + /// [`StringId2`]s should belong to the provided [`ProfilesDictionary`]. + #[inline(never)] + #[cold] + pub fn try_new2( + profiles_dictionary: crate::profiles::collections::Arc, + sample_types: &[ValueType2], + period: Option, + ) -> io::Result { + let mut owned_sample_types = Vec::new(); + // Using try_reserve_exact because it will be converted to a Box<[]>, + // so excess capacity would just make that conversion more expensive. + owned_sample_types.try_reserve_exact(sample_types.len())?; + owned_sample_types.extend(sample_types.iter().map(owned_types::ValueType::from)); + Self::try_new_internal( + period.map(owned_types::Period::from), + Some(owned_sample_types.into_boxed_slice()), + None, + Some(ProfilesDictionaryTranslator::new(profiles_dictionary)), ) } @@ -310,6 +429,7 @@ impl Profile { Self::backup_period(period), Self::backup_sample_types(sample_types), Some(string_storage), + None, ) } @@ -323,10 +443,20 @@ impl Profile { "Can't rotate the profile, there are still active samples. Drain them and try again." ); + let profiles_dictionary_translator = self + .profiles_dictionary_translator + .as_ref() + .map(|t| -> Result<_, ArcOverflow> { + let dict = t.profiles_dictionary.try_clone()?; + Ok(ProfilesDictionaryTranslator::new(dict)) + }) + .transpose()?; + let mut profile = Profile::try_new_internal( self.owned_period.take(), self.owned_sample_types.take(), self.string_storage.clone(), + profiles_dictionary_translator, ) .context("failed to initialize new profile")?; @@ -785,9 +915,11 @@ impl Profile { owned_period: Option, owned_sample_types: Option>, string_storage: Option>>, + profiles_dictionary_translator: Option, ) -> io::Result { let start_time = SystemTime::now(); let mut profile = Self { + profiles_dictionary_translator, owned_period, owned_sample_types, active_samples: Default::default(), @@ -905,6 +1037,35 @@ impl Profile { } Ok(()) } + + #[cfg(debug_assertions)] + fn validate_sample_labels2(labels: &[anyhow::Result]) -> anyhow::Result<()> { + use crate::profiles::collections::StringRef; + let mut seen: HashMap = HashMap::with_capacity(labels.len()); + + for label in labels.iter() { + let Ok(label) = label.as_ref() else { + anyhow::bail!("profiling FFI label string failed to convert") + }; + let key = StringRef::from(label.key); + if let Some(duplicate) = seen.insert(key, label) { + anyhow::bail!("Duplicate label on sample: {duplicate:?} {label:?}"); + } + + if key == StringRef::LOCAL_ROOT_SPAN_ID { + anyhow::ensure!( + label.str.is_empty() && label.num != 0, + "Invalid \"local root span id\" label: {label:?}" + ); + } + + anyhow::ensure!( + key != StringRef::END_TIMESTAMP_NS, + "Timestamp should not be passed as a label {label:?}" + ); + } + Ok(()) + } } /// For testing and debugging purposes diff --git a/libdd-profiling/src/internal/profile/profiles_dictionary_translator.rs b/libdd-profiling/src/internal/profile/profiles_dictionary_translator.rs new file mode 100644 index 0000000000..976ed7c26e --- /dev/null +++ b/libdd-profiling/src/internal/profile/profiles_dictionary_translator.rs @@ -0,0 +1,119 @@ +// Copyright 2025-Present Datadog, Inc. https://www.datadoghq.com/ +// SPDX-License-Identifier: Apache-2.0 + +use crate::collections::identifiable::{Dedup, FxIndexMap, StringId}; +use crate::collections::string_table::StringTable; +use crate::internal::{Function, FunctionId, Mapping, MappingId}; +use crate::profiles::collections::{SetId, StringRef}; +use crate::profiles::datatypes::{ + self as dt, FunctionId2, MappingId2, ProfilesDictionary, StringId2, +}; +use indexmap::map::Entry; + +pub struct ProfilesDictionaryTranslator { + pub profiles_dictionary: crate::profiles::collections::Arc, + pub mappings: FxIndexMap, Option>, + pub functions: FxIndexMap, FunctionId>, + pub strings: FxIndexMap, +} + +// SAFETY: the profiles_dictionary keeps the storage for Ids alive. +unsafe impl Send for ProfilesDictionaryTranslator {} + +impl ProfilesDictionaryTranslator { + pub fn new( + profiles_dictionary: crate::profiles::collections::Arc, + ) -> ProfilesDictionaryTranslator { + ProfilesDictionaryTranslator { + profiles_dictionary, + mappings: Default::default(), + functions: Default::default(), + strings: Default::default(), + } + } + + pub fn translate_function( + &mut self, + functions: &mut impl Dedup, + string_table: &mut StringTable, + id2: FunctionId2, + ) -> anyhow::Result { + let function2 = (unsafe { id2.read() }).unwrap_or_default(); + let set_id = unsafe { core::mem::transmute::>(id2) }; + + if let Some(internal) = self.functions.get(&set_id) { + return Ok(*internal); + } + + let (name, system_name, filename) = ( + self.translate_string(string_table, function2.name)?, + self.translate_string(string_table, function2.system_name)?, + self.translate_string(string_table, function2.file_name)?, + ); + let function = Function { + name, + system_name, + filename, + }; + let internal_id = functions.dedup(function); + self.functions.try_reserve(1)?; + self.functions.insert(set_id, internal_id); + Ok(internal_id) + } + + pub fn translate_mapping( + &mut self, + mappings: &mut impl Dedup, + string_table: &mut StringTable, + id2: MappingId2, + ) -> anyhow::Result> { + let Some(mapping2) = (unsafe { id2.read() }) else { + return Ok(None); + }; + let set_id = unsafe { core::mem::transmute::>(id2) }; + + if let Some(internal) = self.mappings.get(&set_id) { + return Ok(*internal); + } + + let filename = self.translate_string(string_table, mapping2.filename)?; + let build_id = self.translate_string(string_table, mapping2.build_id)?; + let mapping = Mapping { + memory_start: mapping2.memory_start, + memory_limit: mapping2.memory_limit, + file_offset: mapping2.file_offset, + filename, + build_id, + }; + let internal_id = mappings.dedup(mapping); + self.mappings.try_reserve(1)?; + self.mappings.insert(set_id, Some(internal_id)); + Ok(Some(internal_id)) + } + + pub fn translate_string( + &mut self, + string_table: &mut StringTable, + id2: StringId2, + ) -> anyhow::Result { + if id2.is_empty() { + return Ok(StringId::ZERO); + } + + let string_ref = StringRef::from(id2); + self.strings.try_reserve(1)?; + match self.strings.entry(string_ref) { + Entry::Occupied(o) => Ok(*o.get()), + Entry::Vacant(v) => { + let str = unsafe { self.profiles_dictionary.strings().get(string_ref) }; + // SAFETY: we're keeping these lifetimes in sync. I think. + // TODO: BUT longer-term we want to avoid copying them + // entirely, so this should go away. + let decouple_str = unsafe { core::mem::transmute::<&str, &str>(str) }; + let internal_id = string_table.try_intern(decouple_str)?; + v.insert(internal_id); + Ok(internal_id) + } + } + } +} diff --git a/libdd-profiling/src/lib.rs b/libdd-profiling/src/lib.rs index 486104e792..01a99d6c44 100644 --- a/libdd-profiling/src/lib.rs +++ b/libdd-profiling/src/lib.rs @@ -7,6 +7,7 @@ #![cfg_attr(not(test), deny(clippy::unimplemented))] pub mod api; +pub mod api2; pub mod collections; pub mod exporter; pub mod internal; diff --git a/libdd-profiling/src/profiles/datatypes/mod.rs b/libdd-profiling/src/profiles/datatypes/mod.rs index 9706609582..45892fc97a 100644 --- a/libdd-profiling/src/profiles/datatypes/mod.rs +++ b/libdd-profiling/src/profiles/datatypes/mod.rs @@ -5,8 +5,10 @@ mod function; mod mapping; mod profiles_dictionary; mod string; +mod value_type; pub use function::*; pub use mapping::*; pub use profiles_dictionary::*; pub use string::*; +pub use value_type::*; diff --git a/libdd-profiling/src/profiles/datatypes/value_type.rs b/libdd-profiling/src/profiles/datatypes/value_type.rs new file mode 100644 index 0000000000..7eb6d0aece --- /dev/null +++ b/libdd-profiling/src/profiles/datatypes/value_type.rs @@ -0,0 +1,17 @@ +// Copyright 2025-Present Datadog, Inc. https://www.datadoghq.com/ +// SPDX-License-Identifier: Apache-2.0 + +use crate::profiles::collections::StringRef; + +#[repr(C)] +#[derive(Clone, Copy, Debug, Eq, Hash, PartialEq)] +pub struct ValueType { + pub type_id: StringRef, + pub unit_id: StringRef, +} + +impl ValueType { + pub fn new(type_id: StringRef, unit_id: StringRef) -> ValueType { + ValueType { type_id, unit_id } + } +} From a43f7bc361695cd271253e94aaf55e0acda8372e Mon Sep 17 00:00:00 2001 From: Levi Morrison Date: Mon, 17 Nov 2025 22:13:44 -0700 Subject: [PATCH 05/20] test: new functions --- libdd-profiling/src/internal/profile/mod.rs | 195 ++++++++++++++++++++ 1 file changed, 195 insertions(+) diff --git a/libdd-profiling/src/internal/profile/mod.rs b/libdd-profiling/src/internal/profile/mod.rs index 247417f756..87056d9f9d 100644 --- a/libdd-profiling/src/internal/profile/mod.rs +++ b/libdd-profiling/src/internal/profile/mod.rs @@ -2704,6 +2704,201 @@ mod api_tests { Ok(()) } + #[test] + #[cfg_attr(miri, ignore)] + fn test_try_new2_and_try_add_sample2() { + struct Frame { + file_name: &'static str, + line_number: u32, + function_name: &'static str, + } + + // Create a ProfilesDictionary with realistic data from Ruby app + let dict = crate::profiles::datatypes::ProfilesDictionary::try_new().unwrap(); + + // Create sample types + let samples_type = dict.try_insert_str2("samples").unwrap(); + let count_unit = dict.try_insert_str2("count").unwrap(); + let sample_types = vec![ValueType2 { + type_id: samples_type, + unit_id: count_unit, + }]; + + // Ruby stack trace (leaf-to-root order) + // Taken from a Ruby app, everything here is source-available + let frames = [ + Frame { file_name: "/usr/local/bundle/gems/datadog-2.18.0/lib/datadog/appsec/instrumentation/gateway/middleware.rb", line_number: 18, function_name: "call" }, + Frame { file_name: "/usr/local/bundle/gems/datadog-2.18.0/lib/datadog/appsec/instrumentation/gateway.rb", line_number: 37, function_name: "push" }, + Frame { file_name: "/usr/local/bundle/gems/datadog-2.18.0/lib/datadog/appsec/instrumentation/gateway.rb", line_number: 41, function_name: "push" }, + Frame { file_name: "/usr/local/bundle/gems/datadog-2.18.0/lib/datadog/appsec/contrib/rack/request_middleware.rb", line_number: 85, function_name: "catch" }, + Frame { file_name: "/usr/local/lib/libruby.so.3.3", line_number: 0, function_name: "catch" }, + Frame { file_name: "/usr/local/bundle/gems/datadog-2.18.0/lib/datadog/appsec/contrib/rack/request_middleware.rb", line_number: 82, function_name: "call" }, + Frame { file_name: "/usr/local/bundle/gems/datadog-2.18.0/lib/datadog/tracing/contrib/rack/middlewares.rb", line_number: 70, function_name: "call" }, + Frame { file_name: "/usr/local/bundle/gems/datadog-2.18.0/lib/datadog/tracing/contrib/rack/trace_proxy_middleware.rb", line_number: 17, function_name: "call" }, + Frame { file_name: "/usr/local/bundle/gems/datadog-2.18.0/lib/datadog/tracing/contrib/rack/middlewares.rb", line_number: 474, function_name: "call" }, + Frame { file_name: "/usr/local/bundle/gems/railties-7.0.8.7/lib/rails/engine.rb", line_number: 530, function_name: "call" }, + Frame { file_name: "/usr/local/bundle/gems/puma-6.4.3/lib/puma/configuration.rb", line_number: 272, function_name: "call" }, + Frame { file_name: "/usr/local/bundle/gems/puma-6.4.3/lib/puma/request.rb", line_number: 100, function_name: "handle_request" }, + Frame { file_name: "/usr/local/bundle/gems/puma-6.4.3/lib/puma/thread_pool.rb", line_number: 378, function_name: "with_force_shutdown" }, + Frame { file_name: "/usr/local/bundle/gems/puma-6.4.3/lib/puma/request.rb", line_number: 99, function_name: "handle_request" }, + Frame { file_name: "/usr/local/bundle/gems/puma-6.4.3/lib/puma/server.rb", line_number: 464, function_name: "process_client" }, + Frame { file_name: "/usr/local/bundle/gems/puma-6.4.3/lib/puma/server.rb", line_number: 245, function_name: "run" }, + Frame { file_name: "/usr/local/bundle/gems/puma-6.4.3/lib/puma/thread_pool.rb", line_number: 155, function_name: "spawn_thread" }, + Frame { file_name: "/usr/local/bundle/gems/logging-2.4.0/lib/logging/diagnostic_context.rb", line_number: 474, function_name: "create_with_logging_context" }, + ]; + + // Create a fake mapping to exercise the code path (Ruby doesn't currently use mappings) + let fake_mapping_filename = dict.try_insert_str2("/usr/lib/ruby.so").unwrap(); + let fake_mapping = crate::profiles::datatypes::Mapping2 { + memory_start: 0x7f0000000000, + memory_limit: 0x7f0000100000, + file_offset: 0, + filename: fake_mapping_filename, + build_id: crate::profiles::datatypes::StringId2::default(), + }; + let mapping_id = dict.try_insert_mapping2(fake_mapping).unwrap(); + + // Create locations from frames + let mut locations = Vec::new(); + for frame in &frames { + let function_name_id = dict.try_insert_str2(frame.function_name).unwrap(); + let filename_id = dict.try_insert_str2(frame.file_name).unwrap(); + let function = crate::profiles::datatypes::Function2 { + name: function_name_id, + system_name: crate::profiles::datatypes::StringId2::default(), + file_name: filename_id, + }; + let function_id = dict.try_insert_function2(function).unwrap(); + + locations.push(api2::Location2 { + mapping: mapping_id, + function: function_id, + address: 0, + line: frame.line_number as i64, + }); + } + + // Wrap in Arc + let dict = crate::profiles::collections::Arc::try_new(dict).unwrap(); + + // Create profile with dictionary + let mut profile = + Profile::try_new2(dict.try_clone().unwrap(), &sample_types, None).unwrap(); + + assert_eq!(profile.only_for_testing_num_aggregated_samples(), 0); + + let values = vec![1i64]; + + // Add sample without labels + let labels_iter = std::iter::empty::>(); + profile + .try_add_sample2(&locations, &values, labels_iter, None) + .expect("add to succeed"); + + assert_eq!(profile.only_for_testing_num_aggregated_samples(), 1); + + // Add same sample again - should aggregate + let labels_iter = std::iter::empty::>(); + profile + .try_add_sample2(&locations, &values, labels_iter, None) + .expect("add to succeed"); + + // Still 1 sample because it aggregated + assert_eq!(profile.only_for_testing_num_aggregated_samples(), 1); + + // Test with labels + let label_key = dict.try_insert_str2("thread_id").unwrap(); + let label_value = "worker-1"; + + let labels_iter = std::iter::once(Ok(api2::Label::str(label_key, label_value))); + profile + .try_add_sample2(&locations, &values, labels_iter, None) + .expect("add with label to succeed"); + + // Should be 2 samples now (different label set) + assert_eq!(profile.only_for_testing_num_aggregated_samples(), 2); + + // Test with numeric label + let thread_id_key = dict.try_insert_str2("thread_id_num").unwrap(); + let labels_iter = std::iter::once(Ok(api2::Label::num(thread_id_key, 42, ""))); + profile + .try_add_sample2(&locations, &values, labels_iter, None) + .expect("add with numeric label to succeed"); + + // Should be 3 samples now + assert_eq!(profile.only_for_testing_num_aggregated_samples(), 3); + + // Verify the profile roundtrips correctly through pprof serialization + let pprof = roundtrip_to_pprof(profile).unwrap(); + assert_eq!(pprof.samples.len(), 3); + + // Verify we have the expected sample type + assert_eq!(pprof.sample_types.len(), 1); + let sample_type = &pprof.sample_types[0]; + let type_str = &pprof.string_table[sample_type.r#type as usize]; + let unit_str = &pprof.string_table[sample_type.unit as usize]; + assert_eq!(type_str, "samples"); + assert_eq!(unit_str, "count"); + + // Verify the mapping is present and has the correct filename + assert_eq!(pprof.mappings.len(), 1); + let mapping = &pprof.mappings[0]; + let mapping_filename = &pprof.string_table[mapping.filename as usize]; + assert_eq!(mapping_filename, "/usr/lib/ruby.so"); + + // Verify all 18 locations are present in each sample (same stack) + assert_eq!(pprof.samples[0].location_ids.len(), 18); + assert_eq!(pprof.samples[1].location_ids.len(), 18); + assert_eq!(pprof.samples[2].location_ids.len(), 18); + + // Verify all filenames and function names from our frames are present + let mut expected_files = std::collections::HashSet::new(); + let mut expected_functions = std::collections::HashSet::new(); + for frame in &frames { + expected_files.insert(frame.file_name); + expected_functions.insert(frame.function_name); + } + + let string_table_set: std::collections::HashSet<&str> = + pprof.string_table.iter().map(|s| s.as_str()).collect(); + + assert!( + expected_files.is_subset(&string_table_set), + "Missing files from string table: {:?}", + expected_files + .difference(&string_table_set) + .collect::>() + ); + + assert!( + expected_functions.is_subset(&string_table_set), + "Missing functions from string table: {:?}", + expected_functions + .difference(&string_table_set) + .collect::>() + ); + + // Verify the label keys and values we added are present in string table + let expected_label_strings: std::collections::HashSet<&str> = + ["thread_id", "thread_id_num", "worker-1"] + .into_iter() + .collect(); + assert!( + expected_label_strings.is_subset(&string_table_set), + "Missing label strings from string table: {:?}", + expected_label_strings + .difference(&string_table_set) + .collect::>() + ); + + // Verify sample values + // We have 3 samples: one with value 2 (aggregated), two with value 1 + // Samples may be reordered, so collect and sort the values + let mut values: Vec = pprof.samples.iter().map(|s| s.values[0]).collect(); + values.sort_unstable(); + assert_eq!(values, vec![1, 1, 2]); + } + #[test] fn test_regression_managed_string_table_correctly_maps_ids() { let storage = Arc::new(Mutex::new(ManagedStringStorage::new())); From 939907b62d00c9ee6ec13e26ffa7458f0520e76c Mon Sep 17 00:00:00 2001 From: Levi Morrison Date: Wed, 19 Nov 2025 10:42:24 -0700 Subject: [PATCH 06/20] test: write the test in a more obvious way --- libdd-profiling/src/internal/profile/mod.rs | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/libdd-profiling/src/internal/profile/mod.rs b/libdd-profiling/src/internal/profile/mod.rs index 87056d9f9d..fba164fc4c 100644 --- a/libdd-profiling/src/internal/profile/mod.rs +++ b/libdd-profiling/src/internal/profile/mod.rs @@ -1092,6 +1092,7 @@ mod api_tests { use super::*; use crate::pprof::test_utils::{roundtrip_to_pprof, sorted_samples, string_table_fetch}; use libdd_profiling_protobuf::prost_impls; + use std::collections::HashSet; #[test] fn interning() { @@ -2879,16 +2880,16 @@ mod api_tests { ); // Verify the label keys and values we added are present in string table - let expected_label_strings: std::collections::HashSet<&str> = - ["thread_id", "thread_id_num", "worker-1"] - .into_iter() - .collect(); + let expected_label_strings = ["thread_id", "thread_id_num", "worker-1"] + .into_iter() + .collect::>(); + let diff = expected_label_strings + .difference(&string_table_set) + .collect::>(); assert!( - expected_label_strings.is_subset(&string_table_set), + diff.is_empty(), "Missing label strings from string table: {:?}", - expected_label_strings - .difference(&string_table_set) - .collect::>() + diff ); // Verify sample values From cfc8a17401ea455fc7dd1e38410a62dcf9b1fca4 Mon Sep 17 00:00:00 2001 From: Levi Morrison Date: Wed, 19 Nov 2025 11:04:04 -0700 Subject: [PATCH 07/20] docs: improve ProfilesDictionary and related types --- .../src/profiles/datatypes/function.rs | 13 ++++++++++++- libdd-profiling/src/profiles/datatypes/mapping.rs | 13 ++++++++++++- .../src/profiles/datatypes/profiles_dictionary.rs | 7 +++++++ libdd-profiling/src/profiles/datatypes/string.rs | 15 ++++++++++++++- 4 files changed, 45 insertions(+), 3 deletions(-) diff --git a/libdd-profiling/src/profiles/datatypes/function.rs b/libdd-profiling/src/profiles/datatypes/function.rs index 8e37829a84..2e309eccae 100644 --- a/libdd-profiling/src/profiles/datatypes/function.rs +++ b/libdd-profiling/src/profiles/datatypes/function.rs @@ -7,6 +7,9 @@ use crate::profiles::datatypes::StringId2; /// A representation of a function that is an intersection of the Otel and /// Pprof representations. Omits the start line to save space because Datadog /// doesn't use this in any way. +/// +/// This representation is used internally by the `ProfilesDictionary`, and +/// utilizes the fact that `StringRef`s don't have null values. #[derive(Copy, Clone, Debug, Default, Eq, PartialEq, Hash)] #[repr(C)] pub struct Function { @@ -15,7 +18,7 @@ pub struct Function { pub file_name: StringRef, } -/// An FFI-safe version of the Function. +/// An FFI-safe version of the Function which allows null. #[repr(C)] #[derive(Copy, Clone, Debug, Default)] pub struct Function2 { @@ -44,6 +47,14 @@ impl From for Function { } } +/// An FFI-safe representation of a "handle" to a function which has been +/// stored in the `ProfilesDictionary`. The representation is ensured to be a +/// pointer for ABI stability, but callers should not generally dereference +/// this pointer. When using the id, the caller needs to be sure that the +/// `ProfilesDictionary` it refers to is the same one that the operations are +/// performed on; it is not generally guaranteed that ids from one dictionary +/// can be used in another dictionary, even if it happens to work by +/// implementation detail. #[derive(Clone, Copy, Debug)] #[repr(transparent)] pub struct FunctionId2(pub(crate) *mut Function2); diff --git a/libdd-profiling/src/profiles/datatypes/mapping.rs b/libdd-profiling/src/profiles/datatypes/mapping.rs index 941bb3ac0a..ee0b9a262e 100644 --- a/libdd-profiling/src/profiles/datatypes/mapping.rs +++ b/libdd-profiling/src/profiles/datatypes/mapping.rs @@ -7,6 +7,9 @@ use crate::profiles::datatypes::StringId2; /// A representation of a mapping that is an intersection of the Otel and Pprof /// representations. Omits boolean attributes because Datadog doesn't use them /// in any way. +/// +/// This representation is used internally by the `ProfilesDictionary`, and +/// utilizes the fact that `StringRef`s don't have null values. #[repr(C)] #[derive(Copy, Clone, Debug, Default, Eq, PartialEq, Hash)] pub struct Mapping { @@ -17,7 +20,7 @@ pub struct Mapping { pub build_id: StringRef, // missing in Otel, is it made into an attribute? } -/// An FFI-safe version of the Mapping. +/// An FFI-safe version of the Mapping which allows null. #[repr(C)] #[derive(Copy, Clone, Debug, Default)] pub struct Mapping2 { @@ -52,6 +55,14 @@ impl From for Mapping2 { } } +/// An FFI-safe representation of a "handle" to a mapping which has been +/// stored in the `ProfilesDictionary`. The representation is ensured to be a +/// pointer for ABI stability, but callers should not generally dereference +/// this pointer. When using the id, the caller needs to be sure that the +/// `ProfilesDictionary` it refers to is the same one that the operations are +/// performed on; it is not generally guaranteed that ids from one dictionary +/// can be used in another dictionary, even if it happens to work by +/// implementation detail. #[derive(Clone, Copy, Debug)] #[repr(transparent)] pub struct MappingId2(pub(crate) *mut Mapping2); diff --git a/libdd-profiling/src/profiles/datatypes/profiles_dictionary.rs b/libdd-profiling/src/profiles/datatypes/profiles_dictionary.rs index f2ada4ef24..db1ea530eb 100644 --- a/libdd-profiling/src/profiles/datatypes/profiles_dictionary.rs +++ b/libdd-profiling/src/profiles/datatypes/profiles_dictionary.rs @@ -9,6 +9,13 @@ use crate::profiles::datatypes::{ pub type FunctionSet = ParallelSet; pub type MappingSet = ParallelSet; +/// `ProfilesDictionary` contains data which are common to multiple profiles, +/// whether that's multiple profiles simultaneously or multiple profiles +/// through time. +/// +/// The current implementation is thread-safe, though there has been some +/// discussion about making that optional, as some libraries will call these +/// APIs in places where a mutex is already employed. pub struct ProfilesDictionary { functions: FunctionSet, mappings: MappingSet, diff --git a/libdd-profiling/src/profiles/datatypes/string.rs b/libdd-profiling/src/profiles/datatypes/string.rs index 9260f8144a..b556bf9083 100644 --- a/libdd-profiling/src/profiles/datatypes/string.rs +++ b/libdd-profiling/src/profiles/datatypes/string.rs @@ -4,11 +4,24 @@ use crate::profiles::collections::StringRef; /// An FFI-safe string ID where a null StringId2 maps to `StringRef::EMPTY`. +/// The representation is ensured to be a pointer for ABI stability, but +/// callers should not generally dereference this pointer. When using the id, +/// the caller needs to be sure that the `ProfilesDictionary` or string set it +/// refers to is the same one that the operations are performed on; it is not +/// generally guaranteed that ids from one dictionary/set can be used in +/// another, even if it happens to work by implementation detail. There is an +/// exception is for well-known strings, which are considered present in every +/// string set. #[repr(transparent)] #[derive(Clone, Copy, Debug)] pub struct StringId2(*mut StringHeader); -/// Represents a pointer to a string's header. Its definition is intentionally obscured. +/// Represents what StringIds point to. Its definition is intentionally +/// obscured; the actual layout is being hidden. This is here so that +/// cbindgen will generate a unique type as opposed to relying on `void *` or +/// similar. We want StringId2, FunctionId2, and MappingId2 to all point to +/// unique so that compilers will distinguish between them and provide some +/// type safety. #[derive(Clone, Copy, Debug)] #[repr(C)] pub struct StringHeader(u8); From 91d6ac3c687b1fac27a97b86aa6ec377ae9e27c5 Mon Sep 17 00:00:00 2001 From: Levi Morrison Date: Wed, 19 Nov 2025 15:07:44 -0700 Subject: [PATCH 08/20] refactor: try_new2 -> try_new_with_dictionary --- libdd-profiling/benches/add_samples.rs | 4 +-- libdd-profiling/src/api2.rs | 23 +----------- .../src/internal/owned_types/mod.rs | 35 ++++--------------- libdd-profiling/src/internal/profile/mod.rs | 26 +++++--------- 4 files changed, 19 insertions(+), 69 deletions(-) diff --git a/libdd-profiling/benches/add_samples.rs b/libdd-profiling/benches/add_samples.rs index ae4062fde1..7912fb39bb 100644 --- a/libdd-profiling/benches/add_samples.rs +++ b/libdd-profiling/benches/add_samples.rs @@ -149,10 +149,10 @@ pub fn bench_add_sample_vs_add2(c: &mut Criterion) { c.bench_function("profile_add_sample2_frames_x1000", |b| { b.iter(|| { - let mut profile = profiling::internal::Profile::try_new2( - dict.try_clone().unwrap(), + let mut profile = profiling::internal::Profile::try_new_with_dictionary( &sample_types2, None, + dict.try_clone().unwrap(), ) .unwrap(); let (locations, values) = make_stack_api2(frames2.as_slice()); diff --git a/libdd-profiling/src/api2.rs b/libdd-profiling/src/api2.rs index 601e691142..3dd35e90c8 100644 --- a/libdd-profiling/src/api2.rs +++ b/libdd-profiling/src/api2.rs @@ -1,7 +1,7 @@ // Copyright 2025-Present Datadog, Inc. https://www.datadoghq.com/ // SPDX-License-Identifier: Apache-2.0 -use crate::profiles::datatypes::{self, FunctionId2, MappingId2, StringId2}; +use crate::profiles::datatypes::{FunctionId2, MappingId2, StringId2}; #[derive(Copy, Clone, Debug, Default)] #[repr(C)] @@ -55,24 +55,3 @@ impl<'a> Label<'a> { } } } -#[repr(C)] -#[derive(Clone, Copy, Debug)] -pub struct ValueType2 { - pub type_id: StringId2, - pub unit_id: StringId2, -} - -impl From for datatypes::ValueType { - fn from(value: ValueType2) -> datatypes::ValueType { - datatypes::ValueType { - type_id: value.type_id.into(), - unit_id: value.unit_id.into(), - } - } -} - -#[derive(Copy, Clone, Debug)] -pub struct Period2 { - pub r#type: ValueType2, - pub value: i64, -} diff --git a/libdd-profiling/src/internal/owned_types/mod.rs b/libdd-profiling/src/internal/owned_types/mod.rs index 9db99f2507..d4955292f1 100644 --- a/libdd-profiling/src/internal/owned_types/mod.rs +++ b/libdd-profiling/src/internal/owned_types/mod.rs @@ -2,9 +2,6 @@ // SPDX-License-Identifier: Apache-2.0 use crate::api; -use crate::api2::{Period2, ValueType2}; -use crate::profiles::collections::StringRef; -use std::ops::Deref; #[cfg_attr(test, derive(bolero::generator::TypeGenerator))] #[derive(Clone, Debug)] @@ -23,22 +20,6 @@ impl<'a> From<&'a api::ValueType<'a>> for ValueType { } } -impl From for ValueType { - fn from(value_type2: ValueType2) -> ValueType { - let typ: StringRef = value_type2.type_id.into(); - let unit: StringRef = value_type2.unit_id.into(); - ValueType { - typ: Box::from(typ.0.deref()), - unit: Box::from(unit.0.deref()), - } - } -} -impl From<&ValueType2> for ValueType { - fn from(value_type2: &ValueType2) -> ValueType { - ValueType::from(*value_type2) - } -} - impl<'a> From<&'a ValueType> for api::ValueType<'a> { fn from(value: &'a ValueType) -> Self { Self::new(&value.typ, &value.unit) @@ -52,6 +33,13 @@ pub struct Period { pub value: i64, } +impl<'a> From> for Period { + #[inline] + fn from(period: api::Period<'a>) -> Self { + Period::from(&period) + } +} + impl<'a> From<&'a api::Period<'a>> for Period { #[inline] fn from(period: &'a api::Period<'a>) -> Self { @@ -61,12 +49,3 @@ impl<'a> From<&'a api::Period<'a>> for Period { } } } - -impl From for Period { - fn from(period2: Period2) -> Period { - Period { - typ: ValueType::from(period2.r#type), - value: 0, - } - } -} diff --git a/libdd-profiling/src/internal/profile/mod.rs b/libdd-profiling/src/internal/profile/mod.rs index fba164fc4c..8516320829 100644 --- a/libdd-profiling/src/internal/profile/mod.rs +++ b/libdd-profiling/src/internal/profile/mod.rs @@ -12,7 +12,6 @@ pub use profiles_dictionary_translator::*; use self::api::UpscalingInfo; use super::*; use crate::api::ManagedStringId; -use crate::api2::{Period2, ValueType2}; use crate::collections::identifiable::*; use crate::collections::string_storage::{CachedProfileId, ManagedStringStorage}; use crate::collections::string_table::{self, StringTable}; @@ -384,7 +383,7 @@ impl Profile { /// /// All other fields are default. /// - /// It's recommended to use [`Profile::try_new2`] instead. + /// It's recommended to use [`Profile::try_new_with_dictionary`] instead. pub fn try_new( sample_types: &[api::ValueType], period: Option, @@ -397,14 +396,13 @@ impl Profile { ) } - /// Tries to create a profile with the given period and sample types. The - /// [`StringId2`]s should belong to the provided [`ProfilesDictionary`]. + /// Tries to create a profile with the given period and sample types. #[inline(never)] #[cold] - pub fn try_new2( + pub fn try_new_with_dictionary( + sample_types: &[api::ValueType], + period: Option, profiles_dictionary: crate::profiles::collections::Arc, - sample_types: &[ValueType2], - period: Option, ) -> io::Result { let mut owned_sample_types = Vec::new(); // Using try_reserve_exact because it will be converted to a Box<[]>, @@ -2707,7 +2705,7 @@ mod api_tests { #[test] #[cfg_attr(miri, ignore)] - fn test_try_new2_and_try_add_sample2() { + fn test_try_new_with_dictionary_and_try_add_sample2() { struct Frame { file_name: &'static str, line_number: u32, @@ -2716,14 +2714,7 @@ mod api_tests { // Create a ProfilesDictionary with realistic data from Ruby app let dict = crate::profiles::datatypes::ProfilesDictionary::try_new().unwrap(); - - // Create sample types - let samples_type = dict.try_insert_str2("samples").unwrap(); - let count_unit = dict.try_insert_str2("count").unwrap(); - let sample_types = vec![ValueType2 { - type_id: samples_type, - unit_id: count_unit, - }]; + let sample_types = vec![api::ValueType::new("samples", "count")]; // Ruby stack trace (leaf-to-root order) // Taken from a Ruby app, everything here is source-available @@ -2784,7 +2775,8 @@ mod api_tests { // Create profile with dictionary let mut profile = - Profile::try_new2(dict.try_clone().unwrap(), &sample_types, None).unwrap(); + Profile::try_new_with_dictionary(&sample_types, None, dict.try_clone().unwrap()) + .unwrap(); assert_eq!(profile.only_for_testing_num_aggregated_samples(), 0); From 38c8e3cd6608619ee29d289048f2cbd3b811f1fc Mon Sep 17 00:00:00 2001 From: Levi Morrison Date: Wed, 19 Nov 2025 21:27:25 -0700 Subject: [PATCH 09/20] fix: bench too --- libdd-profiling/benches/add_samples.rs | 14 +------------- 1 file changed, 1 insertion(+), 13 deletions(-) diff --git a/libdd-profiling/benches/add_samples.rs b/libdd-profiling/benches/add_samples.rs index 7912fb39bb..7b4946751d 100644 --- a/libdd-profiling/benches/add_samples.rs +++ b/libdd-profiling/benches/add_samples.rs @@ -75,20 +75,8 @@ struct Frame2 { pub fn bench_add_sample_vs_add2(c: &mut Criterion) { let sample_types = make_sample_types(); - - // Create dictionary for both sample types and frames let dict = profiling::profiles::datatypes::ProfilesDictionary::try_new().unwrap(); - // Convert sample_types to ValueType2 by inserting strings into dictionary - let sample_types2: Vec<_> = sample_types - .iter() - .map(|st| { - let type_id = dict.try_insert_str2(st.r#type).unwrap(); - let unit_id = dict.try_insert_str2(st.unit).unwrap(); - api2::ValueType2 { type_id, unit_id } - }) - .collect(); - // This is root-to-leaf, instead of leaf-to-root. We'll reverse it below. // Taken from a Ruby app, everything here is source-available. let mut frames = [ @@ -150,7 +138,7 @@ pub fn bench_add_sample_vs_add2(c: &mut Criterion) { c.bench_function("profile_add_sample2_frames_x1000", |b| { b.iter(|| { let mut profile = profiling::internal::Profile::try_new_with_dictionary( - &sample_types2, + &sample_types, None, dict.try_clone().unwrap(), ) From 45580ed1f46e67a1c97cac66034c8af2bac2ec7c Mon Sep 17 00:00:00 2001 From: Levi Morrison Date: Mon, 1 Dec 2025 19:07:50 -0700 Subject: [PATCH 10/20] test: v1 and v2 have compat reprs --- .../src/profiles/datatypes/function.rs | 24 +++++++++++++ .../src/profiles/datatypes/mapping.rs | 34 +++++++++++++++++++ 2 files changed, 58 insertions(+) diff --git a/libdd-profiling/src/profiles/datatypes/function.rs b/libdd-profiling/src/profiles/datatypes/function.rs index 2e309eccae..4a342f4bc4 100644 --- a/libdd-profiling/src/profiles/datatypes/function.rs +++ b/libdd-profiling/src/profiles/datatypes/function.rs @@ -85,3 +85,27 @@ impl FunctionId2 { } } } + +#[cfg(test)] +mod tests { + use super::*; + use std::mem::offset_of; + + #[test] + fn v1_and_v2_have_compatible_representations() { + // Begin with basic size and alignment. + assert_eq!(size_of::(), size_of::()); + assert_eq!(align_of::(), align_of::()); + + // Then check members. + assert_eq!(offset_of!(Function, name), offset_of!(Function2, name)); + assert_eq!( + offset_of!(Function, system_name), + offset_of!(Function2, system_name) + ); + assert_eq!( + offset_of!(Function, file_name), + offset_of!(Function2, file_name) + ); + } +} diff --git a/libdd-profiling/src/profiles/datatypes/mapping.rs b/libdd-profiling/src/profiles/datatypes/mapping.rs index ee0b9a262e..2578a5d21a 100644 --- a/libdd-profiling/src/profiles/datatypes/mapping.rs +++ b/libdd-profiling/src/profiles/datatypes/mapping.rs @@ -93,3 +93,37 @@ impl MappingId2 { } } } + +#[cfg(test)] +mod tests { + use super::*; + use std::mem::offset_of; + #[test] + fn v1_and_v2_have_compatible_representations() { + // Begin with basic size and alignment. + assert_eq!(size_of::(), size_of::()); + assert_eq!(align_of::(), align_of::()); + + // Then check members. + assert_eq!( + offset_of!(Mapping, memory_start), + offset_of!(Mapping2, memory_start) + ); + assert_eq!( + offset_of!(Mapping, memory_limit), + offset_of!(Mapping2, memory_limit) + ); + assert_eq!( + offset_of!(Mapping, file_offset), + offset_of!(Mapping2, file_offset) + ); + assert_eq!( + offset_of!(Mapping, filename), + offset_of!(Mapping2, filename) + ); + assert_eq!( + offset_of!(Mapping, build_id), + offset_of!(Mapping2, build_id) + ); + } +} From 524c41788bc505470b30011e5c43328cf787009d Mon Sep 17 00:00:00 2001 From: Levi Morrison Date: Mon, 1 Dec 2025 19:20:51 -0700 Subject: [PATCH 11/20] refactor: move transmute into From --- libdd-profiling/src/profiles/datatypes/function.rs | 11 ++++++++++- libdd-profiling/src/profiles/datatypes/mapping.rs | 11 ++++++++++- .../src/profiles/datatypes/profiles_dictionary.rs | 14 ++------------ 3 files changed, 22 insertions(+), 14 deletions(-) diff --git a/libdd-profiling/src/profiles/datatypes/function.rs b/libdd-profiling/src/profiles/datatypes/function.rs index 4a342f4bc4..aa98b33a1f 100644 --- a/libdd-profiling/src/profiles/datatypes/function.rs +++ b/libdd-profiling/src/profiles/datatypes/function.rs @@ -1,7 +1,7 @@ // Copyright 2025-Present Datadog, Inc. https://www.datadoghq.com/ // SPDX-License-Identifier: Apache-2.0 -use crate::profiles::collections::StringRef; +use crate::profiles::collections::{SetId, StringRef}; use crate::profiles::datatypes::StringId2; /// A representation of a function that is an intersection of the Otel and @@ -86,6 +86,15 @@ impl FunctionId2 { } } +impl From> for FunctionId2 { + fn from(id: SetId) -> FunctionId2 { + // SAFETY: the function that SetId points to is layout compatible with + // the one that FunctionId2 points to. The reverse is not true for the + // null StringId cases. + unsafe { core::mem::transmute::, FunctionId2>(id) } + } +} + #[cfg(test)] mod tests { use super::*; diff --git a/libdd-profiling/src/profiles/datatypes/mapping.rs b/libdd-profiling/src/profiles/datatypes/mapping.rs index 2578a5d21a..25aa37fb40 100644 --- a/libdd-profiling/src/profiles/datatypes/mapping.rs +++ b/libdd-profiling/src/profiles/datatypes/mapping.rs @@ -1,7 +1,7 @@ // Copyright 2025-Present Datadog, Inc. https://www.datadoghq.com/ // SPDX-License-Identifier: Apache-2.0 -use crate::profiles::collections::StringRef; +use crate::profiles::collections::{SetId, StringRef}; use crate::profiles::datatypes::StringId2; /// A representation of a mapping that is an intersection of the Otel and Pprof @@ -94,6 +94,15 @@ impl MappingId2 { } } +impl From> for MappingId2 { + fn from(id: SetId) -> MappingId2 { + // SAFETY: the mapping that SetId points to is layout compatible with + // the one that MappingId2 points to. The reverse is not true for the + // null StringId cases. + unsafe { core::mem::transmute::, MappingId2>(id) } + } +} + #[cfg(test)] mod tests { use super::*; diff --git a/libdd-profiling/src/profiles/datatypes/profiles_dictionary.rs b/libdd-profiling/src/profiles/datatypes/profiles_dictionary.rs index db1ea530eb..263dd32cc8 100644 --- a/libdd-profiling/src/profiles/datatypes/profiles_dictionary.rs +++ b/libdd-profiling/src/profiles/datatypes/profiles_dictionary.rs @@ -46,12 +46,7 @@ impl ProfilesDictionary { file_name: function.file_name.into(), }; let set_id = self.functions.try_insert(function)?; - - // SAFETY: the function that SetId points to is layout compatible with - // the one that FunctionId2 points to. The reverse is not true for the - // null StringId cases. - let function_id = unsafe { core::mem::transmute::, FunctionId2>(set_id) }; - Ok(function_id) + Ok(FunctionId2::from(set_id)) } /// Adds the mapping to the mapping set in the dictionary, and returns a @@ -66,12 +61,7 @@ impl ProfilesDictionary { build_id: mapping.build_id.into(), }; let set_id = self.mappings.try_insert(mapping)?; - - // SAFETY: the mapping that SetId points to is layout compatible with - // the one that MappingId2 points to. The reverse is not true for the - // null StringId cases. - let mapping_id = unsafe { core::mem::transmute::, MappingId2>(set_id) }; - Ok(mapping_id) + Ok(MappingId2::from(set_id)) } /// Adds the string to the string set in the dictionary, and returns a From e8a1ad88638a4643488e75810b9d8939fa0f9120 Mon Sep 17 00:00:00 2001 From: Levi Morrison Date: Tue, 2 Dec 2025 20:24:18 -0700 Subject: [PATCH 12/20] refactor: more cautious around transmutes --- libdd-profiling/src/internal/profile/mod.rs | 2 +- .../profile/profiles_dictionary_translator.rs | 106 ++++++++++-------- .../src/profiles/datatypes/mapping.rs | 1 + .../profiles/datatypes/profiles_dictionary.rs | 4 +- 4 files changed, 65 insertions(+), 48 deletions(-) diff --git a/libdd-profiling/src/internal/profile/mod.rs b/libdd-profiling/src/internal/profile/mod.rs index 8516320829..3994dc2657 100644 --- a/libdd-profiling/src/internal/profile/mod.rs +++ b/libdd-profiling/src/internal/profile/mod.rs @@ -205,7 +205,7 @@ impl Profile { lbls.try_reserve_exact(labels.len())?; for label in labels { let label = label.context("profile label failed to convert")?; - let key = translator.translate_string(string_table, label.key)?; + let key = translator.translate_string(string_table, label.key.into())?; let internal_label = if !label.str.is_empty() { let str = string_table.try_intern(label.str)?; Label::str(key, str) diff --git a/libdd-profiling/src/internal/profile/profiles_dictionary_translator.rs b/libdd-profiling/src/internal/profile/profiles_dictionary_translator.rs index 976ed7c26e..5ac9d6b2d4 100644 --- a/libdd-profiling/src/internal/profile/profiles_dictionary_translator.rs +++ b/libdd-profiling/src/internal/profile/profiles_dictionary_translator.rs @@ -5,15 +5,15 @@ use crate::collections::identifiable::{Dedup, FxIndexMap, StringId}; use crate::collections::string_table::StringTable; use crate::internal::{Function, FunctionId, Mapping, MappingId}; use crate::profiles::collections::{SetId, StringRef}; -use crate::profiles::datatypes::{ - self as dt, FunctionId2, MappingId2, ProfilesDictionary, StringId2, -}; +use crate::profiles::datatypes::{self as dt, FunctionId2, MappingId2, ProfilesDictionary}; +use anyhow::Context; use indexmap::map::Entry; +use std::ptr::NonNull; pub struct ProfilesDictionaryTranslator { pub profiles_dictionary: crate::profiles::collections::Arc, - pub mappings: FxIndexMap, Option>, - pub functions: FxIndexMap, FunctionId>, + pub mappings: FxIndexMap, MappingId>, + pub functions: FxIndexMap>, FunctionId>, pub strings: FxIndexMap, } @@ -38,25 +38,40 @@ impl ProfilesDictionaryTranslator { string_table: &mut StringTable, id2: FunctionId2, ) -> anyhow::Result { - let function2 = (unsafe { id2.read() }).unwrap_or_default(); - let set_id = unsafe { core::mem::transmute::>(id2) }; - - if let Some(internal) = self.functions.get(&set_id) { - return Ok(*internal); - } + let (set_id, function) = match NonNull::new(id2.0) { + // Since the internal model treats functions as required, we + // translate the null FunctionId2 to the default function. + None => { + let function = Function { + name: StringId::ZERO, + system_name: StringId::ZERO, + filename: StringId::ZERO, + }; + (None, function) + } + Some(nn) => { + let set_id = SetId(nn.cast::()); + if let Some(internal) = self.functions.get(&Some(set_id)) { + return Ok(*internal); + } - let (name, system_name, filename) = ( - self.translate_string(string_table, function2.name)?, - self.translate_string(string_table, function2.system_name)?, - self.translate_string(string_table, function2.file_name)?, - ); - let function = Function { - name, - system_name, - filename, + // SAFETY: todo + let function = unsafe { *self.profiles_dictionary.functions().get(set_id) }; + let function = Function { + name: self.translate_string(string_table, function.name)?, + system_name: self.translate_string(string_table, function.system_name)?, + filename: self.translate_string(string_table, function.file_name)?, + }; + (Some(set_id), function) + } }; - let internal_id = functions.dedup(function); - self.functions.try_reserve(1)?; + + let internal_id = functions + .try_dedup(function) + .context("failed to deduplicate function in ProfilesDictionaryTranslator")?; + self.functions.try_reserve(1).context( + "failed to reserve memory for a new function in ProfilesDictionaryTranslator", + )?; self.functions.insert(set_id, internal_id); Ok(internal_id) } @@ -67,45 +82,46 @@ impl ProfilesDictionaryTranslator { string_table: &mut StringTable, id2: MappingId2, ) -> anyhow::Result> { - let Some(mapping2) = (unsafe { id2.read() }) else { + // Translate null MappingId2 to Ok(None). This is different from + // functions because the internal module uses Option, + // whereas it assumes functions are required. + let Some(nn) = NonNull::new(id2.0) else { return Ok(None); }; - let set_id = unsafe { core::mem::transmute::>(id2) }; - + let set_id = SetId(nn.cast::()); if let Some(internal) = self.mappings.get(&set_id) { - return Ok(*internal); + return Ok(Some(*internal)); } - let filename = self.translate_string(string_table, mapping2.filename)?; - let build_id = self.translate_string(string_table, mapping2.build_id)?; - let mapping = Mapping { - memory_start: mapping2.memory_start, - memory_limit: mapping2.memory_limit, - file_offset: mapping2.file_offset, - filename, - build_id, + // SAFETY: todo + let mapping = unsafe { *self.profiles_dictionary.mappings().get(set_id) }; + let internal = Mapping { + memory_start: mapping.memory_start, + memory_limit: mapping.memory_limit, + file_offset: mapping.file_offset, + filename: self.translate_string(string_table, mapping.filename)?, + build_id: self.translate_string(string_table, mapping.build_id)?, }; - let internal_id = mappings.dedup(mapping); - self.mappings.try_reserve(1)?; - self.mappings.insert(set_id, Some(internal_id)); + let internal_id = mappings + .try_dedup(internal) + .context("failed to deduplicate mapping in ProfilesDictionaryTranslator")?; + self.mappings.try_reserve(1).context( + "failed to reserve memory for a new mapping in ProfilesDictionaryTranslator", + )?; + self.mappings.insert(set_id, internal_id); Ok(Some(internal_id)) } pub fn translate_string( &mut self, string_table: &mut StringTable, - id2: StringId2, + str_ref: StringRef, ) -> anyhow::Result { - if id2.is_empty() { - return Ok(StringId::ZERO); - } - - let string_ref = StringRef::from(id2); self.strings.try_reserve(1)?; - match self.strings.entry(string_ref) { + match self.strings.entry(str_ref) { Entry::Occupied(o) => Ok(*o.get()), Entry::Vacant(v) => { - let str = unsafe { self.profiles_dictionary.strings().get(string_ref) }; + let str = unsafe { self.profiles_dictionary.strings().get(str_ref) }; // SAFETY: we're keeping these lifetimes in sync. I think. // TODO: BUT longer-term we want to avoid copying them // entirely, so this should go away. diff --git a/libdd-profiling/src/profiles/datatypes/mapping.rs b/libdd-profiling/src/profiles/datatypes/mapping.rs index 25aa37fb40..3c07084b42 100644 --- a/libdd-profiling/src/profiles/datatypes/mapping.rs +++ b/libdd-profiling/src/profiles/datatypes/mapping.rs @@ -107,6 +107,7 @@ impl From> for MappingId2 { mod tests { use super::*; use std::mem::offset_of; + #[test] fn v1_and_v2_have_compatible_representations() { // Begin with basic size and alignment. diff --git a/libdd-profiling/src/profiles/datatypes/profiles_dictionary.rs b/libdd-profiling/src/profiles/datatypes/profiles_dictionary.rs index 263dd32cc8..f47a5e0601 100644 --- a/libdd-profiling/src/profiles/datatypes/profiles_dictionary.rs +++ b/libdd-profiling/src/profiles/datatypes/profiles_dictionary.rs @@ -1,7 +1,7 @@ // Copyright 2025-Present Datadog, Inc. https://www.datadoghq.com/ // SPDX-License-Identifier: Apache-2.0 -use crate::profiles::collections::{ParallelSet, ParallelStringSet, SetError, SetId}; +use crate::profiles::collections::{ParallelSet, ParallelStringSet, SetError}; use crate::profiles::datatypes::{ Function, Function2, FunctionId2, Mapping, Mapping2, MappingId2, StringId2, }; @@ -167,7 +167,7 @@ mod tests { build_id: build_id_id, }; - // Test insert and read back (exercises unsafe transmute) + // Test insert and read back. let id1 = dict.try_insert_mapping2(mapping2).unwrap(); prop_assert!(!id1.is_empty()); From 4f0168e6503c6ef1ffca291b6f0b49a9063dc124 Mon Sep 17 00:00:00 2001 From: Levi Morrison Date: Tue, 2 Dec 2025 20:36:21 -0700 Subject: [PATCH 13/20] docs: repr(C) Function/Mapping --- libdd-profiling/src/profiles/datatypes/function.rs | 8 ++++++-- libdd-profiling/src/profiles/datatypes/mapping.rs | 8 ++++++-- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/libdd-profiling/src/profiles/datatypes/function.rs b/libdd-profiling/src/profiles/datatypes/function.rs index aa98b33a1f..9245ed4320 100644 --- a/libdd-profiling/src/profiles/datatypes/function.rs +++ b/libdd-profiling/src/profiles/datatypes/function.rs @@ -9,7 +9,10 @@ use crate::profiles::datatypes::StringId2; /// doesn't use this in any way. /// /// This representation is used internally by the `ProfilesDictionary`, and -/// utilizes the fact that `StringRef`s don't have null values. +/// utilizes the fact that `StringRef`s don't have null values. It is also +/// repr(C) to be layout-compatible with [`Function2`]. Every pointer to a +/// Function is a valid Function2 (but the reverse is not true for the null +/// case of null StringId2). #[derive(Copy, Clone, Debug, Default, Eq, PartialEq, Hash)] #[repr(C)] pub struct Function { @@ -18,7 +21,8 @@ pub struct Function { pub file_name: StringRef, } -/// An FFI-safe version of the Function which allows null. +/// An FFI-safe version of the Function which allows null. Be sure to maintain +/// layout-compatibility with it, except that StringId2 may be null. #[repr(C)] #[derive(Copy, Clone, Debug, Default)] pub struct Function2 { diff --git a/libdd-profiling/src/profiles/datatypes/mapping.rs b/libdd-profiling/src/profiles/datatypes/mapping.rs index 3c07084b42..66f7c4e1b7 100644 --- a/libdd-profiling/src/profiles/datatypes/mapping.rs +++ b/libdd-profiling/src/profiles/datatypes/mapping.rs @@ -9,7 +9,10 @@ use crate::profiles::datatypes::StringId2; /// in any way. /// /// This representation is used internally by the `ProfilesDictionary`, and -/// utilizes the fact that `StringRef`s don't have null values. +/// utilizes the fact that `StringRef`s don't have null values. It is also +/// repr(C) to be layout-compatible with [`Mapping2`]. Every pointer to a +/// Mapping is a valid Mapping2 (but the reverse is not true for the null case +/// of null StringId2). #[repr(C)] #[derive(Copy, Clone, Debug, Default, Eq, PartialEq, Hash)] pub struct Mapping { @@ -20,7 +23,8 @@ pub struct Mapping { pub build_id: StringRef, // missing in Otel, is it made into an attribute? } -/// An FFI-safe version of the Mapping which allows null. +/// An FFI-safe version of the Mapping which allows null. Be sure to maintain +/// layout-compatibility with it, except that StringId2 may be null. #[repr(C)] #[derive(Copy, Clone, Debug, Default)] pub struct Mapping2 { From f2b3f8ad4b4c7abbd7cca5984843ce9edd73af54 Mon Sep 17 00:00:00 2001 From: Levi Morrison Date: Mon, 1 Dec 2025 19:20:51 -0700 Subject: [PATCH 14/20] refactor: move transmute into From --- libdd-profiling/src/profiles/datatypes/function.rs | 11 ++++++++++- libdd-profiling/src/profiles/datatypes/mapping.rs | 11 ++++++++++- .../src/profiles/datatypes/profiles_dictionary.rs | 14 ++------------ 3 files changed, 22 insertions(+), 14 deletions(-) diff --git a/libdd-profiling/src/profiles/datatypes/function.rs b/libdd-profiling/src/profiles/datatypes/function.rs index 4a342f4bc4..aa98b33a1f 100644 --- a/libdd-profiling/src/profiles/datatypes/function.rs +++ b/libdd-profiling/src/profiles/datatypes/function.rs @@ -1,7 +1,7 @@ // Copyright 2025-Present Datadog, Inc. https://www.datadoghq.com/ // SPDX-License-Identifier: Apache-2.0 -use crate::profiles::collections::StringRef; +use crate::profiles::collections::{SetId, StringRef}; use crate::profiles::datatypes::StringId2; /// A representation of a function that is an intersection of the Otel and @@ -86,6 +86,15 @@ impl FunctionId2 { } } +impl From> for FunctionId2 { + fn from(id: SetId) -> FunctionId2 { + // SAFETY: the function that SetId points to is layout compatible with + // the one that FunctionId2 points to. The reverse is not true for the + // null StringId cases. + unsafe { core::mem::transmute::, FunctionId2>(id) } + } +} + #[cfg(test)] mod tests { use super::*; diff --git a/libdd-profiling/src/profiles/datatypes/mapping.rs b/libdd-profiling/src/profiles/datatypes/mapping.rs index 2578a5d21a..25aa37fb40 100644 --- a/libdd-profiling/src/profiles/datatypes/mapping.rs +++ b/libdd-profiling/src/profiles/datatypes/mapping.rs @@ -1,7 +1,7 @@ // Copyright 2025-Present Datadog, Inc. https://www.datadoghq.com/ // SPDX-License-Identifier: Apache-2.0 -use crate::profiles::collections::StringRef; +use crate::profiles::collections::{SetId, StringRef}; use crate::profiles::datatypes::StringId2; /// A representation of a mapping that is an intersection of the Otel and Pprof @@ -94,6 +94,15 @@ impl MappingId2 { } } +impl From> for MappingId2 { + fn from(id: SetId) -> MappingId2 { + // SAFETY: the mapping that SetId points to is layout compatible with + // the one that MappingId2 points to. The reverse is not true for the + // null StringId cases. + unsafe { core::mem::transmute::, MappingId2>(id) } + } +} + #[cfg(test)] mod tests { use super::*; diff --git a/libdd-profiling/src/profiles/datatypes/profiles_dictionary.rs b/libdd-profiling/src/profiles/datatypes/profiles_dictionary.rs index db1ea530eb..263dd32cc8 100644 --- a/libdd-profiling/src/profiles/datatypes/profiles_dictionary.rs +++ b/libdd-profiling/src/profiles/datatypes/profiles_dictionary.rs @@ -46,12 +46,7 @@ impl ProfilesDictionary { file_name: function.file_name.into(), }; let set_id = self.functions.try_insert(function)?; - - // SAFETY: the function that SetId points to is layout compatible with - // the one that FunctionId2 points to. The reverse is not true for the - // null StringId cases. - let function_id = unsafe { core::mem::transmute::, FunctionId2>(set_id) }; - Ok(function_id) + Ok(FunctionId2::from(set_id)) } /// Adds the mapping to the mapping set in the dictionary, and returns a @@ -66,12 +61,7 @@ impl ProfilesDictionary { build_id: mapping.build_id.into(), }; let set_id = self.mappings.try_insert(mapping)?; - - // SAFETY: the mapping that SetId points to is layout compatible with - // the one that MappingId2 points to. The reverse is not true for the - // null StringId cases. - let mapping_id = unsafe { core::mem::transmute::, MappingId2>(set_id) }; - Ok(mapping_id) + Ok(MappingId2::from(set_id)) } /// Adds the string to the string set in the dictionary, and returns a From f1a03bf961f15e4a9123cd984c067acd25d6e485 Mon Sep 17 00:00:00 2001 From: Levi Morrison Date: Tue, 2 Dec 2025 21:47:05 -0700 Subject: [PATCH 15/20] refactor: expand unsafe and better document it --- libdd-profiling/benches/add_samples.rs | 8 +- libdd-profiling/src/internal/profile/mod.rs | 46 ++++++++---- .../profile/profiles_dictionary_translator.rs | 75 +++++++++++++++---- 3 files changed, 99 insertions(+), 30 deletions(-) diff --git a/libdd-profiling/benches/add_samples.rs b/libdd-profiling/benches/add_samples.rs index 7b4946751d..62de57713c 100644 --- a/libdd-profiling/benches/add_samples.rs +++ b/libdd-profiling/benches/add_samples.rs @@ -113,7 +113,7 @@ pub fn bench_add_sample_vs_add2(c: &mut Criterion) { }) .unwrap(); Frame2 { - function: unsafe { core::mem::transmute::, FunctionId2>(set_id) }, + function: FunctionId2::from(set_id), line_number: f.line_number, } }); @@ -147,7 +147,11 @@ pub fn bench_add_sample_vs_add2(c: &mut Criterion) { for _ in 0..1000 { // Provide an empty iterator for labels conversion path let labels_iter = std::iter::empty::>(); - black_box(profile.try_add_sample2(&locations, &values, labels_iter, None)).unwrap(); + // SAFETY: all ids come from the profile's dictionary. + black_box(unsafe { + profile.try_add_sample2(&locations, &values, labels_iter, None) + }) + .unwrap(); } black_box(profile.only_for_testing_num_aggregated_samples()) }) diff --git a/libdd-profiling/src/internal/profile/mod.rs b/libdd-profiling/src/internal/profile/mod.rs index ac18d53c3c..93ad4a5973 100644 --- a/libdd-profiling/src/internal/profile/mod.rs +++ b/libdd-profiling/src/internal/profile/mod.rs @@ -173,7 +173,15 @@ impl Profile { } /// Tries to add a sample using `api2` structures. - pub fn try_add_sample2<'a, L: ExactSizeIterator>>>( + /// + /// # Safety + /// + /// All MappingId2, FunctionId2, and StringId2 values should be coming + /// from the same profiles dictionary used by this profile internally. + pub unsafe fn try_add_sample2< + 'a, + L: ExactSizeIterator>>, + >( &mut self, locations: &[api2::Location2], values: &[i64], @@ -2784,17 +2792,23 @@ mod api_tests { // Add sample without labels let labels_iter = std::iter::empty::>(); - profile - .try_add_sample2(&locations, &values, labels_iter, None) - .expect("add to succeed"); + // SAFETY: adding ids from the correct ProfilesDictionary. + unsafe { + profile + .try_add_sample2(&locations, &values, labels_iter, None) + .expect("add to succeed"); + } assert_eq!(profile.only_for_testing_num_aggregated_samples(), 1); // Add same sample again - should aggregate let labels_iter = std::iter::empty::>(); - profile - .try_add_sample2(&locations, &values, labels_iter, None) - .expect("add to succeed"); + // SAFETY: adding ids from the correct ProfilesDictionary. + unsafe { + profile + .try_add_sample2(&locations, &values, labels_iter, None) + .expect("add to succeed"); + } // Still 1 sample because it aggregated assert_eq!(profile.only_for_testing_num_aggregated_samples(), 1); @@ -2804,9 +2818,12 @@ mod api_tests { let label_value = "worker-1"; let labels_iter = std::iter::once(Ok(api2::Label::str(label_key, label_value))); - profile - .try_add_sample2(&locations, &values, labels_iter, None) - .expect("add with label to succeed"); + // SAFETY: adding ids from the correct ProfilesDictionary. + unsafe { + profile + .try_add_sample2(&locations, &values, labels_iter, None) + .expect("add with label to succeed"); + } // Should be 2 samples now (different label set) assert_eq!(profile.only_for_testing_num_aggregated_samples(), 2); @@ -2814,9 +2831,12 @@ mod api_tests { // Test with numeric label let thread_id_key = dict.try_insert_str2("thread_id_num").unwrap(); let labels_iter = std::iter::once(Ok(api2::Label::num(thread_id_key, 42, ""))); - profile - .try_add_sample2(&locations, &values, labels_iter, None) - .expect("add with numeric label to succeed"); + // SAFETY: adding ids from the correct ProfilesDictionary. + unsafe { + profile + .try_add_sample2(&locations, &values, labels_iter, None) + .expect("add with numeric label to succeed"); + } // Should be 3 samples now assert_eq!(profile.only_for_testing_num_aggregated_samples(), 3); diff --git a/libdd-profiling/src/internal/profile/profiles_dictionary_translator.rs b/libdd-profiling/src/internal/profile/profiles_dictionary_translator.rs index 5ac9d6b2d4..3bcc6832a5 100644 --- a/libdd-profiling/src/internal/profile/profiles_dictionary_translator.rs +++ b/libdd-profiling/src/internal/profile/profiles_dictionary_translator.rs @@ -10,6 +10,14 @@ use anyhow::Context; use indexmap::map::Entry; use std::ptr::NonNull; +/// Translates IDs from a [`ProfilesDictionary`] into the IDs used by the +/// current Profile's internal collections. +/// +/// # Safety +/// +/// All IDs passed to the translate methods (translate_function, +/// translate_mapping, translate_string) MUST have been created by the same +/// ProfilesDictionary that this translator wraps. pub struct ProfilesDictionaryTranslator { pub profiles_dictionary: crate::profiles::collections::Arc, pub mappings: FxIndexMap, MappingId>, @@ -17,7 +25,15 @@ pub struct ProfilesDictionaryTranslator { pub strings: FxIndexMap, } -// SAFETY: the profiles_dictionary keeps the storage for Ids alive. +// SAFETY: ProfilesDictionaryTranslator is Send because: +// 1. The profiles_dictionary Arc ensures the underlying storage remains alive and valid for the +// lifetime of this translator, and Arc is Send when T is Send + Sync. ProfilesDictionary is +// Send + Sync. +// 2. SetId and StringRef are non-owning handles (thin pointers) to immutable data in the +// ProfilesDictionary's concurrent collections, which use arena allocation with stable addresses. +// The Arc protects this data, making the pointers safe to send across threads. +// 3. FxIndexMap is Send when K and V are Send. The keys (SetId, StringRef) and values +// (MappingId, FunctionId, StringId) are all Copy types that are Send. unsafe impl Send for ProfilesDictionaryTranslator {} impl ProfilesDictionaryTranslator { @@ -32,7 +48,14 @@ impl ProfilesDictionaryTranslator { } } - pub fn translate_function( + /// Translates a FunctionId2 from the ProfilesDictionary into a FunctionId + /// for this profile's StringTable. + /// + /// # Safety + /// + /// The `id2` must have been created by `self.profiles_dictionary`, and + /// the strings must also live in the same dictionary. + pub unsafe fn translate_function( &mut self, functions: &mut impl Dedup, string_table: &mut StringTable, @@ -55,12 +78,19 @@ impl ProfilesDictionaryTranslator { return Ok(*internal); } - // SAFETY: todo + // SAFETY: This is safe if `id2` (the FunctionId2) was created by + // `self.profiles_dictionary`, which is a precondition of calling + // this method. let function = unsafe { *self.profiles_dictionary.functions().get(set_id) }; - let function = Function { - name: self.translate_string(string_table, function.name)?, - system_name: self.translate_string(string_table, function.system_name)?, - filename: self.translate_string(string_table, function.file_name)?, + // SAFETY: safe if the strings were made by + // `self.profiles_dictionary`, which is a precondition of + // calling this method. + let function = unsafe { + Function { + name: self.translate_string(string_table, function.name)?, + system_name: self.translate_string(string_table, function.system_name)?, + filename: self.translate_string(string_table, function.file_name)?, + } }; (Some(set_id), function) } @@ -76,7 +106,14 @@ impl ProfilesDictionaryTranslator { Ok(internal_id) } - pub fn translate_mapping( + /// Translates a MappingId2 from the ProfilesDictionary into a MappingId + /// for this profile's internal collections. + /// + /// # Safety + /// + /// The `id2` must have been created by `self.profiles_dictionary`, and + /// the strings must also live in the same dictionary. + pub unsafe fn translate_mapping( &mut self, mappings: &mut impl Dedup, string_table: &mut StringTable, @@ -93,7 +130,9 @@ impl ProfilesDictionaryTranslator { return Ok(Some(*internal)); } - // SAFETY: todo + // SAFETY: This is safe if `id2` (the MappingId2) was created by + // `self.profiles_dictionary`, which is a precondition of calling + // this method. let mapping = unsafe { *self.profiles_dictionary.mappings().get(set_id) }; let internal = Mapping { memory_start: mapping.memory_start, @@ -112,7 +151,14 @@ impl ProfilesDictionaryTranslator { Ok(Some(internal_id)) } - pub fn translate_string( + /// Translates a StringRef from the ProfilesDictionary into a StringId + /// for this profile's internal string table. + /// + /// # Safety + /// + /// The `str_ref` must have been created by `self.profiles_dictionary`. + /// Violating this precondition results in undefined behavior. + pub unsafe fn translate_string( &mut self, string_table: &mut StringTable, str_ref: StringRef, @@ -121,12 +167,11 @@ impl ProfilesDictionaryTranslator { match self.strings.entry(str_ref) { Entry::Occupied(o) => Ok(*o.get()), Entry::Vacant(v) => { + // SAFETY: This is safe if `str_ref` was created by + // `self.profiles_dictionary`, which is a precondition of calling + // this method. let str = unsafe { self.profiles_dictionary.strings().get(str_ref) }; - // SAFETY: we're keeping these lifetimes in sync. I think. - // TODO: BUT longer-term we want to avoid copying them - // entirely, so this should go away. - let decouple_str = unsafe { core::mem::transmute::<&str, &str>(str) }; - let internal_id = string_table.try_intern(decouple_str)?; + let internal_id = string_table.try_intern(str)?; v.insert(internal_id); Ok(internal_id) } From 19bb0ef2e1d23f422e8ffdbe75c84816c8ee150a Mon Sep 17 00:00:00 2001 From: Levi Morrison Date: Tue, 2 Dec 2025 22:01:38 -0700 Subject: [PATCH 16/20] docs: try_reserve_exact optimization for Box<[]> --- libdd-profiling/src/internal/profile/mod.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/libdd-profiling/src/internal/profile/mod.rs b/libdd-profiling/src/internal/profile/mod.rs index 93ad4a5973..4a4e1f9d28 100644 --- a/libdd-profiling/src/internal/profile/mod.rs +++ b/libdd-profiling/src/internal/profile/mod.rs @@ -143,6 +143,8 @@ impl Profile { let labels = { let mut lbls = Vec::new(); + // Using try_reserve_exact because it will be converted to Box<[]>, + // so excess capacity would make that conversion more expensive. lbls.try_reserve_exact(sample.labels.len())?; for label in &sample.labels { let key = self.try_intern(label.key)?; @@ -210,6 +212,8 @@ impl Profile { let labels = { let mut lbls = Vec::new(); + // Using try_reserve_exact because it will be converted to Box<[]>, + // so excess capacity would make that conversion more expensive. lbls.try_reserve_exact(labels.len())?; for label in labels { let label = label.context("profile label failed to convert")?; @@ -413,8 +417,8 @@ impl Profile { profiles_dictionary: crate::profiles::collections::Arc, ) -> io::Result { let mut owned_sample_types = Vec::new(); - // Using try_reserve_exact because it will be converted to a Box<[]>, - // so excess capacity would just make that conversion more expensive. + // Using try_reserve_exact because it will be converted to Box<[]>, + // so excess capacity would make that conversion more expensive. owned_sample_types.try_reserve_exact(sample_types.len())?; owned_sample_types.extend(sample_types.iter().map(owned_types::ValueType::from)); Self::try_new_internal( From eae2db254301f7c6eb5d498df8b0a0b9b3946ecc Mon Sep 17 00:00:00 2001 From: Levi Morrison Date: Wed, 3 Dec 2025 14:32:20 -0700 Subject: [PATCH 17/20] style: remove unused import --- libdd-profiling/src/profiles/datatypes/profiles_dictionary.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libdd-profiling/src/profiles/datatypes/profiles_dictionary.rs b/libdd-profiling/src/profiles/datatypes/profiles_dictionary.rs index 263dd32cc8..7259903b22 100644 --- a/libdd-profiling/src/profiles/datatypes/profiles_dictionary.rs +++ b/libdd-profiling/src/profiles/datatypes/profiles_dictionary.rs @@ -1,7 +1,7 @@ // Copyright 2025-Present Datadog, Inc. https://www.datadoghq.com/ // SPDX-License-Identifier: Apache-2.0 -use crate::profiles::collections::{ParallelSet, ParallelStringSet, SetError, SetId}; +use crate::profiles::collections::{ParallelSet, ParallelStringSet, SetError}; use crate::profiles::datatypes::{ Function, Function2, FunctionId2, Mapping, Mapping2, MappingId2, StringId2, }; From ef5c761f3940ae168df25e2f5e0efa7bd5549ba4 Mon Sep 17 00:00:00 2001 From: Levi Morrison Date: Wed, 3 Dec 2025 14:51:38 -0700 Subject: [PATCH 18/20] style: remove unused import --- libdd-profiling/benches/add_samples.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/libdd-profiling/benches/add_samples.rs b/libdd-profiling/benches/add_samples.rs index 62de57713c..d9743075d3 100644 --- a/libdd-profiling/benches/add_samples.rs +++ b/libdd-profiling/benches/add_samples.rs @@ -3,7 +3,6 @@ use criterion::*; use libdd_profiling::api2::Location2; -use libdd_profiling::profiles::collections::SetId; use libdd_profiling::profiles::datatypes::{Function, FunctionId2, MappingId2}; use libdd_profiling::{self as profiling, api, api2}; From b315745f47d2b1e821efc3d97b80b24fa1991e8c Mon Sep 17 00:00:00 2001 From: Levi Morrison Date: Wed, 3 Dec 2025 15:05:02 -0700 Subject: [PATCH 19/20] refactor: impl Default for internal::Function --- libdd-profiling/src/internal/function.rs | 2 +- .../internal/profile/profiles_dictionary_translator.rs | 9 +-------- 2 files changed, 2 insertions(+), 9 deletions(-) diff --git a/libdd-profiling/src/internal/function.rs b/libdd-profiling/src/internal/function.rs index 3be7292005..1291cf70a8 100644 --- a/libdd-profiling/src/internal/function.rs +++ b/libdd-profiling/src/internal/function.rs @@ -6,7 +6,7 @@ use super::*; /// Represents a [pprof::Function] with some space-saving changes: /// - The id is not stored on the struct. It's stored in the container that holds the struct. /// - ids for linked objects use 32-bit numbers instead of 64 bit ones. -#[derive(Copy, Clone, Debug, Eq, PartialEq, Hash, PartialOrd, Ord)] +#[derive(Copy, Clone, Debug, Default, Eq, PartialEq, Hash, PartialOrd, Ord)] pub struct Function { pub name: StringId, pub system_name: StringId, diff --git a/libdd-profiling/src/internal/profile/profiles_dictionary_translator.rs b/libdd-profiling/src/internal/profile/profiles_dictionary_translator.rs index 3bcc6832a5..794a7a56bc 100644 --- a/libdd-profiling/src/internal/profile/profiles_dictionary_translator.rs +++ b/libdd-profiling/src/internal/profile/profiles_dictionary_translator.rs @@ -64,14 +64,7 @@ impl ProfilesDictionaryTranslator { let (set_id, function) = match NonNull::new(id2.0) { // Since the internal model treats functions as required, we // translate the null FunctionId2 to the default function. - None => { - let function = Function { - name: StringId::ZERO, - system_name: StringId::ZERO, - filename: StringId::ZERO, - }; - (None, function) - } + None => (None, Function::default()), Some(nn) => { let set_id = SetId(nn.cast::()); if let Some(internal) = self.functions.get(&Some(set_id)) { From d21dd81beaf63c1f1eed32085a7f198b43ef37ae Mon Sep 17 00:00:00 2001 From: Levi Morrison Date: Wed, 3 Dec 2025 15:14:34 -0700 Subject: [PATCH 20/20] bench: fix filename assignment --- libdd-profiling/benches/add_samples.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/libdd-profiling/benches/add_samples.rs b/libdd-profiling/benches/add_samples.rs index d9743075d3..ca392216df 100644 --- a/libdd-profiling/benches/add_samples.rs +++ b/libdd-profiling/benches/add_samples.rs @@ -19,7 +19,7 @@ fn make_stack_api(frames: &[Frame]) -> (Vec>, Vec) { mapping, function: api::Function { name: frame.function_name, - filename: frame.function_name, + filename: frame.file_name, ..Default::default() }, line: frame.line_number as i64, @@ -52,6 +52,7 @@ struct Frame { line_number: u32, function_name: &'static str, } + impl Frame { pub const fn new( file_name: &'static str,