Skip to content

Introduce ThreadBlockBuildingContext, remove old CachedReads. #544

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.

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

Merged
merged 1 commit into from
Apr 9, 2025

Conversation

dvush
Copy link
Contributor

@dvush dvush commented Apr 8, 2025

The idea behind ThreadBlockBuildingContext is to have a place that is owned by a particular thread (for example, max profit ordering builder) that can be used as a scratchpad for caching without doing global caching shared between all threads.

Currently we have only one place for caches BlockBuildingContext. It is global and shared between all builders, top of block simulation, and finalization thread. This approach solves many problems with caching. For example, we use it for eth-sparse-mpt root hash caching.

But sometimes we would like to have a cache that is local to the current thread to avoid the overhead of mutex and multiple threads.

I've seen the need for caches like this multiple times. This commit adds support for local caches like this.

The first thing that is moved to this new setup of 2 caches is cached reads. We had a lot of trouble with passing CachedReads struct around and it introduced itself into traits where it does not belong such as BlockBuildingHelper or backtesting code. Here we move CachedReads to this uniform setup and it simplifies cached reads state handling.

📝 Summary

💡 Motivation and Context


✅ I have completed the following steps:

  • Run make lint
  • Run make test
  • Added tests (if applicable)

@Copilot Copilot AI review requested due to automatic review settings April 8, 2025 14:58
@dvush dvush requested review from ZanCorDX and ferranbt as code owners April 8, 2025 14:58
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 26 out of 26 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (2)

crates/rbuilder/src/building/builders/ordering_builder.rs:166

  • [nitpick] Consider standardizing the placement and naming of the 'local_ctx' parameter across functions (e.g., always placing it immediately after the BlockBuildingContext) and adding a brief inline comment describing its purpose to improve code consistency and readability.
let mut local_ctx = ThreadBlockBuildingContext::default();

crates/rbuilder/src/building/order_commit.rs:296

  • [nitpick] Consider adding inline documentation for the new lifetime parameters ('c' and 'd') in PartialBlockFork so that their purpose and relation to the thread-local context are clearer for future maintainers.
pub struct PartialBlockFork<'a, 'b, 'c, 'd, Tracer: SimulationTracer> {

@dvush dvush force-pushed the local_caches branch 2 times, most recently from f316ecf to d809ab8 Compare April 8, 2025 16:35
The idea behind ThreadBlockBuildingContext is to have a place that
is owned by a particular thread (for example, max profit ordering
builder) that can be used as a scratchpad for caching without doing
global caching shared between all threads.

Currently we have only one place for caches BlockBuildingContext. It
is global and shared between all builders, top of block simulation,
and finalization thread. This approach solves many problems with caching. For
example, we use it for eth-sparse-mpt root hash caching.

But sometimes we would like to have a cache that is local to the current
thread to avoid the overhead of mutex and multiple threads.

I've seen the need for caches like this multiple times.
This commit adds support for local caches like this.

The first thing that is moved to this new setup of 2 caches is cached reads. We had a
lot of trouble with passing CachedReads struct around and it
introduced itself into traits where it does not belong such as
BlockBuildingHelper or backtesting code. Here we move CachedReads to this uniform
setup and it simplifies cached reads state handling.
@ZanCorDX
Copy link
Contributor

ZanCorDX commented Apr 8, 2025

Is it possible to hide the ThreadBlockBuildingContext inside the BlockBuildingHelperFromProvider so we don't have to pass it to edit the block since it's always the same?

@dvush
Copy link
Contributor Author

dvush commented Apr 8, 2025

I decided against it because we move block building helper to send to other thread for sealing. This would be against the idea of having fixed cache that is:

  • tied to some thread
  • never cloned during the slot
  • it only fills with data and grows

If you put thread local cache with block building helper that would mean that we need to clone it every time we finish block building attempt.

@dvush
Copy link
Contributor Author

dvush commented Apr 8, 2025

The only way I see to make it work would be putting Mutex<Option<Arc<ThreadLocalBuildingContext>>> into block building helper and adding the following flow: you set up context for the context of the current thread and then you remove it when you send it to other thread. This is error prone and ugly, passing &mut to every method that requires it seems to be much better and you are actually protected by borrow checker since &mut is correct way to have this context in your code.

@dvush dvush merged commit dc7d3fc into develop Apr 9, 2025
4 checks passed
@dvush dvush deleted the local_caches branch April 9, 2025 12:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants