Fix reserve minimal compute units for builtins #3799

tao-stones · 2024-11-26T16:47:01Z

Problem

Implementing solana-foundation/solana-improvement-documents#170 by defining MAX_BUILTIN_ALLOCATION_COMPUTE_UNIT_LIMIT to 3K CUs, then use it to allocate builtin instructions' CU Meters for VM and cost tracking for leaders.

Summary of Changes

When calculates default tx cu limits, use MAX_BUILTIN_ALLOCATION_COMPUTE_UNIT_LIMIT per builtin instruction, including compute-budget program instructions.
Cost model reads cu limits from RuntimeTransaction's static_meta, replacing a localized implementation that isn't consistent with compute-budget.
Changes are behind Feature gate
updated existing tests to allow additional feature_set parameters (touch many files)

Feature Gate Issue: #2562

runtime-transaction/src/compute_budget_instruction_details.rs

runtime-transaction/Cargo.toml

core/src/banking_stage/immutable_deserialized_packet.rs

jstarry · 2024-12-02T08:18:52Z

runtime-transaction/src/compute_budget_instruction_details.rs

-                    compute_budget_instruction_details.num_non_compute_budget_instructions,
-                    1
-                );
+            match filter.get_program_kind(instruction.program_id_index as usize, program_id) {


We really don't need to do all this builtin program accounting unless the compute limit isn't specified by the transaction. How about we move this all into calculate_default_compute_unit_limit and re-iterate through tx instructions there?

If you agree with moving this to calculate_default_compute_unit_limit, I've implemented the pre-requisite PR (#3853) to unblock adding an instructions iterator param to sanitize_and_convert_to_compute_budget_limits

I'm leaning towards this instead of storing a Vec. Re-iterating is likely less costly than allocation, and we can remove the re-iteration once the feature is activated.

We really don't need to do all this builtin program accounting unless the compute limit isn't specified by the transaction.

This is great observation. Another possibility is to re-iterate in try_from when necessary. Something like this:

struct builtin_instruction_details { num_builtin_instructions: 0, num_non_builtin_instructions: 0, migrating_builtin: vec![0; MIGRATION_FEATURES_ID.len()], } fn try_from( ixs: iter ) -> Result<Self> { // original iteration: // to count `num_compute_budget_instructions` and process_compute_budget_instruction if present // possible second iter if requested_compute_unit_limit.is_none() { // iterate again to create "builtin_instruction_details" } }

Also, based on benchmarking, the cost of allocating vec with fixed size of 3 is much lower than the cost of hashing Pubkey to lookup at BUILTIN_INSTRUCTION_COSTS. Re-iterate within try_from will reuse cached ComputeBudgetProgramIdFilter, avoiding re-hashing.

wdyt something like this: tao-stones@86fcaeb (edited)

wdyt something like this: tao-stones@86fcaeb (edited)

That does fix the issue where we were unnecessarily calculating builtin counts even when the compute unit limit was specified by the transaction. But I think this implementation is too convoluted. I prefer the code to be simpler, especially if we are planning to backport this change.

Also, based on benchmarking, the cost of allocating vec with fixed size of 3 is much lower than the cost of hashing Pubkey to lookup at BUILTIN_INSTRUCTION_COSTS. Re-iterate within try_from will reuse cached ComputeBudgetProgramIdFilter, avoiding re-hashing.

I don't think we can purely rely on benchmarks for making this decision. The extra allocation is far more more expensive while the validator is running than in an isolated benchmark.

That does fix the issue where we were unnecessarily calculating builtin counts even when the compute unit limit was specified by the transaction. But I think this implementation is too convoluted. I prefer the code to be simpler, especially if we are planning to backport this change.

One thing that didn't make sense about your commit is that we are still calculating num_non_compute_budget_instructions. Similar to the builtin_instruction_details field you added, it only needs to be calculated if the tx didn't specify an explicit compute unit limit.

just a note that num_non_compute_budget_instructions is byproduct of first iteration when checking is_compute_budget_program(), so in case cu-limit is not requested and feature is not activated (eg. current behavior), it's ready to calc default cu limit by num_non_compute_budget_instructions * 200K.

jstarry · 2024-12-02T08:20:34Z

runtime-transaction/src/compute_budget_instruction_details.rs

+        if feature_set.is_active(&feature_set::reserve_minimal_cus_for_builtin_instructions::id()) {
+            // evaluate MigratingBuiltin
+            let (additional_num_non_builtin_instructions, additional_num_builtin_instructions) =
+                self.migrating_builtin.iter().enumerate().fold(


I personally would prefer that we re-iterate over instructions to count builtins rather than increasing the amount of memory used per transaction with all of the new accounting related fields you've added to ComputeBudgetInstructionDetails

At one point, we found iteration is relatively costly, so opt to trade memory (additional counters) with single-iteration. Now with #3853 merged, and need of handling migration (now or in near future), I can see re-iter transaction's instructions here (after checked cu-limit not requested, and reserve_minimal_cus_for_builtin_instructions is active) makes sense. Looks like @apfitzge is also onboard. I'll prep a PR for that.

Think it depends on how we store the additional mem. If an extra u16 or 2 per transaction - that may be worth avoiding re-iteration, but we should probably measure. I think as features get activated we may be able to remove both the additional mem or re-iteration.

#3899 is the re-iteration version of implementation, it does not support builtin migration, but I don't think adding that will be a big lift. Need to have tests updated to supply iter to sanitize_and_convert_to_compute_budget_limits(), after that I'll put a separate commit for migration to compare.

jstarry · 2024-12-03T13:51:11Z

I'm now thinking it was a mistake to have this SIMD-0170 implementation PR also handle the builtin program migration case. How about we push that off for now since those migration feature gates aren't scheduled yet? Then this PR can just focus on making the cost tracker and compute limit agree on 3k cu's per builtin. The extra allocation for builtin migration tracking can be figured out later since we don't need to backport that part.

tao-stones

@jstarry @apfitzge cleaned up the PR, touched a lot of file mostly due to adding feature_set or add Clone to iterator. Can you take another look?

tao-stones · 2024-12-04T04:51:05Z

runtime-transaction/src/compute_budget_instruction_details.rs

@@ -44,10 +50,36 @@ impl ComputeBudgetInstructionDetails {
            }
        }



Only when Compute Unite Limit is not requested to re-iterate instructions to collect builtin program counts. This bit of optimization is because is_compute_budget_program() is cheap, get_program_kind() on the other hand does additional HashMap lookup by Pubkey; Saving unnecessary calls to get_program_kind() perhaps justifies additional iteration.

runtime-transaction/src/compute_budget_program_id_filter.rs

jstarry

I'm ok with the PR as is but added some nits

runtime-transaction/src/compute_budget_program_id_filter.rs

jstarry · 2024-12-04T07:13:57Z

runtime-transaction/src/compute_budget_program_id_filter.rs

        solana_sdk::compute_budget::check_id(program_id)
    }
+
+    #[inline]


nit: shouldn't we only be using inline for one-liners for the most part?

jstarry · 2024-12-04T07:20:57Z

runtime-transaction/src/compute_budget_program_id_filter.rs

+
+        if is_builtin_program(program_id) {
+            ProgramKind::Builtin {
+                is_compute_budget: solana_sdk::compute_budget::check_id(program_id),


nit: technically we would know that this isn't the compute budget program already because otherwise the filter would have had a compute budget builtin entry that was populated by the first filter pass

- Implement SIMD-170;

Co-authored-by: Justin Starry <[email protected]>

apfitzge · 2024-12-04T19:13:27Z

core/src/banking_stage/consumer.rs

        let fee_budget_limits = FeeBudgetLimits::from(process_compute_budget_instructions(
            message.program_instructions_iter(),
+            &bank.feature_set,


Split out #3922 - we can use cached value here.

Will wait on merge/review of that until this PR is merged.

cost-model/src/cost_model.rs

apfitzge

lgtm - please wait on @jstarry's confirmation as well

pgarg66

Approving for @anza-xyz/svm

mergify · 2024-12-05T03:50:36Z

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.

mergify · 2024-12-05T03:50:38Z

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.

- Add feature gate, issue #2562; - Implement SIMD-170; --------- Co-authored-by: Justin Starry <[email protected]> (cherry picked from commit 3e9af14) # Conflicts: # builtins-default-costs/src/lib.rs # compute-budget/src/compute_budget_limits.rs # compute-budget/src/compute_budget_processor.rs # core/src/banking_stage/consumer.rs # core/src/banking_stage/immutable_deserialized_packet.rs # core/src/banking_stage/transaction_scheduler/receive_and_buffer.rs # cost-model/src/cost_model.rs # cost-model/src/transaction_cost.rs # programs/compute-budget-bench/benches/compute_budget.rs # programs/sbf/tests/programs.rs # runtime-transaction/benches/process_compute_budget_instructions.rs # runtime-transaction/src/compute_budget_instruction_details.rs # runtime-transaction/src/compute_budget_program_id_filter.rs # runtime-transaction/src/lib.rs # runtime-transaction/src/runtime_transaction.rs # runtime-transaction/src/runtime_transaction/sdk_transactions.rs # runtime/src/bank.rs # runtime/src/bank/tests.rs # runtime/src/prioritization_fee_cache.rs # sdk/src/feature_set.rs # svm-transaction/src/svm_message.rs # svm-transaction/src/svm_message/sanitized_message.rs # svm-transaction/src/svm_message/sanitized_transaction.rs # svm/src/transaction_processor.rs # transaction-view/src/resolved_transaction_view.rs # transaction-view/src/transaction_view.rs

- Add feature gate, issue #2562; - Implement SIMD-170; --------- Co-authored-by: Justin Starry <[email protected]> (cherry picked from commit 3e9af14) # Conflicts: # builtins-default-costs/src/lib.rs # core/src/banking_stage/consumer.rs # core/src/banking_stage/immutable_deserialized_packet.rs # core/src/banking_stage/transaction_scheduler/receive_and_buffer.rs # cost-model/src/cost_model.rs # programs/compute-budget-bench/benches/compute_budget.rs # runtime-transaction/src/lib.rs # runtime-transaction/src/runtime_transaction/sdk_transactions.rs # runtime/src/prioritization_fee_cache.rs

t-nelson · 2024-12-11T17:26:45Z

chat, please merge the SIMDs before the implementations

- Add feature gate, issue #2562; - Implement SIMD-170; --------- Co-authored-by: Justin Starry <[email protected]> (cherry picked from commit 3e9af14) # Conflicts: # builtins-default-costs/src/lib.rs # core/src/banking_stage/consumer.rs # core/src/banking_stage/immutable_deserialized_packet.rs # core/src/banking_stage/transaction_scheduler/receive_and_buffer.rs # cost-model/src/cost_model.rs # programs/compute-budget-bench/benches/compute_budget.rs # runtime-transaction/src/lib.rs # runtime-transaction/src/runtime_transaction/sdk_transactions.rs # runtime/src/prioritization_fee_cache.rs

tao-stones added the feature-gate Pull Request adds or modifies a runtime feature gate label Nov 26, 2024

tao-stones force-pushed the fix-reserve-minimal-cus-for-builtins-less-api-change branch from e416a8f to 6bbd650 Compare November 26, 2024 18:27

apfitzge reviewed Nov 26, 2024

View reviewed changes

runtime-transaction/src/compute_budget_instruction_details.rs Outdated Show resolved Hide resolved

apfitzge reviewed Nov 26, 2024

View reviewed changes

runtime-transaction/src/compute_budget_instruction_details.rs Outdated Show resolved Hide resolved

tao-stones mentioned this pull request Nov 27, 2024

Fix reserve minimal cus for builtins #3755

Closed

tao-stones force-pushed the fix-reserve-minimal-cus-for-builtins-less-api-change branch 2 times, most recently from 77381a1 to 8faee07 Compare November 27, 2024 20:18

tao-stones marked this pull request as ready for review November 27, 2024 20:18

tao-stones requested a review from a team as a code owner November 27, 2024 20:18

tao-stones requested review from jstarry and buffalojoec November 27, 2024 20:19

tao-stones force-pushed the fix-reserve-minimal-cus-for-builtins-less-api-change branch from 8faee07 to c181481 Compare November 27, 2024 20:20

topointon-jump mentioned this pull request Nov 27, 2024

[WIP] SIMD-170: Reserve lower default CU limits for builtin instructions firedancer-io/firedancer#3570

Open

4 tasks

pgarg66 reviewed Nov 27, 2024

View reviewed changes

runtime-transaction/Cargo.toml Outdated Show resolved Hide resolved

tao-stones force-pushed the fix-reserve-minimal-cus-for-builtins-less-api-change branch 4 times, most recently from 0e8a348 to 9192989 Compare December 1, 2024 15:27

jstarry reviewed Dec 2, 2024

View reviewed changes

tao-stones force-pushed the fix-reserve-minimal-cus-for-builtins-less-api-change branch from 9192989 to 96da7c6 Compare December 2, 2024 15:10

tao-stones force-pushed the fix-reserve-minimal-cus-for-builtins-less-api-change branch from 96da7c6 to c2f7271 Compare December 4, 2024 04:43

tao-stones requested review from apfitzge and jstarry December 4, 2024 04:43

tao-stones commented Dec 4, 2024

View reviewed changes

tao-stones force-pushed the fix-reserve-minimal-cus-for-builtins-less-api-change branch from c2f7271 to 7251345 Compare December 4, 2024 05:27

jstarry previously approved these changes Dec 4, 2024

View reviewed changes

tao-stones dismissed jstarry’s stale review via 0a9f3a2 December 4, 2024 14:21

tao-stones and others added 2 commits December 4, 2024 10:33

- Add feature gate, issue solana-labs#2562;

844a2d3

- Implement SIMD-170;

Update runtime-transaction/src/compute_budget_program_id_filter.rs

17e84a8

Co-authored-by: Justin Starry <[email protected]>

split filters for cleaner code, adjustred inline-ness

2c7a7cf

tao-stones force-pushed the fix-reserve-minimal-cus-for-builtins-less-api-change branch from 36a2346 to 2c7a7cf Compare December 4, 2024 16:34

apfitzge reviewed Dec 4, 2024

View reviewed changes

cost-model/src/cost_model.rs Show resolved Hide resolved

apfitzge approved these changes Dec 4, 2024

View reviewed changes

pgarg66 approved these changes Dec 4, 2024

View reviewed changes

jstarry approved these changes Dec 5, 2024

View reviewed changes

jstarry mentioned this pull request Dec 5, 2024

[secp256r1] Add CU costs #3826

Merged

tao-stones merged commit 3e9af14 into anza-xyz:master Dec 5, 2024
50 checks passed

tao-stones added v2.0 Backport to v2.0 branch v2.1 Backport to v2.1 branch labels Dec 5, 2024

mergify bot mentioned this pull request Dec 5, 2024

v2.0: Fix reserve minimal compute units for builtins (backport of #3799) #3930

Open

mergify bot mentioned this pull request Dec 5, 2024

v2.1: Fix reserve minimal compute units for builtins (backport of #3799) #3931

Open

tao-stones deleted the fix-reserve-minimal-cus-for-builtins-less-api-change branch December 5, 2024 15:06

mergify bot mentioned this pull request Dec 12, 2024

v2.1: Accounting migrating builtin programs default Compute Unit Limit with feature status (backport of #3975) #4091

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix reserve minimal compute units for builtins #3799

Fix reserve minimal compute units for builtins #3799

tao-stones commented Nov 26, 2024 •

edited

Loading

jstarry Dec 2, 2024

jstarry Dec 2, 2024

apfitzge Dec 2, 2024

tao-stones Dec 2, 2024

tao-stones Dec 2, 2024

tao-stones Dec 2, 2024 •

edited

Loading

jstarry Dec 3, 2024

jstarry Dec 3, 2024

tao-stones Dec 3, 2024

jstarry Dec 2, 2024

tao-stones Dec 3, 2024

apfitzge Dec 3, 2024

tao-stones Dec 3, 2024

jstarry commented Dec 3, 2024

tao-stones left a comment

tao-stones Dec 4, 2024

jstarry left a comment

jstarry Dec 4, 2024

jstarry Dec 4, 2024

apfitzge Dec 4, 2024

apfitzge left a comment

pgarg66 left a comment

mergify bot commented Dec 5, 2024

mergify bot commented Dec 5, 2024

t-nelson commented Dec 11, 2024

		@@ -44,10 +50,36 @@ impl ComputeBudgetInstructionDetails {
		}
		}

Fix reserve minimal compute units for builtins #3799

Fix reserve minimal compute units for builtins #3799

Conversation

tao-stones commented Nov 26, 2024 • edited Loading

Problem

Summary of Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tao-stones Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jstarry commented Dec 3, 2024

tao-stones left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jstarry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apfitzge left a comment

Choose a reason for hiding this comment

pgarg66 left a comment

Choose a reason for hiding this comment

mergify bot commented Dec 5, 2024

mergify bot commented Dec 5, 2024

t-nelson commented Dec 11, 2024

tao-stones commented Nov 26, 2024 •

edited

Loading

tao-stones Dec 2, 2024 •

edited

Loading