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.

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

Use single heavy input in the transaction_throughput.rs benchmarks #2205

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

Conversation

xgreenx
Copy link
Collaborator

@xgreenx xgreenx commented Sep 16, 2024

@SilentCicero

Benchmark Adjustment
As of the latest benchmarks for TPS on the FuelVM/Client, we seem to do benchmarks each with two signatures per transaction.
This is highly unusual for blockchain benchmarks, namely because there is usually only a single signature payload to verify per transaction in the models for all other blockchains.
We have conservatively, for almost no reason/benefit, halved the actual tps capability of Fuel in our public benchmarks by making each benchmark do two identical signature verifications.
This is because the signature payload being checked twice is identical and can be trivially cached and would be cached in an optimized real world transactional scenario. It would be far more realistic to do fuel client benchmarks with one signature predicate, one non signature predicate and two inputs and two outputs (one fee and one transfer), for realism.

Before requesting review

  • I have reviewed the code myself

@xgreenx xgreenx requested a review from a team September 16, 2024 15:53
@xgreenx xgreenx self-assigned this Sep 16, 2024
@xgreenx xgreenx added the no changelog Skip the CI check of the changelog modification label Sep 16, 2024
@xgreenx xgreenx enabled auto-merge (squash) September 18, 2024 07:13
Copy link
Contributor

@netrome netrome left a comment

Choose a reason for hiding this comment

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

A few questions from me but otherwise this looks good.

Comment on lines +197 to +198
tx.estimate_predicates(&checked_parameters(), MemoryInstance::new())
.expect("Predicate check failed");
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the purpose of estimating the predicates here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We don't use predicate instead. We use one signed coin along with one predicate. All benchmarks now use at least one predicate that returns true. The second input is different and the name reflects it

.finalize();
tx.estimate_predicates(&checked_parameters(), MemoryInstance::new())
.expect("Predicate check failed");
tx
};
bench_txs("signed transfers", c, generator);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we rename this to "predicate transfer" now when we use a predicate instead of a signed coin input?

Copy link
Contributor

@rafal-ch rafal-ch left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no changelog Skip the CI check of the changelog modification
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants