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

Avoid revalidating confirmed transaction during block evaluator recomputation #486

Merged
merged 3 commits into from Nov 11, 2019

Conversation

@tsachiherman
Copy link
Contributor

tsachiherman commented Nov 7, 2019

Summary

When OnNewBlock is called, we already know which transactions have been removed.
This PR attempted to avoid calling the "remember" function on these.

High level changes:

  • On the notifier, store both the block and delta state objects.
  • When sending OnNewBlock, send both block as well as the delta state
  • Take advantage of the added information in the txpool's onnewblock->recomputeEvaluator by excluding transactions the we know we don't need.
@tsachiherman tsachiherman requested a review from egieseke Nov 7, 2019
Copy link
Contributor

egieseke left a comment

Looks ok.

@tsachiherman tsachiherman changed the title Evaluate removing known transactions from pool Avoid revalidating confirmed transaction during block evaluator recomputation Nov 7, 2019
@tsachiherman tsachiherman requested a review from zeldovich Nov 7, 2019
Copy link
Contributor

zeldovich left a comment

In principle seems OK. Would be good to see some performance numbers that justify making the code messier.

@tsachiherman

This comment has been minimized.

Copy link
Contributor Author

tsachiherman commented Nov 7, 2019

@zeldovich

This comment has been minimized.

Copy link
Contributor

zeldovich commented Nov 7, 2019

could you take a look on the slack performance channel? this was my trigger for this change. I hope Eric could help me getting the pprof results tommorow. ( I.e. after )

On Thu, Nov 7, 2019, 3:28 PM Nickolai Zeldovich @.> wrote: @.* commented on this pull request. In principle seems OK. Would be good to see some performance numbers that justify making the code messier. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#486?email_source=notifications&email_token=AF2OOHY7YEBWYXRFYFOHUBTQSR26ZA5CNFSM4JKJNEWKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCKYWLVY#pullrequestreview-313615831>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AF2OOH3BKYACP4NKNHMJCFLQSR26ZANCNFSM4JKJNEWA .

I agree it's a reasonable hypothesis for what to optimize (and the code seems fine), but it'd be good to confirm that it really helps on performance before committing this complication to master.

@@ -65,12 +66,20 @@ type stateDelta struct {
hdr *bookkeeping.BlockHeader
}

// Txids returns the map fo the transactions Ids included
func (st *StateDelta) Txids() map[transactions.Txid]struct{} {

This comment has been minimized.

Copy link
@derbear

derbear Nov 7, 2019

Collaborator

Does it make sense just to expose txids? Doesn't seem like a getter is doing anything very meaningful here...
Alternatively, if you want to make the txids read-only, then perhaps it makes more sense to instead expose a isDup method that lets you check whether a txID is itself a duplicate.

This comment has been minimized.

Copy link
@tsachiherman

tsachiherman Nov 8, 2019

Author Contributor

I'd like to keep it as simple as possible. Adding isDup() would prevent us from iterating on the map. I don't mind exposing the map variable directly either.

@tsachiherman tsachiherman merged commit a1720b5 into algorand:master Nov 11, 2019
2 checks passed
2 checks passed
Travis CI - Pull Request Build Passed
Details
license/cla Contributor License Agreement is signed.
Details
@tsachiherman tsachiherman deleted the tsachiherman:tsachi/commitedchanges branch Nov 11, 2019
winder added a commit that referenced this pull request Nov 12, 2019
…putation (#486)

* Try changes.

* Address reviewer's concerns.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.