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

feat: cpu and store gas updates #3054

Merged
merged 8 commits into from
Dec 17, 2024
Merged

feat: cpu and store gas updates #3054

merged 8 commits into from
Dec 17, 2024

Conversation

piux2
Copy link
Contributor

@piux2 piux2 commented Oct 31, 2024

Summary:

This PR updates the CPU and Store gas based on results from the benchmarking tool:#2241

For CPU gas, the measurement is in nanoseconds per opcode execution.
For storage gas, the measurement is in nanoseconds per byte for each type of Gno store access.

Changes:

We moved the gas meter from the underlying store to the upper Gno store to capture accurate resource consumption for VM transactions. At the same time, we retain the original gas store and gas meter for the Auth Keeper to handle regular blockchain transactions that do not necessarily involve the VM.

We also updated the gas-wanted in the integration test to reflect actual gas usage. This can serve as a flag to alert us to future changes that might increase gas assumptions.

Additional reasons for these changes include:

  • The Gno VM store processes additional complex encoding and decoding of data structures, rather than simply reading and writing bytes to disk.
  • For the above reason, we benchmarked gas for store access at the Gno store level.
  • We want to avoid consuming gas at two points for a single store access during a VM transaction.

Here are the diagrams to explain the store access gas before and after changes

Before:

image

After:

image

Contributors' checklist...
  • Added new tests
  • Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory
  • Updated the official documentation or not needed
  • No breaking changes were made, or a BREAKING CHANGE: xxx message was included in the description
  • Added references to related issues and PRs
  • Provided any useful hints for running manual tests

@github-actions github-actions bot added 📦 🤖 gnovm Issues or PRs gnovm related 📦 🌐 tendermint v2 Issues or PRs tm2 related 📦 ⛰️ gno.land Issues or PRs gno.land package related labels Oct 31, 2024
gno.land/pkg/sdk/vm/gas_test.go Outdated Show resolved Hide resolved
gno.land/cmd/gnoland/testdata/addpkg_namespace.txtar Outdated Show resolved Hide resolved
gnovm/pkg/gnolang/machine.go Show resolved Hide resolved
gno.land/pkg/sdk/vm/keeper.go Outdated Show resolved Hide resolved
gnovm/pkg/gnolang/store.go Outdated Show resolved Hide resolved
gnovm/pkg/gnolang/store.go Show resolved Hide resolved
tm2/pkg/sdk/context.go Show resolved Hide resolved
@Kouteki Kouteki added the in focus Core team is prioritizing this work label Nov 1, 2024
@Kouteki Kouteki added this to the 🚀 Mainnet launch milestone Nov 11, 2024
@Kouteki Kouteki mentioned this pull request Nov 19, 2024
Copy link
Member

@zivkovicmilos zivkovicmilos left a comment

Choose a reason for hiding this comment

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

I think the PR looks fine, apart from the comments left by @thehowl, which should be addressed about the global gas meter setting in the VM keeper 🙏

I'm temporarily just putting a pause on the PR, to allow for us to discuss whether these new gas values make sense, since they are a huge change. I also want to see some more reasoning and gas comparisons, apart from #2241

If we're fine with the bumps, I am also cool with the PR moving forward as is ✅

cc @moul @notJoon

gnovm/pkg/gnolang/machine.go Show resolved Hide resolved
@Kouteki Kouteki requested a review from moul November 20, 2024 11:37
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.

Overall, it looks good. In this PR or a future one, I suggest we:

  • Add a global multiplier/divider parameter to avoid using ngnot while keeping the numbers relatively readable.
  • Create a benchmarking tool that runs all the gnocode in the repository and generates a CSV or TXT file with the gas details we commit periodically (there's no need for this on every commit, in my opinion). This way, whenever we want to fine-tune a parameter, we can run the same script again and compare the results.

Copy link
Member

@zivkovicmilos zivkovicmilos left a comment

Choose a reason for hiding this comment

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

Unblocking after @moul is fine with the bumps

@Gno2D2
Copy link
Collaborator

Gno2D2 commented Dec 6, 2024

🛠 PR Checks Summary

All Automated Checks passed. ✅

Manual Checks (for Reviewers):
  • SKIP: Do not block the CI for this PR
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)
🟢 The pull request head branch must be up-to-date with its base (more info)

☑️ 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
└── 🟢 The pull request was created from a fork (head branch repo: piux2/gno)

Then

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

The pull request head branch must be up-to-date with its base (more info)

If

🟢 Condition met
└── 🟢 On every pull request

Then

🟢 Requirement satisfied
└── 🟢 Head branch (piux2:feat_gas_ops) is up to date with base (master): behind by 0 / ahead by 8

Manual Checks
**SKIP**: Do not block the CI for this PR

If

🟢 Condition met
└── 🟢 On every pull request

Can be checked by

  • Any user with comment edit permission

Copy link

codecov bot commented Dec 6, 2024

Codecov Report

Attention: Patch coverage is 80.32787% with 12 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
gno.land/pkg/keyscli/addpkg.go 0.00% 4 Missing and 2 partials ⚠️
gnovm/pkg/gnolang/store.go 87.50% 4 Missing and 1 partial ⚠️
tm2/pkg/sdk/auth/keeper.go 80.00% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

@piux2 piux2 merged commit 9f181d1 into gnolang:master Dec 17, 2024
105 of 106 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in focus Core team is prioritizing this work 📦 🌐 tendermint v2 Issues or PRs tm2 related 📦 ⛰️ gno.land Issues or PRs gno.land package related 📦 🤖 gnovm Issues or PRs gnovm related
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants