cost model: unify builtin lists for core bpf migration tracking #4039

buffalojoec · 2024-12-10T13:02:29Z

Building on #3975, this PR combines the tracking of builtin Core BPF migrations in the cost modeling and other areas of the runtime to use a single list - the BUILTINS list - as the source of truth.

This should enable contributors to avoid footguns when updating Core BPF migration configs on the BUILTINS list and accidentally breaking cost modeling.

tao-stones

Awesome to have the single source of truth here. Can you take a look at #3975, there two BUILTINS arrays were created - one for migrating one for not-migrating. The motivation is each transaction can have a fixed-length array in meta data, avoiding Vec allocation, by

struct MigrationBuiltinFeatureCounter {
    // The vector of counters, matching the size of the static vector MIGRATION_FEATURE_IDS,
    // each counter representing the number of times its corresponding feature ID is
    // referenced in this transaction.
    migrating_builtin: [u16; MIGRATING_BUILTINS_COSTS.len()],
}

Do you think if that's a reasonable approach? If so, could this PR rebase on top of that one?

builtins/src/cost_modeling.rs

buffalojoec · 2024-12-11T10:18:45Z

@tao-stones thanks for taking a look! I refactored this a bit and re-wrote my commits (I had to rebase anyway from some other changes that went in).

With this approach, we can eliminate the footgun of having to update two lists with a new migration feature ID. The only "secondary" area that needs to get updated now is the NUM_COST_MODELED_BUILTINS_WITH_MIGRATIONS in the cost_modeling module. However, I've added a test that will barf if someone forgets to adjust the constant. The feature IDs are always sourced from BUILTINS, which should eliminate the footgun.

tao-stones

Like it very much to have single source for builtins.

builtins/src/cost_modeling.rs

tao-stones · 2024-12-11T16:49:33Z

builtins/src/cost_modeling.rs

+lazy_static! {
+    // This lazy-static list is designed to panic if any builtin migrations
+    // are changed without updating `NUM_COST_MODELED_BUILTINS_WITH_MIGRATIONS`.
+    static ref COST_MODELED_BUILTINS_WITH_MIGRATIONS: [BuiltinWithMigration; NUM_COST_MODELED_BUILTINS_WITH_MIGRATIONS] = {


nit: it validates migrating builtins not exceeds NUM_COST_MODELED_BUILTINS_WITH_MIGRATIONS, but not check if number of migrating builtins reduced (eg. migration completed).

Yes, true, it only checks if a feature gate was added or removed, but it doesn't know about feature status for this list. That's what the querying API is for below this.

tao-stones · 2024-12-11T17:11:48Z

builtins/src/cost_modeling.rs

+pub fn get_migration_feature_index(program_id: &Pubkey) -> Option<usize> {
+    COST_MODELED_BUILTINS_WITH_MIGRATIONS
+        .iter()
+        .position(|builtin| builtin.program_id == *program_id)


this iter, even small, can/want be avoided -- #3975 addresses by explicitly adding position to BuiltinCost. It is probably not easy to do when COST_MODELED_BUILTINS_WITH_MIGRATIONS is generated unrelated to BUILTIN_INSTRUCTION_COSTS (where lookup happens).

I appreciate the simplicity tho, also think once we have customized lookup and retires BUILTIN_INSTRUCTION_COSTS, this iter can be eliminated.

compute-budget-instruction/src/builtin_programs_filter.rs

tao-stones · 2024-12-11T18:09:26Z

builtins/src/cost_modeling.rs

+}
+
+lazy_static! {
+    static ref BUILTIN_INSTRUCTION_COSTS: AHashMap<Pubkey, BuiltinCost> = BUILTINS


a note for future refactors to remove BUILTIN_INSTRUCTION_COSTS: this AHashMap has two functions:

CostModel uses it to determine if program is builtin. It can be done by checking if NotCostModeled

Provides lookup by program_id. Since the list of builtins is small and static, could implement customized lookup instead.

Another note, would like to profile perf between AHashmap::get() and binary_search if BUILTINS is sorted,

Okay, I can profile this. Afaict BUILTINS being sorted shouldn't break anything in the runtime.

buffalojoec · 2024-12-18T05:55:10Z

Hey @tao-stones thanks for the review! I've rebased and updated this PR based on #3975 now that it was merged. Some of the commits have changed to absorb the changes from #3975, so you might have to give this another once-over.

tao-stones

Looks good! Thanks for refreshing it. Fort transparency, @apfitzge initially pointed out optimization opportunity wrt https://github.com/anza-xyz/agave/pull/4039/files#r1880590905 in Master version, which was easier to implement then. I'm good the way it is implemented in this PR, would like @apfitzge weight in.

builtins/src/cost_modeling.rs

tao-stones · 2024-12-18T16:00:47Z

builtins/src/cost_modeling.rs

+}
+
+lazy_static! {
+    static ref BUILTIN_INSTRUCTION_COSTS: AHashMap<Pubkey, BuiltinCost> = BUILTINS


Another note, would like to profile perf between AHashmap::get() and binary_search if BUILTINS is sorted,

Co-authored-by: Tao Zhu <[email protected]>

LucasSte · 2024-12-19T15:19:12Z

builtins/src/lib.rs

@@ -369,6 +403,7 @@ mod tests {
    // Since a macro is used to initialize the test IDs from the `test_only`
    // module, best to ensure the lists have the expected values within a test
    // context.
+    #[cfg(feature = "mock-builtin-migrations")]


As this test is behind a CFG feature, it won't run in the CI. Do we want it to run?

tao-stones reviewed Dec 10, 2024

View reviewed changes

builtins/src/cost_modeling.rs Outdated Show resolved Hide resolved

builtins/src/cost_modeling.rs Show resolved Hide resolved

builtins/src/cost_modeling.rs Outdated Show resolved Hide resolved

buffalojoec force-pushed the unify-builtin-lists-wip branch from 62aefd8 to 1c5c3d7 Compare December 11, 2024 10:16

buffalojoec mentioned this pull request Dec 11, 2024

Accounting migrating builtin programs default Compute Unit Limit with feature status #3975

Merged

tao-stones reviewed Dec 11, 2024

View reviewed changes

buffalojoec added 6 commits December 18, 2024 12:25

builtins: reorder field names

7c62513

builtins: add cost modeling config

d0fc2f8

builtins: port over cost modeling lookups

7ebab93

builtins: add core bpf migration cost modeling

c99f3b1

compute-budget-instruction: integrate builtins

257727a

drop builtins-default-costs crate

8f0c37a

buffalojoec force-pushed the unify-builtin-lists-wip branch from 1c5c3d7 to 8f0c37a Compare December 18, 2024 05:51

buffalojoec changed the title ~~Unify builtin lists wip~~ cost model: unify builtin lists for core bpf migration tracking Dec 18, 2024

tao-stones reviewed Dec 18, 2024

View reviewed changes

Update builtins/src/cost_modeling.rs

20ebc30

Co-authored-by: Tao Zhu <[email protected]>

buffalojoec marked this pull request as ready for review December 19, 2024 08:05

buffalojoec requested a review from a team as a code owner December 19, 2024 08:05

buffalojoec added 2 commits December 19, 2024 17:59

use special feature for mock builtin migrations

d7e816c

builtins: update contributor comments

73825e3

LucasSte reviewed Dec 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cost model: unify builtin lists for core bpf migration tracking #4039

cost model: unify builtin lists for core bpf migration tracking #4039

buffalojoec commented Dec 10, 2024 •

edited

Loading

tao-stones left a comment

buffalojoec commented Dec 11, 2024

tao-stones left a comment

tao-stones Dec 11, 2024

buffalojoec Dec 12, 2024

tao-stones Dec 11, 2024

tao-stones Dec 11, 2024

tao-stones Dec 18, 2024

buffalojoec Dec 18, 2024

buffalojoec commented Dec 18, 2024

tao-stones left a comment

tao-stones Dec 18, 2024

LucasSte Dec 19, 2024 •

edited

Loading

cost model: unify builtin lists for core bpf migration tracking #4039

Are you sure you want to change the base?

cost model: unify builtin lists for core bpf migration tracking #4039

Conversation

buffalojoec commented Dec 10, 2024 • edited Loading

tao-stones left a comment

Choose a reason for hiding this comment

buffalojoec commented Dec 11, 2024

tao-stones left a comment

Choose a reason for hiding this comment

tao-stones Dec 11, 2024

Choose a reason for hiding this comment

buffalojoec Dec 12, 2024

Choose a reason for hiding this comment

tao-stones Dec 11, 2024

Choose a reason for hiding this comment

tao-stones Dec 11, 2024

Choose a reason for hiding this comment

tao-stones Dec 18, 2024

Choose a reason for hiding this comment

buffalojoec Dec 18, 2024

Choose a reason for hiding this comment

buffalojoec commented Dec 18, 2024

tao-stones left a comment

Choose a reason for hiding this comment

tao-stones Dec 18, 2024

Choose a reason for hiding this comment

LucasSte Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

buffalojoec commented Dec 10, 2024 •

edited

Loading

LucasSte Dec 19, 2024 •

edited

Loading