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

Write transactions earlier if possible #807

Merged
merged 3 commits into from
Dec 10, 2021
Merged

Conversation

tolikzinovyev
Copy link
Contributor

@tolikzinovyev tolikzinovyev commented Dec 10, 2021

Summary

If proto.EnableAssetCloseAmount is already true, we can start writing transactions before running the evaluator.

9200 -> 9460 TPS

Test Plan

TestAddBlockAssetCloseAmountInTxnExtra already tests that we write transactions when proto.EnableAssetCloseAmount is false. There are multiple test checking that transactions are written when proto.EnableAssetCloseAmount is true.

@tolikzinovyev
Copy link
Contributor Author

@winder let me know if you like the change. If so, I will add a small test.

var err0 error
wg.Add(1)
go func() {
defer wg.Done()

f := func(tx pgx.Tx) error {
if !protoChanged {
err := writer.AddTransactions(block, block.Payset, tx)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you want want this in a separate txWithRetry so that it is written in parallel to AddTransactionParticipation?

I like this change but am not sure we need it right now. At the very least I think it's good to add a comment below near AddTransactions, something along the lines of:

if proto.EnableAssetCloseAmount was already true, the transactions can be loaded with block.Payset instead of modifiedTransactions while the evaluator is running

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, writing transactions and transaction participation together takes less time then eval + writing other state, so we don't need more parallelism; it would probably hurt.

I will add a comment.

@tolikzinovyev
Copy link
Contributor Author

Actually, we already have a test that I wanted: TestAddBlockAssetCloseAmountInTxnExtra.

@tolikzinovyev tolikzinovyev marked this pull request as ready for review December 10, 2021 19:24
Copy link
Contributor

@winder winder left a comment

Choose a reason for hiding this comment

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

LGTM, please just add one more comment suggestion

@tolikzinovyev tolikzinovyev merged commit fc0bd06 into develop Dec 10, 2021
@tolikzinovyev tolikzinovyev deleted the tolik/par-import-impr branch December 10, 2021 21:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants