perf(profiling): use an arena-based string table

The arena capacity probably needs some tweaking, and should probably be set to a specific target set by the team after careful consideration. The memory is lazily populated by the OS, at least. Adjust log levels as certain errors will now be more common. Use const thread_local for runtime stats (slightly cheaper).
DataDog · Feb 26, 2024 · 60d7dd5 · 60d7dd5
1 parent d95187a
commit 60d7dd5
Show file tree

Hide file tree

Showing 5 changed files with 45 additions and 17 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/profiling/.cargo/config → profiling/.cargo/config.toml b/profiling/.cargo/config → profiling/.cargo/config.toml
diff --git a/profiling/Cargo.toml b/profiling/Cargo.toml
@@ -20,8 +20,8 @@ bumpalo = { version = "3.12", features = ["collections"] }
 cfg-if = { version = "1.0" }
 cpu-time = { version = "1.0" }
 crossbeam-channel = { version = "0.5", default-features = false, features = ["std"] }
-datadog-profiling = { git = "https://github.com/DataDog/libdatadog", tag = "v6.0.0" }
-ddcommon = { git = "https://github.com/DataDog/libdatadog", tag = "v6.0.0" }
+datadog-profiling = { git = "https://github.com/DataDog/libdatadog", rev = "a21056a5fa3c0f54d555a978bd1f3ea9726ef0f8" }
+ddcommon = { git = "https://github.com/DataDog/libdatadog", rev = "a21056a5fa3c0f54d555a978bd1f3ea9726ef0f8" }
 env_logger = { version = "0.10" }
 indexmap = { version = "2.2.0" }
 lazy_static = { version = "1.4" }

diff --git a/profiling/src/profiling/mod.rs b/profiling/src/profiling/mod.rs
@@ -263,7 +263,10 @@ impl TimeCollector {
     /// makes sense to use an older time than now because if the profiler was
     /// running 4 seconds ago and we're only creating a profile now, that means
     /// we didn't collect any samples during that 4 seconds.
-    fn create_profile(message: &SampleMessage, started_at: SystemTime) -> InternalProfile {
+    fn create_profile(
+        message: &SampleMessage,
+        started_at: SystemTime,
+    ) -> anyhow::Result<InternalProfile> {
         let sample_types: Vec<ApiValueType> = message
             .key
             .sample_types
@@ -299,7 +302,8 @@ impl TimeCollector {
                 },
                 value: period.min(i64::MAX as u128) as i64,
             }),
-        );
+            512 * 1024 * 1024,
+        )?;
 
         #[cfg(feature = "allocation_profiling")]
         if let (Some(alloc_size_offset), Some(alloc_samples_offset)) =
@@ -333,7 +337,7 @@ impl TimeCollector {
             }
         }
 
-        profile
+        Ok(profile)
     }
 
     fn handle_resource_message(
@@ -348,8 +352,14 @@ impl TimeCollector {
         let local_root_span_id = message.local_root_span_id;
         for (_, profile) in profiles.iter_mut() {
             let endpoint = Cow::Borrowed(message.resource.as_str());
-            profile.add_endpoint(local_root_span_id, endpoint.clone());
-            profile.add_endpoint_count(endpoint, 1);
+            match profile.add_endpoint(local_root_span_id, endpoint.clone()) {
+                // todo: the count being broadcast to all profiles is probably
+                //       incorrect because it will be over-counted.
+                Ok(_) => profile.add_endpoint_count(endpoint, 1),
+                Err(err) => {
+                    debug!("failed to add endpoint for local root span id {local_root_span_id}: {err}")
+                }
+            }
         }
     }
 
@@ -367,10 +377,11 @@ impl TimeCollector {
         let profile: &mut InternalProfile = if let Some(value) = profiles.get_mut(&message.key) {
             value
         } else {
-            profiles.insert(
-                message.key.clone(),
-                Self::create_profile(&message, started_at.systemtime),
-            );
+            let Ok(profile) = Self::create_profile(&message, started_at.systemtime) else {
+                error!("Failed to create a new profile. Please report this to Datadog.");
+                return;
+            };
+            profiles.insert(message.key.clone(), profile);
             profiles
                 .get_mut(&message.key)
                 .expect("entry to exist; just inserted it")
@@ -407,7 +418,9 @@ impl TimeCollector {
         match profile.add_sample(sample, timestamp) {
             Ok(_id) => {}
             Err(err) => {
-                warn!("Failed to add sample to the profile: {err}")
+                // If the string table becomes full, this could flood customer
+                // logs, which is why I've chosen this level.
+                debug!("Failed to add sample to the profile: {err}")
             }
         }
     }

diff --git a/profiling/src/profiling/stalk_walking.rs b/profiling/src/profiling/stalk_walking.rs
@@ -8,7 +8,7 @@ use std::str::Utf8Error;
 /// the fact that there are two cache slots used, and they don't have to be in
 /// sync. However, they usually are, so we simplify.
 #[cfg(php_run_time_cache)]
-#[derive(Debug, Default)]
+#[derive(Debug)]
 pub struct FunctionRunTimeCacheStats {
     hit: usize,
     missed: usize,
@@ -17,16 +17,31 @@ pub struct FunctionRunTimeCacheStats {
 
 #[cfg(php_run_time_cache)]
 impl FunctionRunTimeCacheStats {
+    pub const fn new() -> Self {
+        Self {
+            hit: 0,
+            missed: 0,
+            not_applicable: 0,
+        }
+    }
+
     pub fn hit_rate(&self) -> f64 {
         let denominator = (self.hit + self.missed + self.not_applicable) as f64;
         self.hit as f64 / denominator
     }
 }
 
+#[cfg(php_run_time_cache)]
+impl Default for FunctionRunTimeCacheStats {
+    fn default() -> Self {
+        Self::new()
+    }
+}
+
 #[cfg(php_run_time_cache)]
 thread_local! {
     static CACHED_STRINGS: RefCell<StringTable> = RefCell::new(StringTable::new());
-    pub static FUNCTION_CACHE_STATS: RefCell<FunctionRunTimeCacheStats> = RefCell::new(Default::default())
+    pub static FUNCTION_CACHE_STATS: RefCell<FunctionRunTimeCacheStats> = const { RefCell::new(FunctionRunTimeCacheStats::new()) };
 }
 
 /// # Safety