chore: port build profile gradient from tempo#102
Closed
Conversation
Clippy 1.95 reports `storage.sort_by(|(a, _), (b, _)| a.cmp(b))` as a `clippy::unnecessary_sort_by` (prefer `sort_by_key`) in the `apply_curie_hard_fork` test. The fix is the clippy-suggested rewrite. No behavioural change; only the test's sort comparator is reshaped.
morph-reth's root Cargo.toml previously defined no [profile.*] overrides,
so release builds fell back to Cargo defaults (lto=off). Adopt the same
profile shape used by the sibling `tempo` project:
- profile.dev: line-tables-only debug, unpacked split-debuginfo.
- profile.hivetests: opt-level=3 + lto=thin, inherits test (debug_assert on).
- profile.release: opt-level=3, lto=thin, strip=symbols, codegen-units=16.
- profile.profiling: release + line-table debug + strip=none + incremental
— production binaries that still flamegraph cleanly.
- profile.bench: inherits profiling.
- profile.maxperf: release + lto=fat + codegen-units=1 for peak throughput
(expect a noticeably longer link stage).
Match the tempo project's convention: production container images default to the `profiling` profile, which is release-grade (opt-level=3, lto=thin, codegen-units=16) but keeps line-table debug symbols and skips stripping. That preserves flame graphs and backtraces in prod at near-zero runtime cost. Override with `--build-arg BUILD_PROFILE=maxperf` for peak throughput (fat LTO + codegen-units=1, much slower link) or `BUILD_PROFILE=release` for the stripped/symbolless variant.
Previously the release workflow hardcoded `target/<target>/release/` when packaging tarballs, which coupled it to the Makefile's `release` default and silently broke if either was overridden. Changes: - Add a `profile` workflow_dispatch input (default `profiling`, with `maxperf` and `release` as alternatives). Push-tag triggers keep defaulting to `maxperf` for peak throughput on public releases. - Resolve the profile in a dedicated step and thread it through both `make build-<target>` (via PROFILE=) and the subsequent `cp` path (using the computed `profile_dir`, which handles Cargo's dev→debug naming quirk). - Update `MakefileEc2.mk` default PROFILE to `profiling` so S3-deployed EC2 binaries match the Dockerfile default — shipped prod artifacts keep line-table symbols for flame graphs.
📝 WalkthroughWalkthroughThis PR introduces multiple customizable Cargo build profiles ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Cargo.toml`:
- Around line 46-52: The comment above the [profile.dev] section is stale—update
the text to explain that this active profile reduces debug info and that
developers who need full debugger support should override it locally (for
example by removing or changing debug = "line-tables-only" or setting
split-debuginfo) rather than “uncommenting”; modify the comment near the
[profile.dev] header in Cargo.toml to clearly state it is an active local
override that can be changed for full debugger fidelity.
🪄 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: 7c93792a-bdb4-4de5-9356-7f14ffefaef2
📒 Files selected for processing (5)
.github/workflows/release.ymlCargo.tomlDockerfileMakefileEc2.mkcrates/evm/src/block/curie.rs
Comment on lines
+46
to
+52
| # Speed up compilation time for dev builds by reducing emitted debug info. | ||
| # NOTE: Debuggers may provide less useful information with this setting. | ||
| # Uncomment this section if you're using a debugger. | ||
| [profile.dev] | ||
| # https://davidlattimore.github.io/posts/2024/02/04/speeding-up-the-rust-edit-build-run-cycle.html | ||
| debug = "line-tables-only" | ||
| split-debuginfo = "unpacked" |
There was a problem hiding this comment.
Update the stale debugger comment.
Line 48 says to “Uncomment this section”, but [profile.dev] is already active. Reword it to describe the local override needed for full debugger support.
Suggested wording
# Speed up compilation time for dev builds by reducing emitted debug info.
# NOTE: Debuggers may provide less useful information with this setting.
-# Uncomment this section if you're using a debugger.
+# Use `debug = "full"` locally if you need full variable-level debugger support.
[profile.dev]📝 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
| # Speed up compilation time for dev builds by reducing emitted debug info. | |
| # NOTE: Debuggers may provide less useful information with this setting. | |
| # Uncomment this section if you're using a debugger. | |
| [profile.dev] | |
| # https://davidlattimore.github.io/posts/2024/02/04/speeding-up-the-rust-edit-build-run-cycle.html | |
| debug = "line-tables-only" | |
| split-debuginfo = "unpacked" | |
| # Speed up compilation time for dev builds by reducing emitted debug info. | |
| # NOTE: Debuggers may provide less useful information with this setting. | |
| # Use `debug = "full"` locally if you need full variable-level debugger support. | |
| [profile.dev] | |
| # https://davidlattimore.github.io/posts/2024/02/04/speeding-up-the-rust-edit-build-run-cycle.html | |
| debug = "line-tables-only" | |
| split-debuginfo = "unpacked" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Cargo.toml` around lines 46 - 52, The comment above the [profile.dev] section
is stale—update the text to explain that this active profile reduces debug info
and that developers who need full debugger support should override it locally
(for example by removing or changing debug = "line-tables-only" or setting
split-debuginfo) rather than “uncommenting”; modify the comment near the
[profile.dev] header in Cargo.toml to clearly state it is an active local
override that can be changed for full debugger fidelity.
3 tasks
Contributor
Author
|
Superseded — see forthcoming replacement PR that bundles the Docker/EC2 maxperf switch directly into the profile port. |
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
1 participant
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
morph-reth's root
Cargo.tomlpreviously defined no[profile.*]overrides, so release builds fell back to Cargo defaults (lto = off) — worse than even upstream reth's ownrelease. This PR mirrors the profile shape used by the siblingtempoproject and aligns the Dockerfile, release workflow, and EC2 deploy Makefile so that every shipped artifact uses an intentional profile.Commit-by-commit:
chore(evm): silence clippy unnecessary_sort_by in curie.rs test— pre-existing clippy 1.95 error onmainthat blocks the CI lint job. Cherry-picked from the dev branch. Independent fix, kept first so the rest of the PR can pass lint.chore(cargo): port build profile gradient from tempo— addsdev/hivetests/release/profiling/bench/maxperfwith the exact settings tempo uses. No changes to[workspace.lints](already matches tempo).chore(docker): default BUILD_PROFILE to profiling— production Docker images now default toprofiling(release + line-table symbols) so they remain flame-graphable. Override with--build-arg BUILD_PROFILE=maxperforrelease.ci(release): parameterize cargo profile; default tag-push to maxperf— release workflow no longer hardcodestarget/.../release/. Push-tag triggers usemaxperffor peak throughput;workflow_dispatchexposes aprofileinput (defaultprofiling).MakefileEc2.mkdefault also flips toprofilingto match the Dockerfile, so S3-deployed EC2 binaries stay flame-graphable in prod.Profile choice matrix (after this PR)
make build(local dev)release(unchanged)ghcr.io/.../morph-reth)profilingMakefileEc2.mk)profilingmaxperfprofiling(user-selectable)Test plan
cargo metadataparses with new profile sectioncargo fmt --all -- --checkpassescargo clippy --all --all-targets -- -D warningspasses (curie fix lands first)cargo check --profile profiling --bin morph-rethbuilds cleanlylint/test/buildworkflows green on the PRworkflow_dispatchwith defaultprofile=profilingproduces a valid tarballmaxperfdispatch run once ready to cut a tagNon-goals / deliberately skipped
[workspace.lints]already matches tempo; no changes.PROFILE ?= releaseis unchanged. Dailymake build/make test/make lintbehave identically to today. Only the release pipeline and EC2 deploy override the default.paradigmxyz/reth's clippy nursery (too aggressive for this codebase; ~120 findings without much benefit).Summary by CodeRabbit
releasetoprofilingfor improved debuggability in production environments while maintaining performance.