Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 19 commits into from

Conversation

morrisonlevi
Copy link
Collaborator

@morrisonlevi morrisonlevi commented Feb 8, 2024

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.

@github-actions github-actions bot added the profiling Relates to the Continuous Profiler label Feb 8, 2024
@pr-commenter
Copy link

pr-commenter bot commented Feb 8, 2024

Benchmarks

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

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

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

@morrisonlevi morrisonlevi force-pushed the levi/arena-string-table branch 2 times, most recently from e614517 to c7864cb Compare February 26, 2024 00:23
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).
@codecov-commenter
Copy link

codecov-commenter commented Mar 6, 2024

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.

❗ 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.

@morrisonlevi
Copy link
Collaborator Author

Superseded by #2668.

@morrisonlevi morrisonlevi deleted the levi/arena-string-table branch May 31, 2024 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
profiling Relates to the Continuous Profiler
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants