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

v1.17: Combine builtin and BPF compute cost in cost model (backport of #29) #270

Closed
wants to merge 3 commits into from

Conversation

mergify[bot]
Copy link

@mergify mergify bot commented Mar 16, 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 #


This is an automatic backport of pull request #29 done by [Mergify](https://mergify.com).

* 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
Copy link
Author

mergify bot commented Mar 16, 2024

Cherry-pick of 8f3f06c has failed:

On branch mergify/bp/v1.17/pr-29
Your branch is up to date with 'origin/v1.17'.

You are currently cherry-picking commit 8f3f06cc7f.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:
	modified:   core/src/banking_stage.rs
	modified:   core/src/banking_stage/qos_service.rs
	modified:   cost-model/src/cost_tracker.rs
	modified:   cost-model/src/transaction_cost.rs

Unmerged paths:
  (use "git add <file>..." to mark resolution)
	both modified:   core/src/banking_stage/consumer.rs
	both modified:   cost-model/src/cost_model.rs

To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally

@tao-stones tao-stones requested review from t-nelson and apfitzge March 16, 2024 01:41
@tao-stones tao-stones force-pushed the mergify/bp/v1.17/pr-29 branch from a85bb5e to 5152929 Compare March 16, 2024 01:47
@codecov-commenter
Copy link

Codecov Report

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

Project coverage is 81.6%. Comparing base (3809dc0) to head (5152929).

Additional details and impacted files
@@            Coverage Diff            @@
##            v1.17     #270     +/-   ##
=========================================
- Coverage    81.6%    81.6%   -0.1%     
=========================================
  Files         806      806             
  Lines      219203   219193     -10     
=========================================
- Hits       178978   178951     -27     
- Misses      40225    40242     +17     

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 - should fix double counting of builtins and free up some blockspace

@steviez
Copy link

steviez commented Jul 2, 2024

We never got folks on board to BP this to v1.17 but moot point as v1.17 is now EOL with MNB on v1.18 - closing this out

@steviez steviez closed this Jul 2, 2024
@mergify mergify bot deleted the mergify/bp/v1.17/pr-29 branch July 2, 2024 21:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants