Skip to content
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

feat: Allow concurrent world state access #11216

Merged
merged 12 commits into from
Jan 15, 2025
Merged

Conversation

PhilWindle
Copy link
Collaborator

@PhilWindle PhilWindle commented Jan 14, 2025

Implements per-fork queues for requests to the native world state following it's concurrency rules. Also tightens up aspects of the cached store to ensure reads of committed data don't access anything uncommitted.

1. Reads of committed state never need to be queued. LMDB uses MVCC to ensure readers see a consistent view of the DB.
2. Reads of uncommitted state can happen concurrently with other reads of uncommitted state on the same fork (or reads of committed state)
3. All writes require exclusive access to their respective fork

Copy link
Contributor

github-actions bot commented Jan 14, 2025

Changes to circuit sizes

Generated at commit: 41e8a878c8f540cd6529c44b0cd85a8b4ed2e259, compared to commit: dfb0db572868896bea27a13606da2a7e3c10f31e

🧾 Summary (100% most significant diffs)

Program ACIR opcodes (+/-) % Circuit size (+/-) %
rollup_base_private +49 ❌ +0.04% +60 ❌ +0.00%
rollup_base_public +56 ❌ +0.02% +67 ❌ +0.00%

Full diff report 👇
Program ACIR opcodes (+/-) % Circuit size (+/-) %
rollup_base_private 138,590 (+49) +0.04% 1,772,615 (+60) +0.00%
rollup_base_public 366,997 (+56) +0.02% 3,236,019 (+67) +0.00%

Copy link
Contributor

@alexghr alexghr left a comment

Choose a reason for hiding this comment

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

LGTM. Just a question on queue draining. Everything is else is minor

Comment on lines 728 to 729
const { block: block1 } = await mockBlock(1, 8, fork);
const { block: block2 } = await mockBlock(2, 8, fork);
Copy link
Contributor

Choose a reason for hiding this comment

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

IIRC mockBlock modified the fork parameter with the block info (in order to create the correct block header).

Writing the same block data to the same fork inserts empty leaves into the public data tree so the sibling path will still change. The test is still correct but we might want to use two different forks: one for building the blocks and the other to test the writes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh thats was stupid. The test should be using a different fork!

// As many non-mutating requests as we encounter until
// we exhaust the queue or we reach a mutating request
while (this.requests.length > 0) {
const next = this.requests[0];
Copy link
Contributor

Choose a reason for hiding this comment

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

Was going to suggest using shfit here but not all branches of the if statemnent remove the head of the list

Comment on lines 102 to 110
op.request()
.then(resp => {
op.promise.resolve(resp);
this.ops.delete(op.requestId);
})
.catch(err => {
op.promise.reject(err);
this.ops.delete(op.requestId);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be simplified slightly

Suggested change
op.request()
.then(resp => {
op.promise.resolve(resp);
this.ops.delete(op.requestId);
})
.catch(err => {
op.promise.reject(err);
this.ops.delete(op.requestId);
});
op.request().then(op.promise.resolve, op.promise.reject)
.finally(() => {
this.ops.delete(op.requestId);
})

Comment on lines 162 to 172
op.request()
.then(resp => {
op.promise.resolve(resp);
this.checkAndEnqueue(op);
this.ops.delete(op.requestId);
})
.catch(err => {
op.promise.reject(err);
this.checkAndEnqueue(op);
this.ops.delete(op.requestId);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above. If the ops.delete was moved inside checkAndEnqueue it could be even cleaner I think

Suggested change
op.request()
.then(resp => {
op.promise.resolve(resp);
this.checkAndEnqueue(op);
this.ops.delete(op.requestId);
})
.catch(err => {
op.promise.reject(err);
this.checkAndEnqueue(op);
this.ops.delete(op.requestId);
});
op.request().then(op.promise.resolve, op.promise.reject)
.finally(resp => {
this.checkAndEnqueue(op);
this.ops.delete(op.requestId);
})

@PhilWindle PhilWindle added e2e-all CI: Enables this CI job. network-all Run this CI job. labels Jan 15, 2025
@PhilWindle PhilWindle enabled auto-merge (squash) January 15, 2025 14:10
@PhilWindle PhilWindle linked an issue Jan 15, 2025 that may be closed by this pull request
@PhilWindle PhilWindle merged commit 17aa4b4 into master Jan 15, 2025
80 checks passed
@PhilWindle PhilWindle deleted the pw/tree-concurrency branch January 15, 2025 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
e2e-all CI: Enables this CI job. network-all Run this CI job.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable concurrent reads of world state
2 participants