dd-trace-php icon indicating copy to clipboard operation
dd-trace-php copied to clipboard

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

Open morrisonlevi opened this issue 1 year ago • 2 comments

Description

This updates libdatadog to use a branch which has an arena-based string table. See Datadog/libdatadog#306.

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, so it's not automatically used (asking for 512 MiB won't allocate all 512 MiB immediately, it will grow up to that limit and then stop).

Part of PROF-9123.

Overhead

Using the symfony/demo with PHP 8.3 using php-fpm, this netted me about 3-4% less total container memory. I expected there to be a difference because:

  1. There is less memory allocator fragmentation going on as strings are variable size.
  2. I'm packing the string size into a u16, so we save a few bytes per string compared to a usize.

I'm happy with 3-4% off the container, and don't feel a need to try to specifically calculate the reduction of just libdatadog's memory.

I haven't attempted to measure CPU rigorously. Unscientifically, it looks on par with master. I would expect a very slight win with this PR because arena allocators are quick to allocate, and we can skip deallocation altogether (but deallocation is infrequent, roughly once per minute). Since eyeballing it looks on-par with master, which was the expectation, I won't dive into it deeper.

Reviewer checklist

  • [ ] Test coverage seems ok.
  • [ ] Appropriate labels assigned.

morrisonlevi avatar Feb 08 '24 22:02 morrisonlevi

Benchmarks

Benchmark execution time: 2024-03-28 14:09:47

Comparing candidate commit b1d3e7cf4a89ba4aaf74961f654436427cd48d4d in PR branch levi/arena-string-table with baseline commit 8e98f4de9f6e50c5a59b1fee4fd14eb1f4c5c172 in branch master.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 18 metrics, 3 unstable metrics.

pr-commenter[bot] avatar Feb 08 '24 22:02 pr-commenter[bot]

Codecov Report

Merging #2511 (8d65bce) into master (2e1ed12) will increase coverage by 1.16%. Report is 4 commits behind head on master. The diff coverage is n/a.

:exclamation: Current head 8d65bce differs from pull request most recent head c1d7657. Consider uploading reports for the commit c1d7657 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2511      +/-   ##
============================================
+ Coverage     75.91%   77.08%   +1.16%     
  Complexity     2574     2574              
============================================
  Files           240      214      -26     
  Lines         27029    23057    -3972     
  Branches        976        0     -976     
============================================
- Hits          20519    17773    -2746     
+ Misses         5990     5284     -706     
+ Partials        520        0     -520     
Flag Coverage Δ
appsec-extension ?
tracer-extension 78.70% <ø> (ø)
tracer-php 75.08% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

see 26 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 2e1ed12...c1d7657. Read the comment docs.

codecov-commenter avatar Mar 06 '24 19:03 codecov-commenter

Superseded by #2668.

morrisonlevi avatar May 31 '24 15:05 morrisonlevi