Skip to content

Add jemalloc heap profiling to moq-relay#1194

Merged
kixelated merged 8 commits into
mainfrom
jemalloc-profiling
Apr 3, 2026
Merged

Add jemalloc heap profiling to moq-relay#1194
kixelated merged 8 commits into
mainfrom
jemalloc-profiling

Conversation

@kixelated

Copy link
Copy Markdown
Collaborator

Summary

  • Adds optional jemalloc cargo feature to moq-relay with heap profiling support
  • SIGUSR1 signal handler dumps heap profiles to /tmp/moq-relay.heap.<n> for analysis with jeprof
  • Enabled by default in Nix builds; MALLOC_CONF set in the systemd unit
  • No impact on non-Nix builds (feature is opt-in via --features jemalloc)

Context

Investigating memory growth on usc.cdn.moq.dev (moq-relay at 3.2GB RSS after 2 days). This adds the tooling to capture heap profiles on the running CDN servers.

Usage

# Dump a heap profile from a running relay
kill -USR1 <pid>

# Analyze with jeprof
jeprof --svg /path/to/moq-relay /tmp/moq-relay.heap.0 > heap.svg

Test plan

  • cargo check -p moq-relay (without feature — no change)
  • cargo check -p moq-relay --features jemalloc (with feature)
  • Deploy to CDN, trigger heap dump, verify profile output

🤖 Generated with Claude Code

Optional `jemalloc` cargo feature that replaces the system allocator with
jemalloc and enables heap profiling. Send SIGUSR1 to dump a profile to
/tmp/moq-relay.heap.<n> for analysis with jeprof.

Enabled by default in Nix builds with MALLOC_CONF set in the systemd unit.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Apr 2, 2026

Copy link
Copy Markdown
Contributor

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 7b70852f-314d-460c-bad1-e029da2d11e1

📥 Commits

Reviewing files that changed from the base of the PR and between 0ee2142 and 2018ae2.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (2)
  • rs/moq-relay/Cargo.toml
  • rs/moq-relay/src/jemalloc.rs
🚧 Files skipped from review as they are similar to previous changes (2)
  • rs/moq-relay/Cargo.toml
  • rs/moq-relay/src/jemalloc.rs

Walkthrough

Adds an optional Cargo feature jemalloc with two optional dependencies and exposes a conditional jemalloc module and a pub async fn run() that manages jemalloc profiling and triggers dumps on SIGUSR1. When jemalloc is enabled the crate sets tikv_jemallocator::Jemalloc as the global allocator and runs the jemalloc future from main. Nix changes enable the feature in the overlay, disable fortify hardening, add services.moq-relay.heapDumpPrefix, and inject MALLOC_CONF="prof:true,prof_active:true,prof_prefix:${cfg.heapDumpPrefix}" into the systemd service.

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Add jemalloc heap profiling to moq-relay' accurately and concisely describes the main change: adding jemalloc heap profiling functionality to the moq-relay service.
Description check ✅ Passed The description is directly related to the changeset, providing clear context about the jemalloc feature addition, usage examples, testing status, and motivation for the change.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch jemalloc-profiling
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch jemalloc-profiling

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@rs/moq-relay/src/jemalloc_prof.rs`:
- Around line 34-39: The code hard-codes dump path to /tmp in jemalloc_prof.rs;
change the path construction to read a configurable directory (e.g. from an env
var like JEMALLOC_PROF_DUMP_DIR or a runtime config) and fall back to
std::env::temp_dir() if unset, ensure you join the directory with the filename
using Path/PathBuf (keeping the seq suffix and the nul terminator for
raw::write), validate/create the directory if necessary, and replace the direct
format!("/tmp/...") with this configurable path so raw::write(b"prof.dump\0",
path.as_ptr()) uses the configured location (refer to the path variable, seq
counter, and raw::write call in your change).
- Around line 12-14: The jemalloc profiling activation currently ignores errors
from unsafe raw::write(prof_active, true); update the activation code (the block
that calls tracing::info!("jemalloc profiling compiled in; activating") and then
unsafe { raw::write(prof_active, true) }) to handle the Result: match or use
.map_err/.unwrap_or_else to detect failure, log the error with tracing::error
(include the error details and context mentioning prof_active/raw::write), and
then decide a clear recovery action (propagate the error from the surrounding
init function or abort startup) so failures are not silently ignored.
- Around line 21-29: The call to tokio::spawn that invokes signal_handler() and
the async fn signal_handler() itself use Unix-only APIs and must be guarded by
cfg(unix) to avoid compile failures on non-Unix targets; wrap the tokio::spawn {
... } block with #[cfg(unix)] (or an equivalent cfg_if) and annotate or gate the
signal_handler() definition with #[cfg(unix)] (and provide a no-op or absent
alternative for non-unix builds) so the jemalloc feature builds on non-Unix
platforms without referencing tokio::signal::unix APIs.
🪄 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: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: fc325b23-0666-4654-8b4d-9b0fceb421c7

📥 Commits

Reviewing files that changed from the base of the PR and between c2fb7e0 and d82cd27.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (6)
  • nix/modules/moq-relay.nix
  • nix/overlay.nix
  • rs/moq-relay/Cargo.toml
  • rs/moq-relay/src/jemalloc_prof.rs
  • rs/moq-relay/src/lib.rs
  • rs/moq-relay/src/main.rs

Comment thread rs/moq-relay/src/jemalloc.rs
Comment thread rs/moq-relay/src/jemalloc_prof.rs Outdated
Comment on lines +21 to +29
tokio::spawn(async {
if let Err(err) = signal_handler().await {
tracing::error!(%err, "jemalloc signal handler failed");
}
});
}

async fn signal_handler() -> anyhow::Result<()> {
let mut sig = tokio::signal::unix::signal(tokio::signal::unix::SignalKind::user_defined1())?;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail
# Verify unix-only API usage and cfg guards around jemalloc profiling wiring.
rg -n -C3 'tokio::signal::unix|cfg\(unix\)|cfg\(feature = "jemalloc"\)|jemalloc_prof::init|pub mod jemalloc_prof' \
  rs/moq-relay/src/jemalloc_prof.rs \
  rs/moq-relay/src/main.rs \
  rs/moq-relay/src/lib.rs

Repository: moq-dev/moq

Length of output: 2050


🏁 Script executed:

cat -n rs/moq-relay/src/jemalloc_prof.rs

Repository: moq-dev/moq

Length of output: 1622


Guard Unix-only signal handling behind cfg(unix).

Lines 21-25 spawn a task calling signal_handler(), which uses Unix-only Tokio APIs at line 29. When --features jemalloc is used on non-Unix targets, this causes compilation to fail. Add platform guards to both the spawn site and the function definition.

Proposed fix
 pub fn init() {
 	let prof_active = b"prof.active\0";
 
 	match unsafe { raw::read::<bool>(prof_active) } {
 		Ok(true) => tracing::info!("jemalloc heap profiling is active"),
 		Ok(false) => {
 			tracing::info!("jemalloc profiling compiled in; activating");
 			unsafe { raw::write(prof_active, true) }.ok();
 		}
 		Err(err) => {
 			tracing::warn!(%err, "jemalloc profiling not available — set MALLOC_CONF=prof:true to enable");
 			return;
 		}
 	}
 
+	#[cfg(unix)]
 	tokio::spawn(async {
 		if let Err(err) = signal_handler().await {
 			tracing::error!(%err, "jemalloc signal handler failed");
 		}
 	});
+
+	#[cfg(not(unix))]
+	tracing::warn!("jemalloc profiling initialized, but SIGUSR1 heap dumps are only supported on unix targets");
 }
 
+#[cfg(unix)]
 async fn signal_handler() -> anyhow::Result<()> {
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
tokio::spawn(async {
if let Err(err) = signal_handler().await {
tracing::error!(%err, "jemalloc signal handler failed");
}
});
}
async fn signal_handler() -> anyhow::Result<()> {
let mut sig = tokio::signal::unix::signal(tokio::signal::unix::SignalKind::user_defined1())?;
pub fn init() {
let prof_active = b"prof.active\0";
match unsafe { raw::read::<bool>(prof_active) } {
Ok(true) => tracing::info!("jemalloc heap profiling is active"),
Ok(false) => {
tracing::info!("jemalloc profiling compiled in; activating");
unsafe { raw::write(prof_active, true) }.ok();
}
Err(err) => {
tracing::warn!(%err, "jemalloc profiling not available — set MALLOC_CONF=prof:true to enable");
return;
}
}
#[cfg(unix)]
tokio::spawn(async {
if let Err(err) = signal_handler().await {
tracing::error!(%err, "jemalloc signal handler failed");
}
});
#[cfg(not(unix))]
tracing::warn!("jemalloc profiling initialized, but SIGUSR1 heap dumps are only supported on unix targets");
}
#[cfg(unix)]
async fn signal_handler() -> anyhow::Result<()> {
let mut sig = tokio::signal::unix::signal(tokio::signal::unix::SignalKind::user_defined1())?;
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@rs/moq-relay/src/jemalloc_prof.rs` around lines 21 - 29, The call to
tokio::spawn that invokes signal_handler() and the async fn signal_handler()
itself use Unix-only APIs and must be guarded by cfg(unix) to avoid compile
failures on non-Unix targets; wrap the tokio::spawn { ... } block with
#[cfg(unix)] (or an equivalent cfg_if) and annotate or gate the signal_handler()
definition with #[cfg(unix)] (and provide a no-op or absent alternative for
non-unix builds) so the jemalloc feature builds on non-Unix platforms without
referencing tokio::signal::unix APIs.

Comment thread rs/moq-relay/src/jemalloc_prof.rs Outdated
Comment on lines +34 to +39
let path = format!("/tmp/moq-relay.heap.{seq}\0");
seq += 1;

match unsafe { raw::write(b"prof.dump\0", path.as_ptr()) } {
Ok(()) => tracing::info!(path = &path[..path.len() - 1], "heap profile dumped"),
Err(err) => tracing::error!(%err, "failed to dump heap profile"),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Avoid hard-coded /tmp for heap dumps.

Line 34 hard-codes dump location. Heap profiles can contain sensitive data; path should be configurable so deployments can use restricted, persistent storage.

Proposed fix
-		let path = format!("/tmp/moq-relay.heap.{seq}\0");
+		let prefix =
+			std::env::var("MOQ_HEAP_PROFILE_PREFIX").unwrap_or_else(|_| "/tmp/moq-relay.heap".to_string());
+		let path = format!("{prefix}.{seq}\0");
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
let path = format!("/tmp/moq-relay.heap.{seq}\0");
seq += 1;
match unsafe { raw::write(b"prof.dump\0", path.as_ptr()) } {
Ok(()) => tracing::info!(path = &path[..path.len() - 1], "heap profile dumped"),
Err(err) => tracing::error!(%err, "failed to dump heap profile"),
let prefix =
std::env::var("MOQ_HEAP_PROFILE_PREFIX").unwrap_or_else(|_| "/tmp/moq-relay.heap".to_string());
let path = format!("{prefix}.{seq}\0");
seq += 1;
match unsafe { raw::write(b"prof.dump\0", path.as_ptr()) } {
Ok(()) => tracing::info!(path = &path[..path.len() - 1], "heap profile dumped"),
Err(err) => tracing::error!(%err, "failed to dump heap profile"),
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@rs/moq-relay/src/jemalloc_prof.rs` around lines 34 - 39, The code hard-codes
dump path to /tmp in jemalloc_prof.rs; change the path construction to read a
configurable directory (e.g. from an env var like JEMALLOC_PROF_DUMP_DIR or a
runtime config) and fall back to std::env::temp_dir() if unset, ensure you join
the directory with the filename using Path/PathBuf (keeping the seq suffix and
the nul terminator for raw::write), validate/create the directory if necessary,
and replace the direct format!("/tmp/...") with this configurable path so
raw::write(b"prof.dump\0", path.as_ptr()) uses the configured location (refer to
the path variable, seq counter, and raw::write call in your change).

kixelated and others added 2 commits April 2, 2026 15:09
… path

- Replace spawned task with async run() in tokio::select! like other modules
- Delegate dump path to jemalloc's prof_prefix via MALLOC_CONF
- Add configurable heapDumpPrefix option to the Nix module
- Rename jemalloc_prof.rs to jemalloc.rs

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
@kixelated kixelated enabled auto-merge (squash) April 2, 2026 22:12
kixelated and others added 2 commits April 2, 2026 15:20
- Sort tikv-jemalloc-ctl and tikv-jemallocator in Cargo.toml (CI fix)
- Return error on activation failure instead of pending future
- Use debug! instead of warn! when profiling isn't configured
- Return Ok(()) when profiling unavailable instead of pending forever
- Merge main

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@nix/modules/moq-relay.nix`:
- Around line 146-149: The default heap dump prefix uses /tmp which is private
because PrivateTmp is enabled; update the heapDumpPrefix default to use
cfg.stateDir (e.g., join cfg.stateDir with "moq-relay.heap") so dumps land in
the service's allowed ReadWritePaths, and make the same change for the other
heap/profile default(s) referenced later (the entries on lines ~216-217) instead
of /tmp; alternatively, if you truly need host-visible /tmp paths, disable
PrivateTmp in the systemd service unit (PrivateTmp = false).

In `@rs/moq-relay/src/jemalloc.rs`:
- Around line 30-33: The jemalloc prof.dump call is incorrectly passing a
non-null empty C-string (b"\0") which jemalloc treats as a filename; change the
second argument to a true NULL pointer so jemalloc will use prof_prefix. In the
raw::write call in jemalloc.rs (the match on unsafe { raw::write(b"prof.dump\0",
...) }), replace the b"\0" pointer with std::ptr::null::<std::ffi::c_char>() (or
std::ptr::null::<libc::c_char>() if libc types are used) so the call becomes
raw::write(b"prof.dump\0", std::ptr::null::<std::ffi::c_char>()) inside the
existing unsafe block.
- Around line 14-16: The bare `return;` after the `tracing::warn!(%err, "failed
to activate jemalloc profiling");` is a type error because the enclosing
function returns `anyhow::Result<()>`; change that to return `Ok(())` so the
function returns the correct Result and keeps the runtime task inert on
activation failure (match the pattern used around the `raw::write(prof_active,
true)` handling and the behavior used on line 21). Ensure the early-exit path
returns `Ok(())` rather than `()`.
🪄 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: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 83ddf433-6e0f-4a17-8932-363dcb91cba1

📥 Commits

Reviewing files that changed from the base of the PR and between d82cd27 and d4027e0.

📒 Files selected for processing (4)
  • nix/modules/moq-relay.nix
  • rs/moq-relay/src/jemalloc.rs
  • rs/moq-relay/src/lib.rs
  • rs/moq-relay/src/main.rs
🚧 Files skipped from review as they are similar to previous changes (2)
  • rs/moq-relay/src/lib.rs
  • rs/moq-relay/src/main.rs

Comment thread nix/modules/moq-relay.nix
Comment on lines +146 to +149
heapDumpPrefix = lib.mkOption {
type = lib.types.str;
default = "/tmp/moq-relay.heap";
description = "Path prefix for jemalloc heap profile dumps (triggered via kill -USR1)";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🌐 Web query:

In systemd's systemd.exec(5), what does PrivateTmp=truedo to a service's/tmp, and are files written there visible at the host path /tmp/...?

💡 Result:

When PrivateTmp=true is set for a systemd service, systemd creates a new file system namespace for the service's processes and mounts private /tmp and /var/tmp directories inside it. These are bind-mounted over the service's view of /tmp and /var/tmp, using subdirectories of the host's /tmp and /var/tmp (typically named like /tmp/systemd-private--service-). From the service's perspective, /tmp behaves normally but is isolated. Files written by the service to its /tmp are not visible at the host's /tmp/... paths, nor shared with other services or the host unless they join the same namespace via JoinsNamespaceOf=.

Citations:


/tmp is the wrong default dump location with PrivateTmp enabled.

Line 148 sets the default heap dump prefix to /tmp/moq-relay.heap, but the unit enables PrivateTmp = true on line 206. With this setting, systemd creates an isolated file system namespace where the service's /tmp is private—files written there are not visible at the host's /tmp/... paths. The profile will not appear where the option description implies. Default this to cfg.stateDir instead, which is already allowlisted in ReadWritePaths on line 209, or disable PrivateTmp if host-visible dumps are required.

Suggested fix
-      default = "/tmp/moq-relay.heap";
+      default = "${cfg.stateDir}/moq-relay.heap";

Also applies to: 216-217

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@nix/modules/moq-relay.nix` around lines 146 - 149, The default heap dump
prefix uses /tmp which is private because PrivateTmp is enabled; update the
heapDumpPrefix default to use cfg.stateDir (e.g., join cfg.stateDir with
"moq-relay.heap") so dumps land in the service's allowed ReadWritePaths, and
make the same change for the other heap/profile default(s) referenced later (the
entries on lines ~216-217) instead of /tmp; alternatively, if you truly need
host-visible /tmp paths, disable PrivateTmp in the systemd service unit
(PrivateTmp = false).

Comment thread rs/moq-relay/src/jemalloc.rs Outdated
Comment on lines +30 to +33
// Empty path tells jemalloc to use prof_prefix from MALLOC_CONF.
match unsafe { raw::write(b"prof.dump\0", b"\0" as *const u8) } {
Ok(()) => tracing::info!("heap profile dumped"),
Err(err) => tracing::error!(%err, "failed to dump heap profile"),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🌐 Web query:

In jemalloc's mallctlAPI forprof.dump, does passing NULLuseprof_prefix, and does passing an empty string behave differently?

💡 Result:

Yes, passing NULL to the newp parameter of mallctl("prof.dump", NULL, NULL, NULL, 0) uses the prof_prefix to generate the filename according to the pattern <prefix><pid><seq>.m<mseq>.heap, where <prefix> comes from opt.prof_prefix or prof.prefix. No sources explicitly describe passing an empty string (i.e., mallctl("prof.dump", NULL, NULL, "", strlen(""))), but based on the man page description treating only NULL specially to use prof_prefix while non-NULL specifies a filename, an empty string would likely be treated as a filename "", which may fail to open or behave differently (e.g., error or empty file in current directory), not falling back to prof_prefix.

Citations:


prof.dump must be written with NULL to use prof_prefix.

The jemalloc manual specifies that prof.dump uses the <prefix> naming pattern only when passed NULL. This code passes a non-null pointer to an empty string instead, which jemalloc treats as a filename and does not trigger the prof_prefix fallback described in the comment. Use std::ptr::null::<std::ffi::c_char>() to pass the required NULL value.

Suggested fix
-		// Empty path tells jemalloc to use prof_prefix from MALLOC_CONF.
-		match unsafe { raw::write(b"prof.dump\0", b"\0" as *const u8) } {
+		// NULL tells jemalloc to use prof_prefix from MALLOC_CONF.
+		match unsafe { raw::write(b"prof.dump\0", std::ptr::null::<std::ffi::c_char>()) } {
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// Empty path tells jemalloc to use prof_prefix from MALLOC_CONF.
match unsafe { raw::write(b"prof.dump\0", b"\0" as *const u8) } {
Ok(()) => tracing::info!("heap profile dumped"),
Err(err) => tracing::error!(%err, "failed to dump heap profile"),
// NULL tells jemalloc to use prof_prefix from MALLOC_CONF.
match unsafe { raw::write(b"prof.dump\0", std::ptr::null::<std::ffi::c_char>()) } {
Ok(()) => tracing::info!("heap profile dumped"),
Err(err) => tracing::error!(%err, "failed to dump heap profile"),
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@rs/moq-relay/src/jemalloc.rs` around lines 30 - 33, The jemalloc prof.dump
call is incorrectly passing a non-null empty C-string (b"\0") which jemalloc
treats as a filename; change the second argument to a true NULL pointer so
jemalloc will use prof_prefix. In the raw::write call in jemalloc.rs (the match
on unsafe { raw::write(b"prof.dump\0", ...) }), replace the b"\0" pointer with
std::ptr::null::<std::ffi::c_char>() (or std::ptr::null::<libc::c_char>() if
libc types are used) so the call becomes raw::write(b"prof.dump\0",
std::ptr::null::<std::ffi::c_char>()) inside the existing unsafe block.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
rs/moq-relay/Cargo.toml (1)

50-51: Decouple unprefixed malloc symbol interposition from profiling feature.

Line 51 couples the profiling feature with unprefixed_malloc_on_supported_platforms, but these are orthogonal concerns. The unprefixed_malloc_on_supported_platforms feature controls whether jemalloc exports unprefixed symbols (malloc, free, etc.) that override libc's allocator globally, affecting all C/Objective-C code in the binary. Heap profiling via tikv-jemalloc-ctl (e.g., prof.dump) does not depend on this feature—only the profiling feature is required. Splitting this into a separate opt-in feature makes the runtime impact explicit and reduces coupling.

Suggested split
 [features]
 default = ["iroh", "quinn", "websocket"]
 iroh = ["moq-native/iroh"]
 jemalloc = ["dep:tikv-jemallocator", "dep:tikv-jemalloc-ctl"]
+jemalloc-unprefixed = ["tikv-jemallocator/unprefixed_malloc_on_supported_platforms"]
 noq = ["moq-native/noq"]
 quinn = ["moq-native/quinn"]
 quiche = ["moq-native/quiche"]
 websocket = ["moq-native/websocket", "dep:qmux", "axum/ws"]

@@
-tikv-jemallocator = { version = "0.6", features = ["profiling"], optional = true }
+tikv-jemallocator = { version = "0.6", features = ["profiling"], optional = true }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@rs/moq-relay/Cargo.toml` around lines 50 - 51, The dependency line couples
jemalloc's profiling and unprefixed-symbol behavior; update Cargo.toml so
tikv-jemallocator only uses the "profiling" feature for heap profiling and make
"unprefixed_malloc_on_supported_platforms" a separate optional feature flag (so
enabling tikv-jemalloc-ctl / profiling does not implicitly enable symbol
interposition). Concretely, change the tikv-jemallocator features to remove
"unprefixed_malloc_on_supported_platforms" and add a distinct optional feature
or feature-flag entry that consumers can opt into when they explicitly want
unprefixed malloc behavior; keep tikv-jemalloc-ctl as optional and leave its
profiling dependency pointing only to "profiling".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@rs/moq-relay/Cargo.toml`:
- Around line 50-51: The dependency line couples jemalloc's profiling and
unprefixed-symbol behavior; update Cargo.toml so tikv-jemallocator only uses the
"profiling" feature for heap profiling and make
"unprefixed_malloc_on_supported_platforms" a separate optional feature flag (so
enabling tikv-jemalloc-ctl / profiling does not implicitly enable symbol
interposition). Concretely, change the tikv-jemallocator features to remove
"unprefixed_malloc_on_supported_platforms" and add a distinct optional feature
or feature-flag entry that consumers can opt into when they explicitly want
unprefixed malloc behavior; keep tikv-jemalloc-ctl as optional and leave its
profiling dependency pointing only to "profiling".

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 5e51cd30-5950-4f8f-ba89-53717c5c5b16

📥 Commits

Reviewing files that changed from the base of the PR and between d4027e0 and d47d0c0.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (2)
  • rs/moq-relay/Cargo.toml
  • rs/moq-relay/src/jemalloc.rs
🚧 Files skipped from review as they are similar to previous changes (1)
  • rs/moq-relay/src/jemalloc.rs

kixelated and others added 3 commits April 2, 2026 15:46
jemalloc's configure script uses -O0 for test builds, which conflicts
with Nix's _FORTIFY_SOURCE hardening (requires optimization). Disable
fortify in both the dev shell and the moq-relay package build.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…td::error::Error

Replace anyhow::Context with map_err since tikv_jemalloc_ctl::Error
doesn't satisfy the StdError trait bound required by .context().

Also merge latest main to pick up recent changes.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@kixelated kixelated disabled auto-merge April 3, 2026 00:23
@kixelated kixelated enabled auto-merge (squash) April 3, 2026 00:23
@kixelated kixelated disabled auto-merge April 3, 2026 00:39
@kixelated kixelated merged commit 29758bc into main Apr 3, 2026
1 of 2 checks passed
@kixelated kixelated deleted the jemalloc-profiling branch April 3, 2026 00:40
This was referenced Apr 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant