risechain / pevm

Blazingly fast Parallel EVM
MIT License
217 stars 40 forks source link

perf: remove useless fetch_add #187

Closed jackwener closed 2 months ago

jackwener commented 3 months ago

When I read the source code, I found exist two fetch_add

I found fetch_add in next_task is redundant.

hai-rise commented 3 months ago

Ah, this one is not redundant at all. A common race condition during task scheduling is many threads acquiring the same execution_idx first, before doing a completion check and trying a validation or execution task.

Without this fetch_add, 4-5 threads will all try the same execution_idx leading to contention at the transactions_status[execution_idx] mutex. Only the first will win the task while the others block.

With this fetch_add, the threads will correctly alternate execution_idx, execution_idx + 1, execution_idx + 2, etc., without any Mutex contention.

The one in try_execute serves a different purpose alternating execution_idx when the tried index isn't ready for execution.

While reducing atomic operations is critical to minimize synchronization costs, benchmarks showed that mutex contention & multiple threads trying the same task are even costlier. An improvement would likely require re-designing the scheduler, like spawning a dedicated scheduler thread that manages these indices locally to reduce synchronization from N-to-N to N-to-1.

jackwener commented 3 months ago

Thank you so much for your explanation!❤️

hai-rise commented 2 months ago

@jackwener I could combine the fetch_adds on execution_idx now, so let's close this PR 🙏.