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.

Sign up for GitHub

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

Jump to bottom

Improve Conflict Detection in Parallelization by Considering Slots to Reduce False Positives #7923

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

Improve Conflict Detection in Parallelization by Considering Slots to Reduce False Positives #7923

wants to merge 8 commits into from

Conversation

matkt
Copy link
Contributor

@matkt matkt commented Nov 26, 2024 •
edited
Loading

PR description

This update improve conflict detection during transaction parallelization by checking storage slot changes. By considering modified slots in addition to addresses, we improve the accuracy of conflict detection.

Key Changes:

  • Added storage slot consideration in conflict detection.
  • Reduced false positives by more accurately identifying conflicts between transactions and block state.

Fixed Issue(s)

Thanks for sending a pull request! Have you done the following?

  • Checked out our contribution guidelines?
  • Considered documentation and added the doc-change-required label to this PR if updates are required.
  • Considered the changelog and included an update if required.
  • For database changes (e.g. KeyValueSegmentIdentifier) considered compatibility and performed forwards and backwards compatibility tests

Locally, you can run these tests to catch failures early:

  • unit tests: ./gradlew build
  • acceptance tests: ./gradlew acceptanceTest
  • integration tests: ./gradlew integrationTest
  • reference tests: ./gradlew ethereum:referenceTests:referenceTests

matkt added 3 commits November 17, 2024 08:37
@matkt
optimize trx selection
98ae489 
Signed-off-by: Karim Taam <karim.t2am@gmail.com>
@matkt
optimize transaction conflict detection
f4429c9 
Signed-off-by: Karim Taam <karim.t2am@gmail.com>
@matkt
clean tests
1528164 
Signed-off-by: Karim Taam <karim.t2am@gmail.com>
@ahamlat
Copy link
Contributor

ahamlat commented Nov 26, 2024 •
edited
Loading

This new conflict detection improved the number of parallelized transactions from ~40% to ~60%.
The numbers in the charts below are TPS.

Before this PR (24.10.0 running with --Xbonsai-parallel-tx-processing-enabled=true)
image

With this PR (running with --Xbonsai-parallel-tx-processing-enabled=true)
image

With more transactions executed in parallel, we noticed around 20% performance improvement compared to the last parallelization implementation. The numbers below are from instances running both EL and CL and are not using high spec flag.

image

We can notice also almost twice better performance when comparing to besu 24.10.0 with Teku as a CL

image

I don't see any overhead on the resource usage level.

@matkt
Copy link
Contributor Author

matkt commented Nov 26, 2024

we need to do stability tests before merging

@matkt
add fallback for parallelization
4f233dd 
Signed-off-by: Karim Taam <karim.t2am@gmail.com>
@matkt
change log level
b8e978e 
Signed-off-by: Karim Taam <karim.t2am@gmail.com>
ahamlat
ahamlat requested changes Dec 3, 2024
View reviewed changes
...hereum/trie/diffbased/common/worldview/accumulator/DiffBasedWorldStateUpdateAccumulator.java Outdated
@@ -504,6 +504,8 @@ public void commit() {
tracked.setStorageWasCleared(false); // storage already cleared for this transaction
}
});
getUpdatedAccounts().clear();
getDeletedAccounts().clear();
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest to do this clearing in a separate PR as it is not related to parallelization

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes will remove it . moved to #7974

@matkt
- remove clear during commit
720d753 
- copy storageKeyHashLookup when cloning accumulator

Signed-off-by: Karim Taam <karim.t2am@gmail.com>
macfarla
macfarla reviewed Dec 4, 2024
View reviewed changes
Copy link
Contributor

@macfarla macfarla left a comment

Choose a reason for hiding this comment

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

changelog entry?

...java/org/hyperledger/besu/ethereum/mainnet/parallelization/TransactionCollisionDetector.java Outdated Show resolved Hide resolved
...java/org/hyperledger/besu/ethereum/mainnet/parallelization/TransactionCollisionDetector.java Outdated Show resolved Hide resolved
@matkt
update comment
85bfd20 
Signed-off-by: Karim Taam <karim.t2am@gmail.com>
@matkt matkt force-pushed the feature/para-evm-2 branch from 68a5c15 to 85bfd20 Compare December 4, 2024 16:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@daniellehrner daniellehrner daniellehrner left review comments

@macfarla macfarla macfarla left review comments

@ahamlat ahamlat ahamlat requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@matkt @ahamlat @macfarla @daniellehrner