chore: memory profiling Forest#5837
Merged
Merged
Conversation
Contributor
WalkthroughAdds a profiling Cargo profile and Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer
participant Make as Makefile
participant Cargo as cargo (profile=profiling)
participant Build as build.rs
participant Bin as target/profiling/forest
participant OS as OS
Dev->>Make: make gperfheapprofile.forest
Make->>Cargo: FOREST_PROFILING_GPERFTOOLS_BUILD=1 build --no-default-features --features system-alloc --profile=profiling --bin forest
Cargo->>Build: run build script (env)
Note right of Build: Detects FOREST_PROFILING_GPERFTOOLS_BUILD truthy → link tcmalloc
Build-->>Cargo: build metadata (link tcmalloc)
Cargo-->>Make: build artifacts (target/profiling/forest)
Make->>OS: ulimit -n 8192
Make->>Bin: HEAPPROFILE_USE_PID=t HEAPPROFILE=/tmp/gperfheap.forest.prof (run)
Bin-->>Make: generates heap profile file
sequenceDiagram
autonumber
participant Dev as Developer
participant Make as Makefile
participant Cargo as cargo (profile=profiling)
participant Build as build.rs
participant Bin as target/profiling/forest
participant Heap as heaptrack
Dev->>Make: make memprofile-heaptrack.forest
Make->>Cargo: build --no-default-features --features system-alloc --profile=profiling --bin forest
Cargo->>Build: run build script
Note right of Build: Normal build (no tcmalloc link) — allocator may be System via feature
Cargo-->>Make: build artifacts
Make->>Heap: heaptrack -o /tmp/heaptrack.forest.%p.zst target/profiling/forest ...
Heap->>Bin: launch and trace
Heap-->>Make: writes compressed heap profile
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Assessment against linked issues
Out-of-scope changes
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. ⛔ Files ignored due to path filters (6)
📒 Files selected for processing (6)
💤 Files with no reviewable changes (2)
🚧 Files skipped from review as they are similar to previous changes (4)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
LesnyRumcajs
force-pushed
the
memory-profile
branch
2 times, most recently
from
44ed8e6 to
ca45b66
Compare
LesnyRumcajs
force-pushed
the
memory-profile
branch
from
eb2c4fa to
9a42663
Compare
LesnyRumcajs
force-pushed
the
memory-profile
branch
2 times, most recently
from
64ffac5 to
75e6e7c
Compare
LesnyRumcajs
requested review from
elmattic and
hanabi1224
and removed request for
a team
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
src/lib.rs (1)
33-36: Make allocator features mutually exclusive to avoid accidental dual selectionAdding
system-alloclooks good. However, right now nothing prevents users from enablingjemallocandsystem-alloc(orrustalloc) together, in which case the first matchingcfg_if!branch silently wins. This can be confusing during profiling and lead to false conclusions.Recommend compile-time guards to enforce mutual exclusivity.
Apply these additions (outside this range) near the top of the file before the
cfg_if!:// Allocator feature sanity checks #[cfg(all(feature = "jemalloc", feature = "system-alloc"))] compile_error!("Features 'jemalloc' and 'system-alloc' are mutually exclusive; enable only one."); #[cfg(all(feature = "rustalloc", any(feature = "jemalloc", feature = "system-alloc")))] compile_error!("Feature 'rustalloc' cannot be combined with 'jemalloc' or 'system-alloc'.");Cargo.toml (1)
301-301: Document allocator feature intentConsider adding a short comment indicating
system-allocis intended for profiling workflows (with gperftools/heaptrack) and should not be combined withjemallocorrustalloc. This complements the compile-time checks suggested in lib.rs.[features] default = ["jemalloc", "tokio-console", "tracing-loki", "tracing-chrome"] slim = ["rustalloc"] -system-alloc = [] +system-alloc = [] # Use the platform allocator (for memory profiling). Do not combine with 'jemalloc' or 'rustalloc'.build.rs (2)
7-10: Gate tcmalloc linking by OS to avoid surprising failures on non-Linux hostsLinking
tcmallocis primarily useful/available on Linux; on macOS or others this can fail even when users set the env var. Add a small OS gate + warning to make failures more predictable.- if is_env_truthy("FOREST_PROFILING_GPERFTOOLS_BUILD") { - println!("cargo:rustc-link-lib=tcmalloc"); - } + if is_env_truthy("FOREST_PROFILING_GPERFTOOLS_BUILD") { + // tcmalloc is mainly supported on Linux; emit a warning elsewhere. + #[cfg(target_os = "linux")] + { + println!("cargo:rustc-link-lib=tcmalloc"); + } + #[cfg(not(target_os = "linux"))] + { + println!("cargo:warning=FOREST_PROFILING_GPERFTOOLS_BUILD=1 set, \ + but tcmalloc linking is only attempted on Linux."); + } + }
49-54: Tighten truthiness parsing and avoid allocationCurrent implementation lowercases the whole string and accepts an unusual
_yes_token. Suggest trimming whitespace, supporting common tokens, and usingeq_ignore_ascii_caseto avoid allocating.-fn is_env_truthy(env: &str) -> bool { - std::env::var(env) - .ok() - .map(|var| matches!(var.to_lowercase().as_str(), "1" | "true" | "yes" | "_yes_")) - .unwrap_or_default() -} +fn is_env_truthy(env: &str) -> bool { + std::env::var(env).map_or(false, |v| { + let v = v.trim(); + v == "1" + || v.eq_ignore_ascii_case("true") + || v.eq_ignore_ascii_case("yes") + || v.eq_ignore_ascii_case("on") + }) +}If
_yes_is intentional for some pipeline quirk, consider documenting it or keeping it as an extra match with a short comment.Makefile (2)
133-139: gperftools heap profiling flow looks correct; minor env nitThe build + run sequence with
--features system-allocandFOREST_PROFILING_GPERFTOOLS_BUILD=1is spot-on. Tiny nit:HEAPPROFILE_USE_PIDis typically set to1in docs/examples—tlikely works but1is clearer.- HEAPPROFILE_USE_PID=t HEAPPROFILE=/tmp/gperfheap.$(1).prof target/profiling/$(1) $(2) + HEAPPROFILE_USE_PID=1 HEAPPROFILE=/tmp/gperfheap.$(1).prof target/profiling/$(1) $(2)Also consider supporting an override for the output path via a make var like
GPROF_OUT ?= /tmpto avoid hardcoding/tmpin containerized runs.
147-153: Heaptrack target LGTM; add existence checks and guidanceThis target is practical. To improve UX, add quick checks that
heaptrackexists and print a hint otherwise.-memprofile-heaptrack = cargo build --no-default-features --features system-alloc --profile=profiling --bin $(1); \ - ulimit -n 8192; \ - heaptrack -o /tmp/heaptrack.$(1).%p.zst target/profiling/$(1) $(2) +memprofile-heaptrack = command -v heaptrack >/dev/null 2>&1 || { echo "heaptrack not found. See https://github.com/KDE/heaptrack"; exit 127; }; \ + cargo build --no-default-features --features system-alloc --profile=profiling --bin $(1); \ + ulimit -n 8192; \ + heaptrack -o /tmp/heaptrack.$(1).%p.zst target/profiling/$(1) $(2)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (6)
documentation/src/developer_documentation/heaptrack/bottom_up.pngis excluded by!**/*.pngdocumentation/src/developer_documentation/heaptrack/caller_callee.pngis excluded by!**/*.pngdocumentation/src/developer_documentation/heaptrack/consumed.pngis excluded by!**/*.pngdocumentation/src/developer_documentation/heaptrack/flamegraph.pngis excluded by!**/*.pngdocumentation/src/developer_documentation/heaptrack/sizes.pngis excluded by!**/*.pngdocumentation/src/developer_documentation/heaptrack/summary.pngis excluded by!**/*.png
📒 Files selected for processing (6)
Cargo.toml(2 hunks)Makefile(1 hunks)build.rs(2 hunks)documentation/src/SUMMARY.md(0 hunks)documentation/src/developer_documentation/memory-analysis.md(0 hunks)src/lib.rs(1 hunks)
💤 Files with no reviewable changes (2)
- documentation/src/SUMMARY.md
- documentation/src/developer_documentation/memory-analysis.md
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: LesnyRumcajs
PR: ChainSafe/forest#5907
File: src/rpc/methods/state.rs:523-570
Timestamp: 2025-08-06T15:44:33.467Z
Learning: LesnyRumcajs prefers to rely on BufWriter's Drop implementation for automatic flushing rather than explicit flush() calls in Forest codebase.
📚 Learning: 2025-08-07T13:39:15.107Z
Learnt from: hanabi1224
PR: ChainSafe/forest#5886
File: interop-tests/src/tests/go_app/gen.go:29-29
Timestamp: 2025-08-07T13:39:15.107Z
Learning: Auto-generated files like those created by rust2go (indicated by "Generated by rust2go. Please DO NOT edit this C part manually." comment) in the Forest project should be skipped during code review as they are not intended for manual editing.
Applied to files:
build.rs
📚 Learning: 2025-08-08T12:10:45.218Z
Learnt from: hanabi1224
PR: ChainSafe/forest#5867
File: src/ipld/util.rs:553-558
Timestamp: 2025-08-08T12:10:45.218Z
Learning: Forest project targets Rust stable >=1.89; features stabilized in 1.88 like let-chains are acceptable in this codebase.
Applied to files:
build.rs
📚 Learning: 2025-08-04T13:36:22.993Z
Learnt from: hanabi1224
PR: ChainSafe/forest#5835
File: src/chain/tests.rs:58-76
Timestamp: 2025-08-04T13:36:22.993Z
Learning: In the Forest codebase, `Vec<u8>` can be used as an `AsyncWrite` implementation in test contexts. The user confirmed that tests using `&mut Vec<u8>` with `export` and `export_v2` functions compile and pass both locally and on CI.
Applied to files:
build.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: tests-release
- GitHub Check: tests
- GitHub Check: Build forest binaries on Linux AMD64
- GitHub Check: Bootstrap checks - Lotus
- GitHub Check: Snapshot unordered export checks
- GitHub Check: Calibnet no discovery checks
- GitHub Check: Snapshot export checks v2 with F3 data
- GitHub Check: Snapshot export checks
- GitHub Check: Wallet tests
- GitHub Check: Devnet checks
- GitHub Check: Calibnet check
- GitHub Check: Forest CLI checks
- GitHub Check: Analyze (rust)
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
Cargo.toml (1)
279-285: Profiling profile LGTM; aligns with memory tooling needsThe dedicated
[profile.profiling]with debug info andpanic = "abort"is appropriate for heap profilers. No issues spotted.build.rs (1)
17-39: Env opt-out semantics for f3-sidecar: confirm docs and CI expectationsSwitching to
!is_env_truthy("FOREST_F3_SIDECAR_FFI_BUILD_OPT_OUT")changes the default to “build sidecar unless explicitly opted out.” That’s fine, but please ensure:
- The new variable is documented in CONTRIBUTING/docs.
- CI and release pipelines do not set legacy envs expecting the old behavior.
I can scan the repo/CI for references to
FOREST_F3_SIDECAR_FFI_BUILD_OPT_OUTvs any older sidecar env and report mismatches if helpful.Makefile (1)
120-126: Nice: clear, discoverable profiling sectionThe section header and link to reference material are appreciated. This will save time for future investigations.
hanabi1224
previously approved these changes
Aug 21, 2025
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Cargo.toml (1)
279-285: Profiling profile is a good start; consider codegen-units=1 and a small opt-level bump for more realistic allocation behavior.
- For memory investigations that aim to resemble production allocation patterns (while keeping good symbols),
opt-level=1(or2) often strikes a better balance than0.opt-level=0can materially change inlining and allocation sites. Optional tweak below.- Adding
codegen-units = 1helps produce tighter, more stable call stacks for profilers and can modestly reduce noise across runs.- Minor:
split-debuginfo = "unpacked"is already inherited from[profile.dev]; it’s harmless to keep, but redundant.Apply if desired:
[profile.profiling] inherits = "dev" -opt-level = 0 +opt-level = 1 debug = true split-debuginfo = "unpacked" panic = "abort" +codegen-units = 1If you prefer keeping
opt-level = 0, you can still add onlycodegen-units = 1.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
Cargo.toml(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: LesnyRumcajs
PR: ChainSafe/forest#5907
File: src/rpc/methods/state.rs:523-570
Timestamp: 2025-08-06T15:44:33.467Z
Learning: LesnyRumcajs prefers to rely on BufWriter's Drop implementation for automatic flushing rather than explicit flush() calls in Forest codebase.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: All lint checks
- GitHub Check: cargo-publish-dry-run
- GitHub Check: Build MacOS
- GitHub Check: Build Ubuntu
- GitHub Check: Build forest binaries on Linux AMD64
- GitHub Check: tests
- GitHub Check: tests-release
- GitHub Check: Analyze (rust)
- GitHub Check: Analyze (go)
hanabi1224
approved these changes
Aug 22, 2025
elmattic
approved these changes
Aug 22, 2025
LesnyRumcajs
force-pushed
the
memory-profile
branch
from
f5326a6 to
52799b7
Compare
4 tasks
6 tasks
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Summary of changes
Changes introduced in this pull request:
Reference issue to close (if applicable)
Closes #5879
Other information and links
Change checklist
Summary by CodeRabbit
New Features
Documentation
Chores