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

perf!: Add HasAccount to the AuthKeeper to save protobuf decoding time #10022

Merged
merged 7 commits into from
Aug 31, 2021

Conversation

ValarDragon
Copy link
Contributor

@ValarDragon ValarDragon commented Aug 28, 2021

Description

This PR adds a HasAccount method to the AuthKeeper, which lets you just check if an account exists, without unmarshalling it. This PR also fixes one unnecessary spot that it appears within in SendCoins.

This PR is API breaking (Adds HasAccount to the AuthKeeper) but is not state machine breaking.

We found in the Osmosis epoch time, the many accesses to GetAccount's proto unmarshalling was a significant slowdown. This PR has been benchmarked to improve that performance! (As it halves the amount of proto unmarshals in SendCoins. There is still one more remaining, a subsidiary call within LockedCoins(), within SubtractCoins)


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification - Yes to my knowledge?
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

We found in the Osmosis epoch time, the many accesses to GetAccount's proto unmarshalling was a significant slowdown.
This adds a HasAccount method to the AuthKeeper, and fixes one unnecessary spot that it appears within in SendCoins
@codecov
Copy link

codecov bot commented Aug 28, 2021

Codecov Report

Merging #10022 (9cba55e) into master (78c3c4e) will decrease coverage by 0.00%.
The diff coverage is 57.14%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10022      +/-   ##
==========================================
- Coverage   63.56%   63.56%   -0.01%     
==========================================
  Files         572      572              
  Lines       53581    53584       +3     
==========================================
+ Hits        34058    34059       +1     
- Misses      17580    17582       +2     
  Partials     1943     1943              
Impacted Files Coverage Δ
x/auth/keeper/account.go 47.91% <0.00%> (-3.20%) ⬇️
x/auth/keeper/keeper.go 67.27% <ø> (ø)
x/bank/keeper/send.go 82.98% <100.00%> (ø)
crypto/keys/internal/ecdsa/privkey.go 84.21% <0.00%> (+1.75%) ⬆️

Copy link
Member

@tac0turtle tac0turtle left a comment

Choose a reason for hiding this comment

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

LGTM

x/auth/keeper/account.go Outdated Show resolved Hide resolved
Co-authored-by: Federico Kunze Küllmer <31522760+fedekunze@users.noreply.github.com>
@tac0turtle tac0turtle added A:automerge Automatically merge PR once all prerequisites pass. T: Performance Performance improvements labels Aug 30, 2021
@ValarDragon
Copy link
Contributor Author

Automerge seems like its failing due to unrelated cosmovisor error :(

@tac0turtle tac0turtle merged commit 7273bd3 into cosmos:master Aug 31, 2021
@orijbot
Copy link

orijbot commented Aug 31, 2021

@fedekunze
Copy link
Collaborator

@Mergifyio backport release/v0.45.x

@mergify
Copy link
Contributor

mergify bot commented Dec 28, 2021

backport release/v0.45.x

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Dec 28, 2021
#10022)

* Add HasAccount to the AuthKeeper to save protobuf decoding time

We found in the Osmosis epoch time, the many accesses to GetAccount's proto unmarshalling was a significant slowdown.
This adds a HasAccount method to the AuthKeeper, and fixes one unnecessary spot that it appears within in SendCoins

* Update Spec

* Add Changelog entry

* Fix lint & use speedup in SendCoins

* Update x/auth/keeper/account.go

Co-authored-by: Federico Kunze Küllmer <31522760+fedekunze@users.noreply.github.com>

Co-authored-by: Federico Kunze Küllmer <31522760+fedekunze@users.noreply.github.com>
(cherry picked from commit 7273bd3)

# Conflicts:
#	CHANGELOG.md
@fedekunze fedekunze mentioned this pull request Dec 28, 2021
12 tasks
tac0turtle pushed a commit that referenced this pull request Dec 31, 2021
…e (backport #10022) (#10847)

* perf!: Add HasAccount to the AuthKeeper to save protobuf decoding time (#10022)

* Add HasAccount to the AuthKeeper to save protobuf decoding time

We found in the Osmosis epoch time, the many accesses to GetAccount's proto unmarshalling was a significant slowdown.
This adds a HasAccount method to the AuthKeeper, and fixes one unnecessary spot that it appears within in SendCoins

* Update Spec

* Add Changelog entry

* Fix lint & use speedup in SendCoins

* Update x/auth/keeper/account.go

Co-authored-by: Federico Kunze Küllmer <31522760+fedekunze@users.noreply.github.com>

Co-authored-by: Federico Kunze Küllmer <31522760+fedekunze@users.noreply.github.com>
(cherry picked from commit 7273bd3)

# Conflicts:
#	CHANGELOG.md

* conflicts

* changelog

* changelog

Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com>
Co-authored-by: Federico Kunze Küllmer <federico.kunze94@gmail.com>
@faddat faddat mentioned this pull request Feb 28, 2022
8 tasks
JimLarson pushed a commit to agoric-labs/cosmos-sdk that referenced this pull request Jul 7, 2022
…e (backport cosmos#10022) (cosmos#10847)

* perf!: Add HasAccount to the AuthKeeper to save protobuf decoding time (cosmos#10022)

* Add HasAccount to the AuthKeeper to save protobuf decoding time

We found in the Osmosis epoch time, the many accesses to GetAccount's proto unmarshalling was a significant slowdown.
This adds a HasAccount method to the AuthKeeper, and fixes one unnecessary spot that it appears within in SendCoins

* Update Spec

* Add Changelog entry

* Fix lint & use speedup in SendCoins

* Update x/auth/keeper/account.go

Co-authored-by: Federico Kunze Küllmer <31522760+fedekunze@users.noreply.github.com>

Co-authored-by: Federico Kunze Küllmer <31522760+fedekunze@users.noreply.github.com>
(cherry picked from commit 7273bd3)

# Conflicts:
#	CHANGELOG.md

* conflicts

* changelog

* changelog

Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com>
Co-authored-by: Federico Kunze Küllmer <federico.kunze94@gmail.com>
JeancarloBarrios pushed a commit to agoric-labs/cosmos-sdk that referenced this pull request Sep 28, 2024
…e (backport cosmos#10022) (cosmos#10847)

* perf!: Add HasAccount to the AuthKeeper to save protobuf decoding time (cosmos#10022)

* Add HasAccount to the AuthKeeper to save protobuf decoding time

We found in the Osmosis epoch time, the many accesses to GetAccount's proto unmarshalling was a significant slowdown.
This adds a HasAccount method to the AuthKeeper, and fixes one unnecessary spot that it appears within in SendCoins

* Update Spec

* Add Changelog entry

* Fix lint & use speedup in SendCoins

* Update x/auth/keeper/account.go

Co-authored-by: Federico Kunze Küllmer <31522760+fedekunze@users.noreply.github.com>

Co-authored-by: Federico Kunze Küllmer <31522760+fedekunze@users.noreply.github.com>
(cherry picked from commit 7273bd3)

# Conflicts:
#	CHANGELOG.md

* conflicts

* changelog

* changelog

Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com>
Co-authored-by: Federico Kunze Küllmer <federico.kunze94@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:automerge Automatically merge PR once all prerequisites pass. C:x/auth C:x/bank T: Performance Performance improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants