Conversation
LukaszRozmej
left a comment
LukaszRozmej
left a comment
There was a problem hiding this comment.
@marcindsobczak could you take a look?
|
|
||
| codeFlushTask.GetAwaiter().GetResult(); | ||
|
|
There was a problem hiding this comment.
This is explicit optimization by @benaadams not to do it here, but in the background, then in Commit you have wait for the one from previous block.
There was a problem hiding this comment.
Really? Then what is with the other wait at the end of the same function?
There was a problem hiding this comment.
I have a PR that moves it to end of block processing; though is horribly merged conflicted atm #9012
There was a problem hiding this comment.
So @benaadams is this PR acceptable or makes your solution unachievable and you are confident you will revisit it?
| for (int i = 0; i < addedTxsCount; i++) | ||
| { |
There was a problem hiding this comment.
Broadcast should be thread safe so you can put it under Parallel.For
There was a problem hiding this comment.
At least it should be - order doesn't matter here so worth to move to parallelized part and verify it
| for (int i = 0; i < addedTxsCount; i++) | ||
| { |
There was a problem hiding this comment.
Pull Request Overview
This PR optimizes test execution time in the transaction pool test suite by implementing parallelization and reducing test repetition. The goal is to reduce test runtime from 12 minutes to approximately 5 minutes.
Key Changes:
- Parallelized transaction creation in multiple test methods using
Parallel.ForandParallel.ForEach - Reduced the number of private keys used in blob transaction tests from full
TestItem.PrivateKeysarray to first 64 elements - Decreased test repetition count from 10 to 2 for nonce manager tests
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| TxPoolTests.Blobs.cs | Parallelized transaction creation and reduced private key count for blob indexing tests |
| TxBroadcasterTests.cs | Parallelized transaction creation in two broadcast test methods |
| NonceManagerTests.cs | Reduced test repetition count from 10 to 2 for nonce locking test |
| StateProvider.cs | Added synchronous wait for code flush task completion |
Comments suppressed due to low confidence (1)
src/Nethermind/Nethermind.TxPool.Test/TxBroadcasterTests.cs:1
- Parallel transaction creation may cause race conditions when accessing the shared
_ethereumEcdsainstance. Consider using thread-safe alternatives or creating separate ECDSA instances per thread.
// SPDX-FileCopyrightText: 2022 Demerzel Solutions Limited
| FlushToTree(); | ||
| } | ||
|
|
||
| codeFlushTask.GetAwaiter().GetResult(); |
There was a problem hiding this comment.
Using GetAwaiter().GetResult() can cause thread pool starvation and deadlocks. Consider using await if the method can be made async, or use ConfigureAwait(false) to avoid potential deadlocks: codeFlushTask.ConfigureAwait(false).GetAwaiter().GetResult().
|
@asdacap should you experiment with parallelizing broadcast and txpool usage in tests? |
|
I just want faster test 😢. |
* Make txpool test run faster * Fix randomly failing pyspec test * parallelize BroadcasterTests more * Even more * Parallelize more * Remove this part * more parallel --------- Co-authored-by: lukasz.rozmej <lukasz.rozmej@gmail.com>
Types of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?
Notes on testing