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: use UnsafeVec to replace Vec<Mutex<>> #267

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jackwener
Copy link
Contributor

last_locations will only be updated in record(), validate_read_locations(), and convert_writes_to_estimates(). For the same TxIdx, these functions cannot execute concurrently, so we can replace Vec<Mutex<>> with UnsafeVec.

`last_locations` will only be updated in `record()`, `validate_read_locations()`, and `convert_writes_to_estimates()`. For the same `TxIdx`, these functions cannot execute concurrently, so we can replace `Vec<Mutex<>>` with `UnsafeVec`.
@jackwener jackwener changed the title perf: use UnsafeVec to replace Vec<Mutex<>> feat: use UnsafeVec to replace Vec<Mutex<>> Aug 22, 2024
@hai-rise
Copy link
Contributor

hai-rise commented Aug 23, 2024

@jackwener Good insight as always!

Actually, there can be concurrent validation tasks for the same transaction index.
There can only be one execution task when the status transitions from ReadyToExecute to Executing:

pevm/src/scheduler.rs

Lines 125 to 131 in cce4fc9

if tx.status == IncarnationStatus::ReadyToExecute {
tx.status = IncarnationStatus::Executing;
return Some(Task::Execution(TxVersion {
tx_idx,
tx_incarnation: tx.incarnation,
}));
}

But a new validation task doesn't transition the status, so multiple can be scheduled for the same executed/validated transaction:

pevm/src/scheduler.rs

Lines 133 to 140 in cce4fc9

if matches!(
tx.status,
IncarnationStatus::Executed | IncarnationStatus::Validated
) {
return Some(Task::Validation(TxVersion {
tx_idx,
tx_incarnation: tx.incarnation,
}));

This happens when validation_idx is reset to a lower value. We don't want a blocking Validating status as we want the latter task to start (with the latest data) as soon as possible. A relevant optimization is to have a way to signal the on-going validation task(s) to exit when a new one (of the same transaction) starts.

Anyway, that is just extra information, as it should still be safe to have concurrent reads!
I had a local branch but with Vec<SyncUnsafeCell<LastLocations>>, which is much shorter than rolling our own UnsafeVec. Do you think it's worth re-trying this that way?

@jackwener
Copy link
Contributor Author

Actually, there can be concurrent validation tasks for the same transaction index.

Get it! Indeed, for the same TxIdx, there may be multiple validation tasks at the same time

I had a local branch but with Vec<SyncUnsafeCell<LastLocations>>, which is much shorter than rolling our own UnsafeVec. Do you think it's worth re-trying this that way?

Yes, it's will be easier and simpler

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