Add profiling_hooks feature exposing jemalloc's experimental sample hooks - #172
Add profiling_hooks feature exposing jemalloc's experimental sample hooks #172scottgerring wants to merge 3 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 4 included reviews per hour; 2 remain after this review. 📝 WalkthroughWalkthroughThe PR exposes profiling hook APIs under the ChangesProfiling controls and hooks
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to The change exposes profiling controls and hooks without any identified current merge-blocking risk. Sequence Diagram(s)sequenceDiagram
participant Caller
participant ProfilingModule
participant Jemalloc
participant Allocation
Caller->>ProfilingModule: set_prof_sample_hook
ProfilingModule->>Jemalloc: update profiling hook
Allocation->>Jemalloc: allocate or free memory
Jemalloc->>ProfilingModule: invoke profiling hook
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Title checkExplanation The title describes exposing jemalloc profiling hooks, but it incorrectly states that the pull request adds a
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Welcome @scottgerring! It looks like this is your first PR to tikv/jemallocator 🎉 |
96bfb1b to
272ebf9
Compare
272ebf9 to
5c2fc16
Compare
…ooks Signed-off-by: Scott Gerring <scott@scottgerring.com>
5c2fc16 to
ff0e303
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
jemalloc-ctl/src/profiling.rs (1)
286-329: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider making the hook setters
unsafe fn.
ProfSampleHook/ProfSampleFreeHook/ProfBacktraceHookcarry a documented# Safetycontract (no unwinding across the FFI boundary, reentrancy constraints) that the compiler cannot verify. Installing a hook that jemalloc will invoke under those constraints is the caller's responsibility, similar to signal-handler registration — conventionally exposed asunsafe fnin Rust. Currently these are safepub fns, so callers get no compiler nudge to read the safety docs. Since this API is new in this PR, tightening it now avoids a breaking change later.♻️ Proposed refactor
-pub fn set_prof_sample_hook( +pub unsafe fn set_prof_sample_hook( hook: Option<ProfSampleHook>, ) -> crate::error::Result<Option<ProfSampleHook>> { - unsafe { crate::raw::update(b"experimental.hooks.prof_sample\0", hook) } + crate::raw::update(b"experimental.hooks.prof_sample\0", hook) }(same pattern for
set_prof_sample_free_hookandset_prof_backtrace_hook; call sites inhook_testswould needunsafe { ... }wrapping.)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@jemalloc-ctl/src/profiling.rs` around lines 286 - 329, Change set_prof_sample_hook, set_prof_sample_free_hook, and set_prof_backtrace_hook to unsafe functions because callers must uphold each hook type’s FFI, unwinding, and reentrancy safety contract. Update their documentation to expose the relevant safety requirements and wrap all hook_tests call sites in unsafe blocks while preserving existing return and error behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@jemalloc-ctl/src/profiling.rs`:
- Around line 167-220: Update the `prof_active` rustdoc to state that jemalloc
initializes it from `opt.prof_active`, distinguishing it from
`thread.prof.active`, which uses `opt.prof_thread_active_init`. Also revise
`prof_reset`’s error documentation to say the `prof:true` default is provided
only by the `profiling_hooks` feature, while `profiling` alone merely enables
profiling support.
---
Nitpick comments:
In `@jemalloc-ctl/src/profiling.rs`:
- Around line 286-329: Change set_prof_sample_hook, set_prof_sample_free_hook,
and set_prof_backtrace_hook to unsafe functions because callers must uphold each
hook type’s FFI, unwinding, and reentrancy safety contract. Update their
documentation to expose the relevant safety requirements and wrap all hook_tests
call sites in unsafe blocks while preserving existing return and error behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 309da892-9803-45d0-aa0e-0ee5b495c73e
📒 Files selected for processing (6)
jemalloc-ctl/Cargo.tomljemalloc-ctl/src/macros.rsjemalloc-ctl/src/profiling.rsjemalloc-sys/Cargo.tomljemalloc-sys/README.mdjemalloc-sys/build.rs
|
@BusyJay - bump! |
8d6dbf0 to
3d81ccd
Compare
Signed-off-by: Scott Gerring <scott@scottgerring.com>
3d81ccd to
d9f07bd
Compare
Signed-off-by: Scott Gerring <scott@scottgerring.com>
d9f07bd to
b19340a
Compare
|
@BusyJay thanks for taking a look! Comments addressed 💪 |
Addresses #160, adding a
profiling_hooksfeature totikv-jemalloc-sys(forwarded throughtikv-jemalloc-ctl) exposing jemalloc's experimentalexperimental.hooks.prof_sample/prof_sample_free/prof_backtracehooks, so an external sampler (e.g. an eBPF profiler) can piggyback jemalloc's sampling decision without using its stack walking. Over in opentelemetry world, we are looking at adding heap profiling to https://github.com/open-telemetry/opentelemetry-ebpf-profiler and this would give us a way of providing sample points for it to use for folks running jemalloc.Since
jemalloc-syshaslinks = "jemalloc", there's only one build per dependency graph, so this bakesprof:true,prof_active:falseinto the defaultmalloc_conf— sampling is installable but inert until a consumer flipsprof.activeat runtime, avoiding surprises for other dependents.Also adds
prof_active(r/w/u) andprof_reset(w/o) runtime controls totikv_jemalloc_ctl::profiling(always compiled underprofiling), plus the hook setters and anoop_prof_backtrace_hookfor callers that only want the sampling clock - this is helpful, again, on the eBPF profiler side, as it has all the stack unwinding stuff built in, and we can avoid paying the cost twice.No USDT/eBPF emission included; just the hook-registration API for a downstream consumer to build on.
Summary by CodeRabbit
New Features
profilingfeature, including allocation sampling, deallocation, and backtrace hooks.Documentation
profilingfeature.Bug Fixes
profilingfeature is enabled.