feat(profiling)!: add arena-based string table #306

morrisonlevi · 2024-02-07T22:28:53Z

What does this PR do?

Creates a string table that stores the bytes of the strings into an arena, instead of using individual allocations. The arena has a maximum capacity, and uses virtual memory to acquire pages lazily.

This is a breaking change:

Many routines in Rust which weren't fallible now are to account for the string table hitting its capacity, or if initial allocations fail.
The minimum string arena capacity in bytes must be passed to ddog_prof_Profile_new.

Motivation

I'm working on reducing overhead of the PHP profiler, and reducing memory of the string table is a nice step. It also puts less pressure onto the system allocator, which should help others like the Ruby profiler as well.

Additional Notes

The PR is quite large. Maybe I should split it into two pieces, but it's kind of hard to know where to split it.

PROF-9125

How to test the change?

The function ddog_prof_Profile_new has a new parameter for the minimum size of the arena, which will round up to the nearest page size for you. The memory is virtually allocated, so it will be paged in by the OS as needed. You don't need to worry too much about its size, as long as you are being reasonable (e.g. 2 GiB may be fine, 2 TiB is not).

For Reviewers

If this PR touches code that signs or publishes builds or packages, or handles credentials of any kind, I've requested a review from @DataDog/security-design-and-guidance.
This PR doesn't touch any of that.

morrisonlevi · 2024-02-09T17:03:29Z

In PHP, I got a 3-4% memory savings, ~2.22MiB, on the containers I tested. I'm happy with this number for PHP and look forward to hearing numbers from any other runtime which can test it.

This is a breaking change: 1. Many routines which weren't fallable now are. 2. The minimum string arena capacity must be passed to the profile.

ci skip

gleocadie · 2024-03-29T23:28:12Z

profiling-ffi/src/profiles.rs

 /// and must have the correct number of elements for the slice.
 #[no_mangle]
 #[must_use]
 pub unsafe extern "C" fn ddog_prof_Profile_new(
    sample_types: Slice<ValueType>,
    period: Option<&Period>,
    start_time: Option<&Timespec>,
+    string_arena_min_capacity: libc::size_t,


Do we really need to expose this to the client libraries ? I feel like that should be internal.
To be honest, when I create a profile object I do not know the minimal.
I feel like it's worth hiding it (since it's a technical detail)

It's exposed because every runtime has different ideas about how much memory usage is okay. You making this comment made me realize that "min_capacity" is not the best name: it rounds up to a page size, and if you exceed that it will fail. So this number should be close to the maximum you want. It's lazily allocated, so you don't need to worry too much about the memory unless you actually use it, as long as your request is sane (you know, like 1 GiB, not 1 TiB).

It's exposed because every runtime has different ideas about how much memory usage is okay.

Hum I see, but for .NET I do not know what would be a good value.
It will push libraries to use a big value (since it's lazy) and forget about it. Going that way, having it internal still makes sense.
If you still want to let the libraries to set this value, you could make it optional no?

gleocadie · 2024-03-29T23:28:54Z

profiling-ffi/src/profiles.rs

-    ProfileNewResult::Ok(ffi_profile)
+    match internal::Profile::new(start_time, &types, period, string_arena_min_capacity) {
+        Ok(internal_profile) => ProfileNewResult::Ok(Profile::new(internal_profile)),
+        Err(err) => ProfileNewResult::Err(Error::from(err.context("ddog_prof_Profile_new failed"))),


maybe add a test that actually fails no?

gleocadie · 2024-03-29T23:29:08Z

profiling-ffi/src/profiles.rs

@@ -468,8 +469,13 @@ pub unsafe extern "C" fn ddog_prof_Profile_set_endpoint(
        }
    };
    let endpoint = endpoint.to_utf8_lossy();
-    profile.add_endpoint(local_root_span_id, endpoint);
-    ProfileResult::Ok(true)
+    if let Err(err) = profile.add_endpoint(local_root_span_id, endpoint) {


same comment about testing

This helps keep the APIs for various platforms in sync. It also helps with testing, to ensure the code that handles virtual_alloc failures actually works.

This is a more accurate name that suggests the exact value will not necessarily be used.

danielsn · 2024-04-01T17:16:30Z

profiling-ffi/src/profiles.rs

@@ -884,6 +893,7 @@ mod test {
            Slice::from_raw_parts(sample_type, 1),
            None,
            None,
+            4096,


Do we have any tests that vary this value?

Not specifically in FFI, no. There are tests on arena and string table for small values like 64, and there is a test on string table which does 4 * u16::MAX or in other words, a number just under 256 KiB.

But not currently testing other values in FFI.

danielsn · 2024-04-01T17:16:51Z

profiling-replayer/src/main.rs

@@ -151,7 +151,8 @@ fn main() -> anyhow::Result<()> {
        replayer.start_time,
        &replayer.sample_types,
        replayer.period,
-    );
+        u32::MAX as usize,


why this value?

I had to provide some value. A max of 4 GiB seemed okay for request replayer.

profiling/src/alloc/arena.rs

danielsn · 2024-04-01T17:32:48Z

profiling/src/alloc/arena.rs

+        let layout = layout.pad_to_align();
+        let size = layout.size();
+        if size == 0 {
+            return Ok(NonNull::from(&mut []));


Why not NonNull::dangling()

Because it's a wide pointer NonNull<[u8]>, so we also need size info.

error[E0277]: the size for values of type `[u8]` cannot be known at compilation time --> profiling/src/alloc/arena.rs:70:23 | 70 | return Ok(NonNull::dangling()); | ^^^^^^^^^^^^^^^^^ doesn't have a size known at compile-time | = help: the trait `Sized` is not implemented for `[u8]` note: required by a bound in `NonNull::<T>::dangling` --> /Users/levi.morrison/.rustup/toolchains/1.69.0-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/ptr/non_null.rs:67:6 | 67 | impl<T: Sized> NonNull<T> { | ^ required by this bound in `NonNull::<T>::dangling`

profiling/src/alloc/arena.rs

danielsn · 2024-04-01T17:57:02Z

profiling/src/alloc/arena.rs

+        // but in practice, it won't fail because it's rounded to a page size,
+        // and I've never seen pages that small, even for 16 bit. However, in
+        // any case, it should not panic, which is the point of the call.
+        _ = std::hint::black_box(arena.allocate_zeroed(Layout::new::<u8>()));


TIL that we don't need a let before _

danielsn · 2024-04-01T17:58:26Z

profiling/src/alloc/arena.rs

+
+    #[test]
+    fn test_arena_simple_alignment() -> anyhow::Result<()> {
+        const DISTANCE: usize = 16;


Is there a reason that DISTANCE has the opposite meaning in this vs the previous test?

danielsn · 2024-04-01T17:59:15Z

profiling/src/alloc/arena.rs

+        let third = arena.allocate_zeroed(pointer)?;
+
+        expect_distance(first, second, mem::size_of::<*const ()>());
+        expect_distance(second, third, mem::size_of::<*const ()>());


Also directly test that the pointers are aligned

profiling/src/alloc/arena.rs

danielsn · 2024-04-01T18:08:25Z

profiling/src/internal/value_type.rs

-    pub r#type: StringId,
-    pub unit: StringId,
+pub(crate) struct ValueType {
+    pub r#type: (Option<LengthPrefixedStr>, StringId),


Why do we need both of these?

danielsn · 2024-04-01T20:31:07Z

profiling/src/alloc/mod.rs

+use std::sync::Once;
+
+#[derive(Copy, Clone, Debug, Eq, PartialEq)]
+pub struct AllocError;


Do we want opaque or discriminated errors on this API

danielsn · 2024-04-01T20:33:08Z

profiling/src/alloc/mod.rs

+    static INIT: Once = Once::new();
+    static mut PAGE_SIZE: usize = 0;
+
+    unsafe {
+        INIT.call_once(|| {
+            PAGE_SIZE = os::page_size().unwrap();
+        });
+        PAGE_SIZE
+    }
+}


Could this be a lazy_static?

danielsn · 2024-04-01T20:45:08Z

profiling/src/alloc/virtual.rs

+    /// The fatptr must have been previously allocated by [virtual_alloc] by
+    /// this allocator, and must have the same address and length as it was
+    /// returned with.


And must not have been previously freed

danielsn · 2024-04-01T20:50:18Z