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

Combine builtin and BPF compute cost in cost model #29

Conversation

tao-stones
Copy link

@tao-stones tao-stones commented Mar 3, 2024

Problem

feature gate solana-labs#30620 has been activated everywhere, SVM now consumes compute units uniformly for both builtins and BPF instructions. Cost Model should no longer estimate their cost separately.

Original PR: solana-labs#32080

Summary of Changes

  • combine builtin and bpf cost into programs_execution_cost, update metrics reporting as well.
  • update tests

Fixes #

@mergify mergify bot requested a review from a team March 3, 2024 17:17
@tao-stones tao-stones force-pushed the adjust_cost_model_for_builtin_consume_cu branch from 463e136 to abb983c Compare March 4, 2024 16:03
@tao-stones tao-stones requested review from apfitzge and removed request for a team March 4, 2024 17:31
@tao-stones tao-stones marked this pull request as ready for review March 4, 2024 17:48
@tao-stones tao-stones force-pushed the adjust_cost_model_for_builtin_consume_cu branch from fee6db7 to 2c1398a Compare March 4, 2024 22:40
@codecov-commenter
Copy link

codecov-commenter commented Mar 5, 2024

Codecov Report

Attention: Patch coverage is 95.65217% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 81.8%. Comparing base (3f9a7a5) to head (79a31b9).
Report is 8 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##           master      #29     +/-   ##
=========================================
- Coverage    81.8%    81.8%   -0.1%     
=========================================
  Files         837      838      +1     
  Lines      225922   225939     +17     
=========================================
- Hits       184955   184913     -42     
- Misses      40967    41026     +59     

@tao-stones tao-stones requested a review from apfitzge March 5, 2024 15:15
cost-model/src/cost_model.rs Outdated Show resolved Hide resolved
cost-model/src/transaction_cost.rs Show resolved Hide resolved
@tao-stones tao-stones requested a review from apfitzge March 6, 2024 17:15
Copy link

@apfitzge apfitzge left a comment

Choose a reason for hiding this comment

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

lgtm

@tao-stones tao-stones merged commit 8f3f06c into anza-xyz:master Mar 7, 2024
35 checks passed
@tao-stones tao-stones deleted the adjust_cost_model_for_builtin_consume_cu branch March 7, 2024 15:23
Copy link

mergify bot commented Mar 16, 2024

Backports to the stable branch are to be avoided unless absolutely necessary for fixing bugs, security issues, and perf regressions. Changes intended for backport should be structured such that a minimum effective diff can be committed separately from any refactoring, plumbing, cleanup, etc that are not strictly necessary to achieve the goal. Any of the latter should go only into master and ride the normal stabilization schedule.

Copy link

mergify bot commented Mar 16, 2024

Backports to the beta branch are to be avoided unless absolutely necessary for fixing bugs, security issues, and perf regressions. Changes intended for backport should be structured such that a minimum effective diff can be committed separately from any refactoring, plumbing, cleanup, etc that are not strictly necessary to achieve the goal. Any of the latter should go only into master and ride the normal stabilization schedule. Exceptions include CI/metrics changes, CLI improvements and documentation updates on a case by case basis.

mergify bot pushed a commit that referenced this pull request Mar 16, 2024
* Combine builtin and BPF execution cost into programs_execution_cost since VM has started to consume CUs uniformly

* update tests

* apply suggestions from code review

(cherry picked from commit 8f3f06c)
mergify bot pushed a commit that referenced this pull request Mar 16, 2024
* Combine builtin and BPF execution cost into programs_execution_cost since VM has started to consume CUs uniformly

* update tests

* apply suggestions from code review

(cherry picked from commit 8f3f06c)

# Conflicts:
#	core/src/banking_stage/consumer.rs
#	cost-model/src/cost_model.rs
codebender828 pushed a commit to codebender828/agave that referenced this pull request Oct 3, 2024
* Combine builtin and BPF execution cost into programs_execution_cost since VM has started to consume CUs uniformly

* update tests

* apply suggestions from code review
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants