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

Improve withdrawals processing performance #5026

Merged
merged 5 commits into from
Feb 2, 2023
Merged

Improve withdrawals processing performance #5026

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6ab849c
Improve withdrawals processing performance
ahamlat Jan 31, 2023
f05d666
Merge branch 'main' into improve-withdrawals-performance
ahamlat Jan 31, 2023
ea3c23a
Add a changelog entry
ahamlat Jan 31, 2023
4c53b43
refactoring + remove the changelog entry as we have not change logs f…
ahamlat Feb 1, 2023
d81efc6
Merge branch 'main' into improve-withdrawals-performance
ahamlat Feb 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
- Add implementation for eth_createAccessList RPC method [#4942](https://github.com/hyperledger/besu/pull/4942)
- Updated reference tests to v11.3 [#4996](https://github.com/hyperledger/besu/pull/4996)
- Add DebugGetRawBlock and DebugGetRawHeader RPC methods [#5011](https://github.com/hyperledger/besu/pull/5011)
- Improve withdrawals processing performance [#5026](https://github.com/hyperledger/besu/pull/5026)
ahamlat marked this conversation as resolved.
Show resolved Hide resolved

### Bug Fixes

Expand Down
2 changes: 1 addition & 1 deletion datatypes/src/main/java/org/hyperledger/besu/datatypes/GWei.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ public static GWei fromHexString(final String str) {
* @return Wei
*/
public Wei getAsWei() {
return Wei.of(getAsBigInteger().multiply(BigInteger.TEN.pow(9)));
return Wei.of(getAsBigInteger().multiply(BigInteger.valueOf(1_000_000_000L)));
ahamlat marked this conversation as resolved.
Show resolved Hide resolved
}

@Override
Expand Down
80 changes: 41 additions & 39 deletions ...reum/core/src/main/java/org/hyperledger/besu/ethereum/bonsai/BonsaiWorldStateUpdater.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,47 +284,49 @@ public void commit() {
new BonsaiValue<>(wrappedWorldView().getCode(addr).orElse(null), null));
pendingCode.setUpdated(updatedAccount.getCode());
}
if (!updatedAccount.getUpdatedStorage().isEmpty()) {
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: if we flip the condition and return early, we might avoid the indentions. i.e. if (updatedAccount.getUpdatedStorage.isEmpty() {return;}

Copy link
Contributor

@siladu siladu Feb 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

It's not clear to me why this improves withdrawals performance, can you please explain?

Also, it seems to be a change that will affect things other than withdrawals, so wondering what has been done to ensure we haven't regressed anything else? Would this be covered by existing automated tests or have you performed some relevant manual tests?

Finally, it's bonsai specific - what if we process withdrawals with FOREST?

Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like different optimisation from the one mentioned initially. I thought the optimization was to avoid the cost of the parallel streams overhead. This looks like it avoids doing unnecessary work if there is no storage to update. Which is a good thing though.

Are you planning to add an optimization to avoid the parallel stream for withdrawal balance updates?

Copy link
Contributor Author

@ahamlat ahamlat Feb 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@siladu You're right, the PR description is not detailed enough. During block processing, we have 3 big consumers in terms of time processing : Transactions execution, commit the changes of each transaction to the worldstate updater and persist all the changes (in memory or on disk). During withdrawals processing, we also commit the changes of all the withdrawals to a worldstate updater but we use the same method (commit) that is used to manage all the cases (accounts deletion, storage update, storage deletion, ..etc). In the case of withdrawals, we only modify the balances, so we should just focus on accounts updates.
@jframe good point, you're totally right. I wanted to start with this first optimisation to see if it is sufficient, and avoid creating another specific method for withdrawals to keep the code simple.

Copy link
Contributor Author

@ahamlat ahamlat Feb 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I added the flamegraphs at the block processing level in the description to better see that withdrawals processing doesn't appear as it takes less time than before

Copy link
Contributor Author

@ahamlat ahamlat Feb 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Regarding FOREST, I couldn't get more insights from the profiling as it couldn't capture anything (after my last modification). It could be interesting to do the same work with lots of transactions to see the real impact. Currently, block processing with withdrawals takes less than 2 ms, that's why it is hard to get samples on Block processing. Async Profiler, the tool I'm using, has a default frequency of 100 Hz (1 sample every 10 ms).

final StorageConsumingMap<BonsaiValue<UInt256>> pendingStorageUpdates =
storageToUpdate.computeIfAbsent(
updatedAddress,
__ ->
new StorageConsumingMap<>(
updatedAddress, new ConcurrentHashMap<>(), storagePreloader));
if (tracked.getStorageWasCleared()) {
storageToClear.add(updatedAddress);
pendingStorageUpdates.clear();
}

final StorageConsumingMap<BonsaiValue<UInt256>> pendingStorageUpdates =
storageToUpdate.computeIfAbsent(
updatedAddress,
__ ->
new StorageConsumingMap<>(
updatedAddress, new ConcurrentHashMap<>(), storagePreloader));
if (tracked.getStorageWasCleared()) {
storageToClear.add(updatedAddress);
pendingStorageUpdates.clear();
}

final TreeSet<Map.Entry<UInt256, UInt256>> entries =
new TreeSet<>(Map.Entry.comparingByKey());
entries.addAll(updatedAccount.getUpdatedStorage().entrySet());

// parallel stream here may cause database corruption
entries.forEach(
storageUpdate -> {
final UInt256 keyUInt = storageUpdate.getKey();
final Hash slotHash = Hash.hash(keyUInt);
final UInt256 value = storageUpdate.getValue();
final BonsaiValue<UInt256> pendingValue = pendingStorageUpdates.get(slotHash);
if (pendingValue == null) {
pendingStorageUpdates.put(
slotHash,
new BonsaiValue<>(
updatedAccount.getOriginalStorageValue(keyUInt), value));
} else {
pendingValue.setUpdated(value);
}
});

updatedAccount.getUpdatedStorage().clear();

if (pendingStorageUpdates.isEmpty()) {
storageToUpdate.remove(updatedAddress);
}
final TreeSet<Map.Entry<UInt256, UInt256>> entries =
new TreeSet<>(Map.Entry.comparingByKey());
entries.addAll(updatedAccount.getUpdatedStorage().entrySet());

// parallel stream here may cause database corruption
entries.forEach(
storageUpdate -> {
final UInt256 keyUInt = storageUpdate.getKey();
final Hash slotHash = Hash.hash(keyUInt);
final UInt256 value = storageUpdate.getValue();
final BonsaiValue<UInt256> pendingValue = pendingStorageUpdates.get(slotHash);
if (pendingValue == null) {
pendingStorageUpdates.put(
slotHash,
new BonsaiValue<>(
updatedAccount.getOriginalStorageValue(keyUInt), value));
} else {
pendingValue.setUpdated(value);
}
});

updatedAccount.getUpdatedStorage().clear();

if (pendingStorageUpdates.isEmpty()) {
storageToUpdate.remove(updatedAddress);
}

if (tracked.getStorageWasCleared()) {
tracked.setStorageWasCleared(false); // storage already cleared for this transaction
if (tracked.getStorageWasCleared()) {
tracked.setStorageWasCleared(
false); // storage already cleared for this transaction
}
}
});
}
Expand Down