Skip to content

perf(crypto): simplify and speed up Address.Compare #4130

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

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

Conversation

odeke-em
Copy link
Contributor

@odeke-em odeke-em commented Apr 13, 2025

Takes advantage of knowledge that Address is simply an array derived from []byte and Go's rules allow us to address each with [:] to find []byte and pass those into bytes.Compare instead of the unnecessarily inefficient make, copy. crypto.Address is very fundamental and a building block to Gno and all of cryptocurrencies, thus making its operations much more efficient is necessary.
While here also made Address.Compare take a pointer receiver which reduces the need to make a stack copy of the receiver and with that we have this speed up while making the code more succinct and direct and more idiomatic

$ benchstat before.txt after.txt
name              old time/op    new time/op    delta
AddressCompare-8    79.5ns ± 1%    33.3ns ± 8%  -58.17%  (p=0.000 n=10+9)

name              old alloc/op   new alloc/op   delta
AddressCompare-8     8.00B ± 0%     8.00B ± 0%     ~     (all equal)

name              old allocs/op  new allocs/op  delta
AddressCompare-8      1.00 ± 0%      1.00 ± 0%     ~     (all equal)

Fixes #4129

@github-actions github-actions bot added the 📦 🌐 tendermint v2 Issues or PRs tm2 related label Apr 13, 2025
@Gno2D2 Gno2D2 requested a review from a team April 13, 2025 23:25
@Gno2D2 Gno2D2 added the review/triage-pending PRs opened by external contributors that are waiting for the 1st review label Apr 13, 2025
@Gno2D2
Copy link
Collaborator

Gno2D2 commented Apr 13, 2025

🛠 PR Checks Summary

All Automated Checks passed. ✅

Manual Checks (for Reviewers):
  • IGNORE the bot requirements for this PR (force green CI check)
  • The pull request description provides enough details (checked by @thehowl)
Read More

🤖 This bot helps streamline PR reviews by verifying automated checks and providing guidance for contributors and reviewers.

✅ Automated Checks (for Contributors):

🟢 Maintainers must be able to edit this pull request (more info)
🟢 Pending initial approval by a review team member, or review from tech-staff

☑️ Contributor Actions:
  1. Fix any issues flagged by automated checks.
  2. Follow the Contributor Checklist to ensure your PR is ready for review.
    • Add new tests, or document why they are unnecessary.
    • Provide clear examples/screenshots, if necessary.
    • Update documentation, if required.
    • Ensure no breaking changes, or include BREAKING CHANGE notes.
    • Link related issues/PRs, where applicable.
☑️ Reviewer Actions:
  1. Complete manual checks for the PR, including the guidelines and additional checks if applicable.
📚 Resources:
Debug
Automated Checks
Maintainers must be able to edit this pull request (more info)

If

🟢 Condition met
└── 🟢 And
    ├── 🟢 The base branch matches this pattern: ^master$
    └── 🟢 The pull request was created from a fork (head branch repo: odeke-em/gno)

Then

🟢 Requirement satisfied
└── 🟢 Maintainer can modify this pull request

Pending initial approval by a review team member, or review from tech-staff

If

🟢 Condition met
└── 🟢 And
    ├── 🟢 The base branch matches this pattern: ^master$
    └── 🟢 Not (🔴 Pull request author is a member of the team: tech-staff)

Then

🟢 Requirement satisfied
└── 🟢 If
    ├── 🟢 Condition
    │   └── 🟢 Or
    │       ├── 🟢 At least 1 user(s) of the organization reviewed the pull request (with state "APPROVED")
    │       ├── 🟢 At least 1 user(s) of the team tech-staff reviewed pull request
    │       └── 🔴 This pull request is a draft
    └── 🟢 Then
        └── 🟢 And
            ├── 🟢 Not (🔴 This label is applied to pull request: review/triage-pending)
            └── 🟢 At least 1 user(s) of the team tech-staff reviewed pull request

Manual Checks
**IGNORE** the bot requirements for this PR (force green CI check)

If

🟢 Condition met
└── 🟢 On every pull request

Can be checked by

  • Any user with comment edit permission
The pull request description provides enough details

If

🟢 Condition met
└── 🟢 And
    ├── 🟢 Not (🔴 Pull request author is a member of the team: core-contributors)
    └── 🟢 Not (🔴 Pull request author is user: dependabot[bot])

Can be checked by

  • team core-contributors

@odeke-em odeke-em force-pushed the crypto-simplify+speed-up-Address.Compare branch 4 times, most recently from fcd6916 to 6e68708 Compare April 13, 2025 23:45
@github-actions github-actions bot added the 📦 ⛰️ gno.land Issues or PRs gno.land package related label Apr 13, 2025
@odeke-em odeke-em force-pushed the crypto-simplify+speed-up-Address.Compare branch from 6e68708 to 6b5b5ec Compare April 13, 2025 23:49
Copy link

codecov bot commented Apr 13, 2025

Codecov Report

Attention: Patch coverage is 87.50000% with 2 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
contribs/gnodev/pkg/address/book.go 0.00% 2 Missing ⚠️

📢 Thoughts on this report? Let us know!

Copy link
Member

@notJoon notJoon left a comment

Choose a reason for hiding this comment

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

I can see that reduced memory copying and unnecessary repeated calls. It looks reasonable to me. approved 👍

remove: review/triage-pending flag

@notJoon notJoon removed the review/triage-pending PRs opened by external contributors that are waiting for the 1st review label Apr 14, 2025
Takes advantage of knowledge that Address is simply an
array derived from []byte and Go's rules allow us to address
each with [:] to find []byte and pass those into bytes.Compare
instead of the unnecessarily inefficient make, copy.
crypto.Address is very fundamental and a building block to Gno
and all of cryptocurrencies, thus making its operations much more
efficient is necessary.
While here also made Address.Compare take a pointer receiver which
reduces the need to make a stack copy of the receiver and with
that we have this speed up while making the code more succinct
and direct and more idiomatic

```shell
$ benchstat before.txt after.txt
name              old time/op    new time/op    delta
AddressCompare-8    79.5ns ± 1%    52.9ns ± 3%  -33.54%  (p=0.000 n=10+9)

name              old alloc/op   new alloc/op   delta
AddressCompare-8     8.00B ± 0%     8.00B ± 0%     ~     (all equal)

name              old allocs/op  new allocs/op  delta
AddressCompare-8      1.00 ± 0%      1.00 ± 0%     ~     (all equal)
```

Fixes gnolang#4129
The updated benchmark then becomes

```shell
name              old time/op    new time/op    delta
AddressCompare-8    79.5ns ± 1%    33.3ns ± 8%  -58.17%  (p=0.000 n=10+9)

name              old alloc/op   new alloc/op   delta
AddressCompare-8     8.00B ± 0%     8.00B ± 0%     ~     (all equal)

name              old allocs/op  new allocs/op  delta
AddressCompare-8      1.00 ± 0%      1.00 ± 0%     ~     (all equal)
```
@odeke-em odeke-em force-pushed the crypto-simplify+speed-up-Address.Compare branch from 54af354 to 47e1195 Compare April 14, 2025 14:05
@odeke-em odeke-em requested review from thehowl April 14, 2025 14:16
@thehowl
Copy link
Member

thehowl commented Apr 14, 2025

I don't know if this matters enough; it seems to be used in some functions in Validator/ValidatorSet to matter enough. I'll let someone else take a second look first.

@moul
Copy link
Member

moul commented Apr 15, 2025

I'm also skeptical about how much it matters. We should have not only "per function benchmarks" but also flame graphs that show how frequently calls are made in real usage.

These flame graphs could become shared assets rather than something we expect with each pull request.

I'll open an issue to explore if someone has a good idea for generating flame graphs in the CI. We don't need a history; ideally, we just want a few generated flame graphs that match a recent version of the code or a pull request for convenience.

@moul
Copy link
Member

moul commented Apr 15, 2025

-> #4151

Copy link
Member

@moul moul left a comment

Choose a reason for hiding this comment

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

Let's block this PR until we can compare a before-and-after using a usage flamegraph or another method that provides real-world usage data, not just per-function metrics.

@odeke-em
Copy link
Contributor Author

One thing though to objectively acknowledge and note is that the prior code was really extraneous and my change isn't even an "optimization" but really how the code should have been written for idiomatic Go the proper way, it just happens to make the code much more efficient and faster.

Skepticism over performance is usually more pertinent when one is being clever and the performance checks will later show, Address comparison is pretty important and quite heavily used.

@moul
Copy link
Member

moul commented Apr 15, 2025

Let's prove it.

@Kouteki Kouteki moved this from Triage to In Review in 🧙‍♂️gno.land core team Apr 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 🌐 tendermint v2 Issues or PRs tm2 related 📦 ⛰️ gno.land Issues or PRs gno.land package related
Projects
Status: In Progress
Status: In Review
Development

Successfully merging this pull request may close these issues.

tm2/pkg/crypto: Address.Compare needlessly allocates
5 participants