-
Notifications
You must be signed in to change notification settings - Fork 995
BAL-based perfect parallelization of transaction execution #9536
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
Conversation
4a3e7dd to
527422d
Compare
...org/hyperledger/besu/ethereum/mainnet/parallelization/BalConcurrentTransactionProcessor.java
Outdated
Show resolved
Hide resolved
...org/hyperledger/besu/ethereum/mainnet/parallelization/BalConcurrentTransactionProcessor.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Miroslav Kovar <miroslavkovar@protonmail.com>
Signed-off-by: Miroslav Kovar <miroslavkovar@protonmail.com>
…y `PreprocessingContext`, use single preprocessor Signed-off-by: Miroslav Kovar <miroslavkovar@protonmail.com>
527422d to
b2aafc3
Compare
…mplementations Signed-off-by: Miroslav Kovar <miroslavkovar@protonmail.com>
…or implementations Signed-off-by: Miroslav Kovar <miroslavkovar@protonmail.com>
matkt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
really like the new implementation
matkt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, if we can just add tests for BALL //. I will run some nodes on mainnet with this PR to be sure we don't have any regression in the // part
Signed-off-by: Miroslav Kovar <miroslavkovar@protonmail.com>
0f1c2c9 to
56b9830
Compare
…plementation Signed-off-by: Miroslav Kovar <miroslavkovar@protonmail.com>
263baec to
f4f1d46
Compare
Signed-off-by: Miroslav Kovar <miroslavkovar@protonmail.com>
f4f1d46 to
9c08d04
Compare
Signed-off-by: Miroslav Kovar <miroslavkovar@protonmail.com>
...hyperledger/besu/ethereum/mainnet/parallelization/ParallelBlockTransactionProcessorTest.java
Outdated
Show resolved
Hide resolved
...hyperledger/besu/ethereum/mainnet/parallelization/ParallelBlockTransactionProcessorTest.java
Outdated
Show resolved
Hide resolved
...hyperledger/besu/ethereum/mainnet/parallelization/ParallelBlockTransactionProcessorTest.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Karim Taam <karim.t2am@gmail.com>
|
|
||
| public abstract class ParallelBlockTransactionProcessor { | ||
|
|
||
| protected CompletableFuture<ParallelizedTransactionContext>[] futures; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Java arrays are not thread safe. I would suggest using a ConcurrentHashMap with an Integer as an index.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't a problem because we change each index separately. That's already how it's done in the current code. We'll never have the same index modified by multiple parts of the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should change it. Using a data structure that is not thread safe in a multi threaded environment can lead to inconsistent behaviour.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think adding synchronization where it's not necessary might impact performance. maybe not too much. It's not necessarily within the scope of this PR, but if Mirgee wants to change it, I'm not against it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A ConcurrentHashMap does introduce some synchronization, but with fine-grained locking and reads are lock-free. The array can explain some of the edge cases where we still have errors on parallel transaction processing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is true that this would be a problem if multiple threads were modifying the array simultaneously, but in this case the array is always accessed exclusively by the main thread, so I don't see the need to use a synchronized data structure.
...ledger/besu/ethereum/mainnet/parallelization/ParallelizedConcurrentTransactionProcessor.java
Show resolved
Hide resolved
...org/hyperledger/besu/ethereum/mainnet/parallelization/BalConcurrentTransactionProcessor.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Miroslav Kovar <miroslavkovar@protonmail.com>
e0f7e5a to
db3cd34
Compare
Signed-off-by: Miroslav Kovar <miroslavkovar@protonmail.com>
…el processing path Signed-off-by: Miroslav Kovar <miroslavkovar@protonmail.com>
...org/hyperledger/besu/ethereum/mainnet/parallelization/BalConcurrentTransactionProcessor.java
Show resolved
Hide resolved
Signed-off-by: Miroslav Kovar <miroslavkovar@protonmail.com>
0fd2a1d to
4aad685
Compare
Signed-off-by: Miroslav Kovar <miroslavkovar@protonmail.com>
...org/hyperledger/besu/ethereum/mainnet/parallelization/BalConcurrentTransactionProcessor.java
Outdated
Show resolved
Hide resolved
...org/hyperledger/besu/ethereum/mainnet/parallelization/BalConcurrentTransactionProcessor.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Miroslav Kovar <miroslavkovar@protonmail.com>
matkt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Adds
--Xbal-perfect-parallelization-enabledconfig flag, enabled by default, which activates BAL-based perfect parallelization of transaction execution.Adds a new interface,
ParallelBlockTransactionProcessor, implemented byParallelizedConcurrentTransactionProcessor(optimistic parallelization) andBalConcurrentTransactionProcessor(BAL-based parallelization), and used byParallelTransactionPreprocessingto launch parallel block (pre)processing, which returnsPreprocessingContext(unified for both processing types) currently containing only reference to theParallelBlockTransactionProcessorused to obtain the transaction processing results later.BalConcurrentTransactionProcessorlaunches and manages the futures processing individual transactions in the background. At the beginning of transaction processing, the prestate is loaded by applying corresponding state changes from the BAL. When the transaction processing result is ready, the state changes are imported to the block's accumulator in the main thread. Some categories of (pre)processing failures result in fallback to sequential processing.The
AbstractBlockProcessortrusts that the presence of absence of BALs in the block's body was validated beforeprocessBlockwas called, and then uses the BALs if they are present, and, based on configuration, falls back to optimistic parallel or sequential processing. This is because the block may still be valid even if BALs are active and BAL is missing from the block body, e.g. in case of replaying a block older than BAL retention period.These changes were tested by running a local devnet with spamoor and tx_fuzz, and it was verified all transactions are executed in parallel without conflicts.
An optional fail-fast transaction-wise validation of
PartialBlockAccessListViews fromTransactionProcessingResults against the blocksBlockAccessList(validating execution matches BAL) will be added separately, as well as BAL-based batch read prefetching.Resolves #9079 .