Skip to content

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

Closed
mergify[bot] wants to merge 2 commits intov1.18from
mergify/bp/v1.18/pr-29
Closed

v1.18: Combine builtin and BPF compute cost in cost model (backport of #29)#271
mergify[bot] wants to merge 2 commits intov1.18from
mergify/bp/v1.18/pr-29

Conversation

@mergify
Copy link
Copy Markdown

@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)
@codecov-commenter
Copy link
Copy Markdown

Codecov Report

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

Project coverage is 81.6%. Comparing base (04356e7) to head (1786258).

Additional details and impacted files
@@            Coverage Diff            @@
##            v1.18     #271     +/-   ##
=========================================
+ Coverage    80.6%    81.6%   +1.0%     
=========================================
  Files         827      827             
  Lines      224496   224486     -10     
=========================================
+ Hits       181015   183264   +2249     
+ Misses      43481    41222   -2259     

@tao-stones tao-stones requested review from apfitzge and t-nelson March 16, 2024 01:40
Copy link
Copy Markdown

@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.

change lgtm, same as the change on master branch. diff for backport is a bit large but I'm okay with it, as much of it is simply renames or simple combination of 2 fields into a single one.

Please still get approval from @t-nelson, as I was the reviewer on original PR.

@tao-stones tao-stones requested a review from a team as a code owner May 30, 2024 14:38
@willhickey
Copy link
Copy Markdown

I'm hesitant to backport to v1.18 right now, since we're planning to recommend it for mainnet-beta in the next couple of weeks.

Do you know how much block space gets wasted, on average, by the current implementation?

@tao-stones
Copy link
Copy Markdown

The double counted built-ins CUs waste about 10-14% block space (last time I checked). Mango dashboard visualizes it nicely: the unfilled space in BLOCK FILL RATIO column can largely be contributed to this. Probably also worth pointing out: the more votes (turquoise portion in BLOCK CONTENT column), the higher un-filled ratio.

I'd like to see this being deployed to mainnet-beta, but I'd also agree it is not as critical. Happy to close this it if deemed so.

@tao-stones
Copy link
Copy Markdown

Not going to bp to 1.18

@tao-stones tao-stones closed this Jun 5, 2024
@mergify mergify Bot deleted the mergify/bp/v1.18/pr-29 branch June 5, 2024 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants