Skip to content

refactor: decouple max proof task concurrency from inflight proof limits #19171

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 8 commits into from
Oct 22, 2025
Merged

refactor: decouple max proof task concurrency from inflight proof limits #19171

merged 8 commits into from
Oct 22, 2025

Conversation

@yongkangc
Copy link
Member

@yongkangc yongkangc commented Oct 21, 2025 •
edited
Loading

Closes: #19100

Problem:

  • Now that we have moved our workers to a pool based system see perf(tree): worker pooling for storage in multiproof generation #18887 we have also configured the workers independently.
  • But it's confusing that we have max proof task concurrency variable, which is now only used for inflight limit, and the naming doesn't reflect the actual nature of the use case.

Changes:

  1. Remove max proof task concurrency as we don't need it anymore in the code. It only serves as inflight proof limit.
  2. Introduced inflight limit. This would be removed in perf: rm pending queue from MultiproofManager #19178 but this pr aims to just remove cli Arg for now to keep it simple.

Benches:no regression while running

@github-project-automation github-project-automation bot moved this to Backlog in Reth Tracker Oct 21, 2025
@yongkangc yongkangc self-assigned this Oct 21, 2025
@yongkangc yongkangc changed the title refactor: remove max proof task concurrency refactor: decouple max proof task concurrency from inflight proof limits Oct 21, 2025
@yongkangc yongkangc marked this pull request as ready for review October 21, 2025 08:49
yongkangc
yongkangc commented Oct 21, 2025
View reviewed changes
crates/engine/tree/src/tree/payload_processor/multiproof.rs Outdated

/// Default upper bound for inflight multiproof calculations. These would be sitting in the queue
/// waiting to
pub const DEFAULT_MULTIPROOF_INFLIGHT_LIMIT: usize = 256;
Copy link
Member Author

@yongkangc yongkangc Oct 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

initially wanted this as a cli arg, but dont think it makes sense as its too hard to configure

Copy link
Collaborator

@mattsse mattsse Oct 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reasonable

@yongkangc yongkangc moved this from Backlog to In Review in Reth Tracker Oct 21, 2025
@yongkangc yongkangc requested a review from shekhirin October 21, 2025 09:24
@yongkangc yongkangc mentioned this pull request Oct 21, 2025
Closed
@yongkangc yongkangc mentioned this pull request Oct 22, 2025
Merged
@yongkangc yongkangc added C-perf A change motivated by improving speed, memory usage or disk footprint S-needs-benchmark This set of changes needs performance benchmarking to double-check that they help labels Oct 22, 2025
@yongkangc yongkangc added this to the v1.9.0 milestone Oct 22, 2025
@yongkangc yongkangc closed this Oct 22, 2025
@github-project-automation github-project-automation bot moved this from In Review to Done in Reth Tracker Oct 22, 2025
@yongkangc yongkangc reopened this Oct 22, 2025
@github-project-automation github-project-automation bot moved this from Done to In Progress in Reth Tracker Oct 22, 2025
mattsse
mattsse approved these changes Oct 22, 2025
View reviewed changes
Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense, especially with #19183 coming up

@yongkangc yongkangc added this pull request to the merge queue Oct 22, 2025
Merged via the queue into main with commit 60e3ede Oct 22, 2025
84 of 85 checks passed
@yongkangc yongkangc deleted the yk/fix-maxcc branch October 22, 2025 07:11
@github-project-automation github-project-automation bot moved this from In Progress to Done in Reth Tracker Oct 22, 2025
@jenpaff jenpaff moved this from Done to Completed in Reth Tracker Oct 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mattsse mattsse mattsse approved these changes

@Rjected Rjected Awaiting requested review from Rjected Rjected is a code owner

@klkvr klkvr Awaiting requested review from klkvr klkvr is a code owner

@rkrasiuk rkrasiuk Awaiting requested review from rkrasiuk

@fgimenez fgimenez Awaiting requested review from fgimenez fgimenez is a code owner

@mediocregopher mediocregopher Awaiting requested review from mediocregopher mediocregopher is a code owner

@gakonst gakonst Awaiting requested review from gakonst gakonst is a code owner

@shekhirin shekhirin Awaiting requested review from shekhirin

Assignees

@yongkangc yongkangc

Labels

C-perf A change motivated by improving speed, memory usage or disk footprint S-needs-benchmark This set of changes needs performance benchmarking to double-check that they help

Projects

Reth Tracker
Status: Completed

Milestone

v1.9.0

Development

Successfully merging this pull request may close these issues.

Refactor: Decouple logic for max concurrency with worker and in flight request

3 participants

@yongkangc @mattsse