chore(deploy): switch Docker and EC2 defaults to maxperf#103
Closed
chore(deploy): switch Docker and EC2 defaults to maxperf#103
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.
Follow-up to the profile gradient PR (#102). Flip the production-binary defaults from `profiling` to `maxperf` to capture the full 2-5% runtime gain from fat LTO + single codegen unit on prod nodes. - Dockerfile: `BUILD_PROFILE=maxperf` (was `profiling`) - MakefileEc2.mk: `PROFILE ?= maxperf` (was `profiling`) Tradeoff: symbols are stripped, so flame graphs and symbolicated backtraces require a one-off `--build-arg BUILD_PROFILE=profiling` (or `PROFILE=profiling make ...`) rebuild. Given the L2 execution client is throughput-sensitive and prod incidents are rare, the default is tilted toward runtime performance. Docker image builds will also take ~2-3x longer at the link stage — a one-time cost per image tag. Depends on #102 (introduces the `maxperf` profile definition).
📝 WalkthroughWalkthroughThis PR introduces customizable Cargo build profiles and updates the build system to select between them. It adds 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 @.github/workflows/release.yml:
- Around line 15-22: The profile input currently defaults to "profiling" and can
produce artifacts that clash with the canonical maxperf release filename; update
the release artifact naming logic so that when the input profile (the "profile"
choice) is not "maxperf" the profile is embedded into the tarball name (e.g.,
morph-reth-${VERSION}-${{ inputs.profile }}-${{ matrix.arch }}-linux.tar.gz),
and/or alternatively enforce profile="maxperf" when workflow_dispatch is used
with dry_run=false by adding a conditional that overrides the "profile" input
before build; locate references to the "profile" input and the tarball filename
string in the build/upload and draft-release job steps (where the current
morph-reth-${VERSION}-<arch>-linux.tar.gz pattern is formed) and apply the
conditional naming or override accordingly.
🪄 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: 84c011a5-78b7-495a-a5af-9bb0884020f9
📒 Files selected for processing (5)
.github/workflows/release.ymlCargo.tomlDockerfileMakefileEc2.mkcrates/evm/src/block/curie.rs
Comment on lines
+15
to
+22
| profile: | ||
| description: "Cargo profile for release binaries" | ||
| type: choice | ||
| default: "profiling" | ||
| options: | ||
| - profiling | ||
| - maxperf | ||
| - release |
There was a problem hiding this comment.
Consider whether release artifacts built from a non-maxperf profile should be distinguishable.
When triggered via workflow_dispatch, a user can pick profiling (current default) or release, and the resulting tarball is uploaded with the same filename pattern (morph-reth-${VERSION}-<arch>-linux.tar.gz) as the canonical tag-push maxperf artifact. If that build ever reaches the draft-release job (i.e., dry_run=false), it would be published under the same download URL that users expect to be the maxperf production binary — with measurably different runtime characteristics and, for profiling, unstripped symbols.
Two low-effort mitigations to consider:
- Embed the profile into the archive name when it's not
maxperf(e.g.,morph-reth-${VERSION}-${profile}-x86_64-linux.tar.gz), or - Force
profile=maxperfwheneverdry_run=falseonworkflow_dispatch(i.e., only allow non-default profiles for dry runs).
Not a blocker for this PR's scope, but worth thinking through before the next manual release.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/release.yml around lines 15 - 22, The profile input
currently defaults to "profiling" and can produce artifacts that clash with the
canonical maxperf release filename; update the release artifact naming logic so
that when the input profile (the "profile" choice) is not "maxperf" the profile
is embedded into the tarball name (e.g., morph-reth-${VERSION}-${{
inputs.profile }}-${{ matrix.arch }}-linux.tar.gz), and/or alternatively enforce
profile="maxperf" when workflow_dispatch is used with dry_run=false by adding a
conditional that overrides the "profile" input before build; locate references
to the "profile" input and the tarball filename string in the build/upload and
draft-release job steps (where the current
morph-reth-${VERSION}-<arch>-linux.tar.gz pattern is formed) and apply the
conditional naming or override accordingly.
Contributor
Author
|
Superseded — squashing into a single PR per reviewer preference. |
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
Follow-up to #102. Flip the production-deploy defaults from
profiling→maxperfso prod nodes capture the full 2–5% runtime gain from fat LTO + single codegen unit.BUILD_PROFILE=maxperf(wasprofiling)PROFILE ?= maxperf(wasprofiling)Kept consistent: both prod artifacts still share the same profile, so S3-deployed EC2 binaries match what the Docker image ships.
Tradeoff
profiling(previous default in #102)maxperf(this PR)profilingon-demand)Given morph-reth is a throughput-bound L2 execution client and prod incidents are rare, the default tilts toward runtime performance. When a prod incident actually needs symbols, rebuild on-demand:
docker build --build-arg BUILD_PROFILE=profiling ...PROFILE=profiling make -f MakefileEc2.mk build-bk-prod-morph-prod-mainnet-to-morph-rethStacking
[profile.maxperf]definition inCargo.toml. Merge chore: port build profile gradient from tempo #102 first, then this.Test plan
Summary by CodeRabbit