-
Notifications
You must be signed in to change notification settings - Fork 150
Revert to jemalloc allocator #3954
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,6 +11,9 @@ use { | |
| /// When "dump" command is sent, it generates a heap profile using | ||
| /// jemalloc_pprof and streams the binary protobuf data back through the socket. | ||
| /// | ||
| /// Profiling is enabled at runtime via the MALLOC_CONF environment variable. | ||
| /// Set MALLOC_CONF=prof:true to enable heap profiling. | ||
| /// | ||
| /// Usage: | ||
| /// ```bash | ||
| /// # From your local machine (one-liner): | ||
|
|
@@ -19,21 +22,19 @@ use { | |
| /// # Analyze with pprof: | ||
| /// go tool pprof -http=:8080 heap.pprof | ||
| /// ``` | ||
| #[cfg(all(unix, feature = "jemalloc-profiling"))] | ||
| pub fn spawn_heap_dump_handler() { | ||
| // Check if jemalloc profiling is available before spawning the handler | ||
| // This prevents panics that would crash the entire process | ||
| // Check if jemalloc profiling is available at runtime | ||
| // This depends on whether MALLOC_CONF=prof:true was set | ||
| let profiling_available = | ||
| std::panic::catch_unwind(|| jemalloc_pprof::PROF_CTL.as_ref().is_some()).unwrap_or(false); | ||
|
|
||
| if !profiling_available { | ||
| tracing::warn!( | ||
| "jemalloc profiling not available - heap dump handler not started. Ensure service is \ | ||
| built with jemalloc-profiling feature and MALLOC_CONF is set." | ||
| ); | ||
| // Profiling is disabled - do nothing | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is not merge blocking but long term it would be nice if we could always spawn the dump handler and also support enabling profiling while the process is already running. This is possible with jemalloc but I'm not sure if just updating the env variable after starting the process would be sufficient for that.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unfortunately, it doesn't work well with delayed profiling(tikv/jemallocator#140). Probably, if the profiler missed the root allocation, it is not possible to record detached children, I don't know. Also, this overcomplicates the solution, where the unix socket connection would need to accept more commands, etc. Anyway, I can try to do that in the future. But since the enabled profiling doesn't really affect the performance, this is not so critical.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This is already implemented for the filter reloading logic so not terrible. But obviously if the feature doesn't work well in jemalloc itself it clearly doesn't make sense to add this complexity.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This is how the memory profiler dumps currently work as well. |
||
| return; | ||
| } | ||
|
|
||
| tracing::info!("jemalloc heap profiling is active"); | ||
|
|
||
| tokio::spawn(async move { | ||
| let name = binary_name().unwrap_or("unknown".to_string()); | ||
| let socket_path = format!("/tmp/heap_dump_{name}.sock"); | ||
|
|
||
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.
Very nice! This can be made even more user friendly by using
type: choiceand providing a list ofoptions(see here). That way you don't even have to know the exact feature names. (obviously runs the risk of having the action out of sync with the actually supported features).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.
Yeah, I was thinking about a list, but that would be more difficult to test new features since, in that case, you would need to provide custom inputs. AFAIK, GH doesn’t support a hybrid "preset options + custom text" input. A
choiceinput only accepts values listed in options.