opt: pre-allocate chunk blocks slice in chunk proposer by qcrao · Pull Request #1572 · scroll-tech/scroll · GitHub

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

perf: A code change that improves performance

Deployment tag versioning

No, this PR doesn't involve a new deployment, git tag, docker image tag

Breaking change label

No, this PR is not a breaking change

Summary by CodeRabbit

New Features
- Enhanced error handling for chunk proposals, including checks for block limits.
- New methods added for recording chunk metrics.
Bug Fixes
- Improved memory allocation for chunk blocks.
Chores
- Version updated from v4.4.81 to v4.4.83.

Caution

Review failed

The head commit changed during the review from 9604d86 to 722463b.

Walkthrough

The changes in the pull request focus on the ChunkProposer class within the rollup/internal/controller/watcher/chunk_proposer.go file. Key modifications include the initialization of the chunk.Blocks slice for optimized memory allocation and enhanced error handling in the proposeChunk method. A new check for the first block's compliance with defined limits is introduced, improving the method's robustness. Additionally, two new methods for recording chunk metrics are added, and existing methods are updated to ensure better logging and database updates related to chunk processing. The version number in common/version/version.go is updated from "v4.4.81" to "v4.4.83".

Changes

File Path	Change Summary
rollup/internal/controller/watcher/chunk_proposer.go	- Updated proposeChunk to initialize chunk.Blocks with capacity based on blocks length. - Added error handling for first block limit checks. - Updated updateDBChunkInfo for chunk metrics recalculation and logging. - Added methods: recordAllChunkMetrics and recordTimerChunkMetrics for chunk metrics recording. - Modified method signature of NewChunkProposer without parameter changes.
common/version/version.go	- Updated version tag from "v4.4.81" to "v4.4.83".

Suggested labels

bump-version

Suggested reviewers

georgehao
colinlyguo
roylou

🐇 In a world where chunks do play,
A proposer hops, brightening the day.
With limits checked and metrics in sight,
Errors are caught, making things right!
So let’s celebrate this code with cheer,
For a smoother rollup, we hold dear! 🌟

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?

❤️ Share

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>, please review it.
- Generate unit testing code for this file.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai generate unit testing code for this file.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
- @coderabbitai read src/utils.ts and generate unit testing code.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
- @coderabbitai help me debug CodeRabbit configuration file.

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)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai full review to do a full review from scratch and review all the files again.
@coderabbitai summary to regenerate the summary of the PR.
@coderabbitai generate docstrings to generate docstrings for this PR. (Experiment)
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai configuration to show the current CodeRabbit configuration for the repository.
@coderabbitai help to get help.

Other keywords and placeholders

Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
Please see the configuration documentation for more information.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

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 calcErr occurs, 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

📥 Commits

Reviewing files that changed from the base of the PR and between 06beb5d and 25f741f.

📒 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:

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.

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 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:

The truncation operation (slice[:n-1]) doesn't involve memory reallocation or data copying - it just adjusts the slice length
Even with occasional truncation, having pre-allocated capacity helps avoid multiple grow-and-copy operations during the append operations
The memory for the pre-allocated array will be reused in subsequent operations rather than being reallocated

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 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:

The truncation operation (slice[:n-1]) doesn't involve memory reallocation or data copying - it just adjusts the slice length

Even with occasional truncation, having pre-allocated capacity helps avoid multiple grow-and-copy operations during the append operations

The memory for the pre-allocated array will be reused in subsequent operations rather than being reallocated

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.

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 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:

The truncation operation (slice[:n-1]) doesn't involve memory reallocation or data copying - it just adjusts the slice length

Even with occasional truncation, having pre-allocated capacity helps avoid multiple grow-and-copy operations during the append operations

The memory for the pre-allocated array will be reused in subsequent operations rather than being reallocated

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
The "not fully used" space is just unused capacity, which has minimal overhead compared to the cost of dynamic resizing

Thegaram requested a review from colinlyguo December 11, 2024 14:46

coderabbitai Bot reviewed Dec 11, 2024

View reviewed changes

colinlyguo previously approved these changes Dec 11, 2024

View reviewed changes

qcrao dismissed colinlyguo’s stale review via c8a233c December 11, 2024 15:01

qcrao force-pushed the develop branch from 25f741f to c8a233c Compare December 11, 2024 15:01

qcrao requested a review from colinlyguo December 11, 2024 15:07

colinlyguo added the bump-version Bump the version tag for deployment label Dec 11, 2024

colinlyguo previously approved these changes Dec 11, 2024

View reviewed changes

georgehao previously approved these changes Dec 12, 2024

View reviewed changes

georgehao removed the bump-version Bump the version tag for deployment label Dec 12, 2024

qcrao dismissed stale reviews from georgehao and colinlyguo via a9bf9a4 December 12, 2024 01:58

qcrao force-pushed the develop branch from c8a233c to a9bf9a4 Compare December 12, 2024 01:58

georgehao added the bump-version Bump the version tag for deployment label Dec 12, 2024

opt: pre-allocate chunk blocks slice in chunk proposer

563f1b5

qcrao force-pushed the develop branch from a9bf9a4 to 563f1b5 Compare December 12, 2024 02:38

georgehao removed the bump-version Bump the version tag for deployment label Dec 12, 2024

Update version.go

419e29c

georgehao previously approved these changes Dec 12, 2024

View reviewed changes

Merge branch 'develop' into develop

e20f283

colinlyguo previously approved these changes Dec 12, 2024

View reviewed changes

Merge branch 'develop' into develop

9604d86

colinlyguo dismissed stale reviews from georgehao and themself via 9604d86 December 12, 2024 06:23

colinlyguo previously approved these changes Dec 12, 2024

View reviewed changes

colinlyguo reviewed Dec 12, 2024

View reviewed changes

Comment thread common/version/version.go Outdated

Update common/version/version.go

722463b

colinlyguo dismissed their stale review via 722463b December 12, 2024 06:24

colinlyguo approved these changes Dec 12, 2024

View reviewed changes

georgehao approved these changes Dec 12, 2024

View reviewed changes

colinlyguo merged commit 33b1b3c into scroll-tech:develop Dec 12, 2024

coderabbitai Bot mentioned this pull request Feb 26, 2025

fix(build): image building #1603

Merged

3 tasks

coderabbitai Bot mentioned this pull request Mar 7, 2025

feat: euclid prover and coordinator #1597

Merged

13 tasks

coderabbitai Bot mentioned this pull request Apr 21, 2025

feat(rollup-relayer): add a tool to analyze chunk/batch/bundle proposing #1645

Merged

3 tasks

coderabbitai Bot mentioned this pull request May 13, 2025

refactor(rollup-relayer): simplify logic post-Euclid #1658

Merged

3 tasks

coderabbitai Bot mentioned this pull request Sep 8, 2025

refactor(rollup relayer): remove max_block_num_per_chunk configuration parameter #1729

Merged

13 tasks

coderabbitai Bot mentioned this pull request Sep 18, 2025

feat(tx sender): add multiple write clients for more reliable tx submission #1740

Merged

13 tasks

coderabbitai Bot mentioned this pull request Oct 2, 2025

feat(l2 relayer): enhance batch submission strategy based on backlog size #1745

Merged

13 tasks

coderabbitai Bot mentioned this pull request Nov 25, 2025

feat: propose chunk at hardfork boundary #1767

Merged

5 tasks

coderabbitai Bot mentioned this pull request Feb 26, 2026

feat(coordinator): limit parallelism during chunk assignment #1797

Open

5 tasks

coderabbitai Bot mentioned this pull request Mar 9, 2026

fix: use empty access list if eth_createAccessList returns error #1799

Merged

5 tasks

opt: pre-allocate chunk blocks slice in chunk proposer by qcrao · Pull Request #1572 · scroll-tech/scroll · GitHub

Uh oh!

opt: pre-allocate chunk blocks slice in chunk proposer - #1572

opt: pre-allocate chunk blocks slice in chunk proposer#1572
colinlyguo merged 5 commits into
scroll-tech:developfrom
qcrao:develop

qcrao commented Dec 11, 2024 •
edited by coderabbitai Bot
Loading

Uh oh!

Uh oh!

coderabbitai Bot commented Dec 11, 2024 •
edited
Loading

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Uh oh!

colinlyguo left a comment •
edited
Loading

Uh oh!

Uh oh!

qcrao commented Dec 11, 2024

Uh oh!

colinlyguo commented Dec 11, 2024

Uh oh!

qcrao commented Dec 12, 2024

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

opt: pre-allocate chunk blocks slice in chunk proposer by qcrao · Pull Request #1572 · scroll-tech/scroll · GitHub

Uh oh!

opt: pre-allocate chunk blocks slice in chunk proposer - #1572

opt: pre-allocate chunk blocks slice in chunk proposer#1572colinlyguo merged 5 commits intoscroll-tech:developscroll-tech/scroll:developfrom qcrao:developqcrao/scroll:developCopy head branch name to clipboard

Conversation

qcrao commented Dec 11, 2024 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose or design rationale of this PR

PR title

Deployment tag versioning

Breaking change label

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Dec 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review failed

Walkthrough

Changes

Suggested labels

Suggested reviewers

Chat

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

CodeRabbit Configuration File (.coderabbit.yaml)

Documentation and Community

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

colinlyguo left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qcrao commented Dec 11, 2024

Uh oh!

colinlyguo commented Dec 11, 2024

Uh oh!

qcrao commented Dec 12, 2024

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

opt: pre-allocate chunk blocks slice in chunk proposer#1572
colinlyguo merged 5 commits into
scroll-tech:developfrom
qcrao:develop

qcrao commented Dec 11, 2024 •
edited by coderabbitai Bot
Loading

coderabbitai Bot commented Dec 11, 2024 •
edited
Loading

colinlyguo left a comment •
edited
Loading