Unify builtin lists wip #4039
Unify builtin lists wip #4039
Conversation
There was a problem hiding this comment.
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?
buffalojoec
force-pushed
the
unify-builtin-lists-wip
branch
from
62aefd8 to
1c5c3d7
Compare
|
@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 |
| }; | ||
|
|
||
| // Hehe, if you change me, I will break things... | ||
| pub const NUM_COST_MODELED_BUILTINS_WITH_MIGRATIONS: usize = 3; |
There was a problem hiding this comment.
Yet another way to let compiler know the size of migrating builtins. Tho it also requires developer to pay attention and take manual steps as they are #3975, both cases have guardrails builtin. Personally prefer this simplified approach.
There was a problem hiding this comment.
Yeah I'm sure you tried the same steps as me 😂 - I couldn't get a solution where there wasn't some compiler gotcha, but at least here it will throw if you don't do it right.
| 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] = { |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
| pub fn get_migration_feature_index(program_id: &Pubkey) -> Option<usize> { | ||
| COST_MODELED_BUILTINS_WITH_MIGRATIONS | ||
| .iter() | ||
| .position(|builtin| builtin.program_id == *program_id) |
There was a problem hiding this comment.
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.
| @@ -40,7 +45,11 @@ impl BuiltinProgramsFilter { | |||
| return ProgramKind::NotBuiltin; | |||
| } | |||
|
|
|||
| if is_builtin_program(program_id) { | |||
| if let Some(index) = get_migration_feature_index(program_id) { | |||
There was a problem hiding this comment.
Currently most popular builtins are not migrating yet, might be more efficient to check in this order: Not-builtinin -> Builtin -> migrating-builtin
| } | ||
|
|
||
| lazy_static! { | ||
| static ref BUILTIN_INSTRUCTION_COSTS: AHashMap<Pubkey, BuiltinCost> = BUILTINS |
There was a problem hiding this comment.
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.
No description provided.