opt: pre-allocate chunk blocks slice in chunk proposer #1572
Conversation
WalkthroughThe changes in the pull request focus on the Changes
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
rollup/internal/controller/watcher/chunk_proposer.go (1)
Line range hint
298-341: Consider improving error handling for metrics calculation.In the loop where metrics are calculated for each block, if
calcErroccurs, we return immediately. Consider recording this failure in metrics before returning, as it would help in monitoring and debugging these calculation failures.metrics, calcErr := utils.CalculateChunkMetrics(&chunk, codecVersion) if calcErr != nil { + p.proposeChunkFailureTotal.Inc() return fmt.Errorf("failed to calculate chunk metrics: %w", calcErr) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
rollup/internal/controller/watcher/chunk_proposer.go(1 hunks)
🔇 Additional comments (1)
rollup/internal/controller/watcher/chunk_proposer.go (1)
298-298: LGTM! Pre-allocation optimization looks good.
The pre-allocation of chunk.Blocks with the exact capacity is a good optimization that will reduce memory allocations and copying during chunk processing.
Let's verify the performance impact of this change:
There was a problem hiding this comment.
lgtm. could you also add the patch version here? https://github.com/scroll-tech/scroll/blob/develop/common/version/version.go#L8
btw, chunk.Blocks array may be truncated (not fully used) in https://github.com/scroll-tech/scroll/pull/1572/files#diff-8d7032b59131e8543188def29571bf6769f1fd460cc04519c541cecdab5ef5f7R337, do you think pre-allocate will have downgraded performance related to it?
e.g. when there are many blocks in db, it will fetch 100 blocks each round, thus pre-allocating 100 blocks' memory, while a chunk usually contains much less than 100 blocks.
@colinlyguo Good catch! In this case, pre-allocation still provides a net benefit:
|
yes. you're right. it's "not fully used the pre-allocated memory" instead of "truncated". As for point 3 I still have some doubts, would it be used again in next round? IMO the previous round's memory will be garbage collected. |
Let me clarify: The pre-allocated memory from a previous round will indeed be subject to garbage collection once the chunk variable goes out of scope. Each new round will allocate fresh memory. However, the pre-allocation is still beneficial because within each round: It eliminates multiple grow-and-copy operations during the append phase |
Purpose or design rationale of this PR
This PR improves performance by pre-allocating the capacity of chunk blocks slice based on the actual number of blocks to be processed.
What: Pre-allocate slice capacity in chunk proposer
Why: To reduce memory allocations and copying during chunk processing
How: Use len(blocks) as the initial capacity when creating the chunk.Blocks slice
PR title
perf: pre-allocate chunk blocks slice in chunk proposer
Deployment tag versioning
Breaking change label
Summary by CodeRabbit
New Features
Bug Fixes
Chores