-
Notifications
You must be signed in to change notification settings - Fork 890
Add jemalloc profiling via HTTP API #7746
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
base: unstable
Are you sure you want to change the base?
Conversation
common/malloc_utils/src/jemalloc.rs
Outdated
let cstr = CStr::from_bytes_with_nul(&terminated_filename) | ||
.map_err(|e| format!("Cannot convert filename to CStr: {e:?}"))?; | ||
|
||
unsafe { raw::write("prof.dump".as_ref(), cstr) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, looking at the source this corresponds to mallctl("prof.dump", NULL, NULL, filename, size)
which is exactly what we want
Ref:
jemalloc-profilg
featurejemalloc-profiling
feature
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Let's merge before this bitrots (again)!!
Actually, don't we need some way to trigger this? Paul had an HTTP API I think |
This pull request has been removed from the queue for the following reason: Pull request #7746 has been dequeued. The pull request rule doesn't match anymore. The following conditions don't match anymore:
You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it. |
89a8ff3
to
e1a7178
Compare
Sorry, I didn't include everything - I've pushed all changes now and I'm going to test it first as there were quite a bit of conflicts and changes that I had to make. |
Co-authored-by: Paul Hauner <[email protected]>
a73c418
to
adfc523
Compare
jemalloc-profiling
feature
Got a bit stuck with getting debug symbols in the dump, will come back to this later. |
Managed to get this to work, I've added a page to the lighthouse book with the instructions. |
Although we're working on jemalloc profiler support in #7746, heaptrack seems to be producing more sensible results. This PR adds a heaptrack profile and a heaptrack feature so that we no longer need to patch the code in order to use heaptrack. This may prove complementary to jemalloc profiling, so I think there is no harm in having both.
@michaelsproul I feel like we probably should try to merge this even if the output wasn't very useful this time - it's likely at some point in the future we're going to come back and look for this branch and try to revive it (happened twice already!) We can probably try both jemalloc profiling and heaptrack again after the OOM fix. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good now for the documentation side of the change
Issue Addressed
Add jemalloc profiling via HTTP API.
This is included at build time if
jemalloc
is enabled, however not activated by default. User will need to enable profiling via the curl command below.Commands
Additional notes
components_by_range_requests
#7767)might need a way to purge samples in memory- I don't think this is strictly necessary at this point. We can add it if we need it later.