Improve withdrawals processing performance #5026

ahamlat · 2023-01-31T10:47:01Z

Signed-off-by: Ameziane H ameziane.hamlat@consensys.net

PR description

During withdrawals processing on devnet, profiling the execution showed two possible little improvements regarding performance.

The flamegraph at the block processing level

With this PR, Withdrawals processing doesn't appear in the profiling flame graph (see below) which means it is faster than before. We can check also that the withdrawals are processed for each block with this log (see 16 ws for 16 withdrawals).

2023-01-31 14:21:24.413+00:00 | vert.x-worker-thread-0 | INFO  | AbstractEngineNewPayload | Imported #28,191 / 0 tx / 16 ws / base fee 7 wei / 0 (0.0%) gas / (0x6a9b06fb18ffdae6377bd0d60d21a665df02c587393402e73cb2502eeda13cae) in 0.001s. Peers: 19
2023-01-31 14:21:37.092+00:00 | vert.x-worker-thread-0 | INFO  | AbstractEngineNewPayload | Imported #28,192 / 0 tx / 16 ws / base fee 7 wei / 0 (0.0%) gas / (0x7c8060685283b172ed030cf8e7d7d609be61b16f58c2e49c493abceea80e572d) in 0.001s. Peers: 14
2023-01-31 14:21:48.651+00:00 | vert.x-worker-thread-0 | INFO  | AbstractEngineNewPayload | Imported #28,193 / 0 tx / 16 ws / base fee 7 wei / 0 (0.0%) gas / (0x96494ca7da0cdc097a19f829f9decd16ed18b5d7f9d2c3920ca609a7936d09c3) in 0.001s. Peers: 14
2023-01-31 14:22:00.572+00:00 | vert.x-worker-thread-0 | INFO  | AbstractEngineNewPayload | Imported #28,194 / 0 tx / 16 ws / base fee 7 wei / 0 (0.0%) gas / (0xb2f8c474e6f5d348b49269068cc0bd06da5affadd51411fb2eec56d5e59a58be) in 0.001s. Peers: 16

The flamegraph at the block processing level

Fixed Issue(s)

Documentation

I thought about documentation and added the doc-change-required label to this PR if
updates are required.

Changelog

I thought about the changelog and included a changelog update if required.

Signed-off-by: Ameziane H <ameziane.hamlat@consensys.net>

usmansaleem · 2023-01-31T23:20:41Z

ethereum/core/src/main/java/org/hyperledger/besu/ethereum/bonsai/BonsaiWorldStateUpdater.java

@@ -284,47 +284,49 @@ public void commit() {
                            new BonsaiValue<>(wrappedWorldView().getCode(addr).orElse(null), null));
                pendingCode.setUpdated(updatedAccount.getCode());
              }
+              if (!updatedAccount.getUpdatedStorage().isEmpty()) {


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

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?

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?

@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.

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

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).

usmansaleem

LGTM, one minor nit pick.

datatypes/src/main/java/org/hyperledger/besu/datatypes/GWei.java

CHANGELOG.md

siladu · 2023-02-01T05:41:38Z

ethereum/core/src/main/java/org/hyperledger/besu/ethereum/bonsai/BonsaiWorldStateUpdater.java

@@ -284,47 +284,49 @@ public void commit() {
                            new BonsaiValue<>(wrappedWorldView().getCode(addr).orElse(null), null));
                pendingCode.setUpdated(updatedAccount.getCode());
              }
+              if (!updatedAccount.getUpdatedStorage().isEmpty()) {


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?

…or the withdrawals Signed-off-by: Ameziane H <ameziane.hamlat@consensys.net>

* Improve withdrawals processing performance * Add a changelog entry * refactoring + remove the changelog entry as we have not change logs for the withdrawals Signed-off-by: Ameziane H <ameziane.hamlat@consensys.net>

ahamlat added 2 commits January 31, 2023 11:38

Improve withdrawals processing performance

6ab849c

Signed-off-by: Ameziane H <ameziane.hamlat@consensys.net>

Merge branch 'main' into improve-withdrawals-performance

f05d666

ahamlat requested review from siladu and gezero January 31, 2023 14:25

Add a changelog entry

ea3c23a

Signed-off-by: Ameziane H <ameziane.hamlat@consensys.net>

ahamlat marked this pull request as ready for review January 31, 2023 14:33

ahamlat added the performance label Jan 31, 2023

siladu requested review from jframe and usmansaleem January 31, 2023 18:34

usmansaleem reviewed Jan 31, 2023

View reviewed changes

usmansaleem approved these changes Jan 31, 2023

View reviewed changes

usmansaleem reviewed Jan 31, 2023

View reviewed changes

datatypes/src/main/java/org/hyperledger/besu/datatypes/GWei.java Outdated Show resolved Hide resolved

siladu reviewed Feb 1, 2023

View reviewed changes

refactoring + remove the changelog entry as we have not change logs f…

4c53b43

…or the withdrawals Signed-off-by: Ameziane H <ameziane.hamlat@consensys.net>

jframe approved these changes Feb 2, 2023

View reviewed changes

Merge branch 'main' into improve-withdrawals-performance

d81efc6

ahamlat merged commit cdfc579 into hyperledger:main Feb 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve withdrawals processing performance #5026

Improve withdrawals processing performance #5026

ahamlat commented Jan 31, 2023 •

edited

Loading

usmansaleem Jan 31, 2023

siladu Feb 1, 2023 •

edited

Loading

jframe Feb 1, 2023

ahamlat Feb 1, 2023 •

edited

Loading

ahamlat Feb 1, 2023 •

edited

Loading

ahamlat Feb 1, 2023 •

edited

Loading

usmansaleem left a comment

siladu Feb 1, 2023 •

edited

Loading

Improve withdrawals processing performance #5026

Improve withdrawals processing performance #5026

Conversation

ahamlat commented Jan 31, 2023 • edited Loading

PR description

Fixed Issue(s)

Documentation

Changelog

usmansaleem Jan 31, 2023

Choose a reason for hiding this comment

siladu Feb 1, 2023 • edited Loading

Choose a reason for hiding this comment

jframe Feb 1, 2023

Choose a reason for hiding this comment

ahamlat Feb 1, 2023 • edited Loading

Choose a reason for hiding this comment

ahamlat Feb 1, 2023 • edited Loading

Choose a reason for hiding this comment

ahamlat Feb 1, 2023 • edited Loading

Choose a reason for hiding this comment

usmansaleem left a comment

Choose a reason for hiding this comment

siladu Feb 1, 2023 • edited Loading

Choose a reason for hiding this comment

ahamlat commented Jan 31, 2023 •

edited

Loading

siladu Feb 1, 2023 •

edited

Loading

ahamlat Feb 1, 2023 •

edited

Loading

ahamlat Feb 1, 2023 •

edited

Loading

ahamlat Feb 1, 2023 •

edited

Loading

siladu Feb 1, 2023 •

edited

Loading