perf: less mem per entry in the string table #303
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This uses a `Box<str>` instead of `String`, saving 1 `usize` per entry. When it's serialized to pprof proto, it gets encoded directly without allocating a temporary `String`. This currently is waiting on a PR in prost to expose the needed function to directly encode `AsRef<str>` instead of `&String`. This part enables future optimizations like using a memory arena for string table data. PROF-9125
morrisonlevi
force-pushed
the
levi/box-str
branch
from
fd3f65f
to
5239206
Compare
|
Nice! :D |
The PR didn't receive immediate attention from a maintainer, so let's not stay blocked when it's such a small thing.
danielsn
reviewed
Feb 9, 2024
Comment on lines
-88
to
-93
| #[derive(Eq, Hash, PartialEq, ::prost::Message)] | ||
| pub struct ProfileStringTableEntry { | ||
| #[prost(string, required, tag = "6")] | ||
| pub string_table_entry: String, | ||
| } | ||
|
|
There was a problem hiding this comment.
Why the change to do this differently from all the other field serializers?
There was a problem hiding this comment.
Because prost::Message requires a String to encode strings. You can't encode any other String-like representations such as &str, Box<str>, etc with a prost::Message. Doing it this way avoids an unnecessary conversion to String. A Box<str> into String is cheap, but in the next iteration the string data will be arena-allocated so it doesn't make sense to invest too much into Box<str> when we don't need anything more than this PR for the longer-term goal, and we'd throw away the Box<str> specific stuff.
danielsn
reviewed
Feb 9, 2024
morrisonlevi
changed the title
morrisonlevi
force-pushed
the
levi/box-str
branch
from
1bed9d5
to
03e808d
Compare
bantonsson
approved these changes
Feb 13, 2024
ivoanjo
added a commit
to DataDog/dd-trace-rb
that referenced
this pull request
Mar 19, 2024
**What does this PR do?** This PR upgrades dd-trace-rb to use libdatadog 7. There was a small breaking API change -- the rename of `ddog_Endpoint` APIs to `ddog_prof_Endpoint`. **Motivation:** Make sure Ruby is up-to-date on libdatadog. **Additional Notes:** As far as Ruby is impacted, the main changes in libdatadog 7 are a number of fixes to the crash tracker (getting us closer to merging #3384) as well as some potential memory improvements from (DataDog/libdatadog#303). I'm opening this as a draft PR as libdatadog 7 is not yet available on rubygems.org, and I'll come back to re-trigger CI and mark this as non-draft once it is. **How to test the change?** Our existing test coverage includes libdatadog testing, so a green CI is good here :)
2 tasks
ivoanjo
added a commit
to DataDog/dd-trace-rb
that referenced
this pull request
Mar 27, 2024
**What does this PR do?** This PR upgrades dd-trace-rb to use libdatadog 7. There was a small breaking API change -- the rename of `ddog_Endpoint` APIs to `ddog_prof_Endpoint`. **Motivation:** Make sure Ruby is up-to-date on libdatadog. **Additional Notes:** As far as Ruby is impacted, the main changes in libdatadog 7 are a number of fixes to the crash tracker (getting us closer to merging #3384) as well as some potential memory improvements from (DataDog/libdatadog#303). I'm opening this as a draft PR as libdatadog 7 is not yet available on rubygems.org, and I'll come back to re-trigger CI and mark this as non-draft once it is. **How to test the change?** Our existing test coverage includes libdatadog testing, so a green CI is good here :)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR do?
This uses a
Box<str>instead ofString, saving 1usizeper entry. When it's serialized to pprof proto, it gets encoded directly without allocating a temporaryString.Motivation
I'm working on reducing memory and CPU overhead of profiles and noticed this pretty cheap reduction in memory.
Additional Notes
I've opened a PR upstream to put a generic str encoder helper there, as I think others could benefit from it as well. This part enables future optimizations like using a memory arena for string table data.
PROF-9125
How to test the change?
Test the same as usual, public API should be the same.
For Reviewers
@DataDog/security-design-and-guidance.