Use consistent naming for compute unit limit #25229
Conversation
jstarry
force-pushed
the
rename-compute-unit-limit
branch
from
9dbf4d7 to
5782a52
Compare
| } | ||
|
|
||
| /// average cost of all recorded programs | ||
| pub fn get_global_average_units(&self) -> u64 { | ||
| pub fn get_global_average_program_cost(&self) -> u64 { |
There was a problem hiding this comment.
just a side note, early it was changed from cost to units for clarity, terms are more consistent since then, program_cost might be more intuitive now. Thanks for making change.
| @@ -369,12 +369,13 @@ fn main() { | |||
| .help("deactivate this feature in genesis.") | |||
| ) | |||
| .arg( | |||
| Arg::with_name("max_compute_units") | |||
| .long("max-compute-units") | |||
| Arg::with_name("max_compute_unit_limit") | |||
There was a problem hiding this comment.
I thought this was overriding the max compute unit limit (1.4M) that a program could use. Felt like max was appropriate here.
There was a problem hiding this comment.
Is there a limit that isn't max?
There was a problem hiding this comment.
I think max_units turned into max_compute_units so that its use here was clear. If we take the new naming we should just keep it as compute_unit_limit
| /// Number of compute units that a transaction or individual instruction is | ||
| /// allowed to consume. Compute units are consumed by program execution, | ||
| /// resources they use, etc... | ||
| pub compute_unit_limit: u64, |
There was a problem hiding this comment.
Don't see the clarity this brings, ComputeBudget::max_units seems pretty clear, ComputeBudget::compute_unit_limit doesn't add much, at least not worth the churn it creates.
There was a problem hiding this comment.
I feel that ComputeBudget::max_units is confusingly named because it's only the max units for a particular tx, not the whole runtime. What churn are you concerned about? Just this PR itself?
There was a problem hiding this comment.
the budget is applied per tx.
Yeah, I was thinking about historic naming and doc churn. Don't feel too strongly opposed to the changes.
|
automerge label removed due to a CI failure |
mergify
bot
removed
the
automerge
* Use consistent naming for compute unit limit * feedback (cherry picked from commit a1522d0) # Conflicts: # runtime/src/cost_model.rs # runtime/src/execute_cost_table.rs # test-validator/src/lib.rs # validator/src/bin/solana-test-validator.rs
* Use consistent naming for compute unit limit (#25229) * Use consistent naming for compute unit limit * feedback (cherry picked from commit a1522d0) # Conflicts: # runtime/src/cost_model.rs # runtime/src/execute_cost_table.rs # test-validator/src/lib.rs # validator/src/bin/solana-test-validator.rs * fix conflicts Co-authored-by: Justin Starry <[email protected]>
* Use consistent naming for compute unit limit * feedback
* Use consistent naming for compute unit limit * feedback
| /// default program cost, set to ComputeBudget::DEFAULT_UNITS | ||
| pub fn get_default_units(&self) -> u64 { | ||
| DEFAULT_UNITS as u64 | ||
| /// default program cost, set to ComputeBudget::DEFAULT_COMPUTE_UNIT_LIMIT |
There was a problem hiding this comment.
There was a problem hiding this comment.
i think this is typo. correct one would be DEFAULT_INSTRUCTION_COMPUTE_UNIT_LIMIT
Problem
RequestUnitsix for example)ComputeBudget::max_unitsis confusingly named because it's only the max units for a particular tx, not the whole runtimeSummary of Changes
Fixes #