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

Update cost model and cost tracker logic when runtime starts to consume CU for all builtins #32080

Conversation

tao-stones
Copy link
Contributor

@tao-stones tao-stones commented Jun 12, 2023

Problem

feature gate native_programs_consume_cu (#30620) changes how builtins consume compute units:

feature gate status builtins consume CU (Y/N) compute_budget_compute_unit_limit applies to
not activated (now) No BPF programs only
activated Yes both BPF and Builtin programs

This change requires cost model and cost tracker logic to be updated accordingly.

Summary of Changes

  1. cost_model: after feature gate is enabled. compute_unit_limit would apply to both builtin and bpf programs in get_transaction_cost().
  2. cost_tracker: when update with "actual consumed CUs", with feature gate enabled, it shall count builtin cost as part of "actual consumed CUs"
  3. add method programs_execution_cost() to TransactionCost to combine builtin and bpf execution costs
  4. updated tests to cover feature gate native_programs_consume_cu status

Note: behind feature gate #30620

@tao-stones tao-stones force-pushed the feature_gate_adjust_cost_model_for_builtin_consume_cu branch from e6ded48 to ec7d167 Compare June 12, 2023 23:21
@codecov
Copy link

codecov bot commented Jun 13, 2023

Codecov Report

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

Project coverage is 81.9%. Comparing base (1bc1012) to head (8841096).
Report is 1865 commits behind head on master.

❗ Current head 8841096 differs from pull request most recent head 31c2018. Consider uploading reports for the commit 31c2018 to get more accurate results

Additional details and impacted files
@@            Coverage Diff            @@
##           master   #32080     +/-   ##
=========================================
- Coverage    82.0%    81.9%   -0.1%     
=========================================
  Files         769      765      -4     
  Lines      208993   208735    -258     
=========================================
- Hits       171390   171112    -278     
- Misses      37603    37623     +20     

@tao-stones tao-stones requested review from apfitzge and steviez June 13, 2023 17:13
core/src/banking_stage/qos_service.rs Show resolved Hide resolved
runtime/src/cost_model.rs Show resolved Hide resolved
assert_eq!(*expected_execution_cost, tx_cost.builtins_execution_cost);
assert_eq!(0, tx_cost.bpf_execution_cost);
assert_eq!(3, tx_cost.data_bytes_cost);
for feature_set in [FeatureSet::default(), FeatureSet::all_enabled()] {
Copy link
Contributor

Choose a reason for hiding this comment

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

is default() all disabled? this seems like we're testing more than 1 feature at a time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All features are disabled by default(). Yea, the point is valid. I did this because it's easy and (looks) clean - it tests two status: all on or all off, but that may have added ambiguity

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah testing features is unfortunately kind of messy, particularly if we've got multiple "in flight" features in a single component such as cost model

…rograms_consume_cu status;

2. TransactionCost to combine builtin and bpf execution cost into programs_execution_cost;
3. enhance tests to cover feature gate native_programs_consume_cu status
@tao-stones tao-stones force-pushed the feature_gate_adjust_cost_model_for_builtin_consume_cu branch from 8841096 to 31c2018 Compare June 20, 2023 22:17
@github-actions github-actions bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Jul 5, 2023
@github-actions github-actions bot closed this Jul 13, 2023
@tao-stones tao-stones reopened this Mar 1, 2024
@willhickey
Copy link
Contributor

This repository is no longer in use. Please re-open this pull request in the agave repo: https://github.com/anza-xyz/agave

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale [bot only] Added to stale content; results in auto-close after a week.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants