[move] Move out tag cache (safe speculation) #15676
Conversation
⏱️ 48m total CI duration on this PR
|
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
georgemitenkov
force-pushed
the
george/struct-tag-cache
branch
from
22da815 to
ad7e871
Compare
georgemitenkov
requested review from
runtian-zhou,
vgao1996,
gelash,
zekun000 and
ziaptos
georgemitenkov
force-pushed
the
george/struct-tag-cache
branch
from
ad7e871 to
5102271
Compare
| @@ -1671,15 +1672,15 @@ pub const VALUE_DEPTH_MAX: u64 = 128; | |||
| /// fields for struct types. | |||
| const MAX_TYPE_TO_LAYOUT_NODES: u64 = 256; | |||
There was a problem hiding this comment.
hm, random thought but shouldn't such constants be defined in some more centralized place.. there should be something already, maybe @runtian-zhou ?
There was a problem hiding this comment.
Yes, we should put them in VM config or even gas schedule. I will moving this code around for lazy loading and V2 loader layout cache, so we can do it then.
| pub(crate) struct PseudoGasContext { | ||
| pub(crate) max_cost: u64, | ||
| pub(crate) cost: u64, | ||
| pub(crate) cost_base: u64, |
There was a problem hiding this comment.
this should really better be base_cost, right?
There was a problem hiding this comment.
I think it aligns with cost_per_byte? So we have cost_... everywhere, but I have no preference here.
| let mut gas_context = PseudoGasContext { | ||
| cost: 0, | ||
| max_cost: loader.vm_config().type_max_cost, | ||
| cost_base: loader.vm_config().type_base_cost, |
There was a problem hiding this comment.
does VM config check max_cost >= base? it would be a nice check to add to some places (here or config or ty_tag_cache)
There was a problem hiding this comment.
Added a constructor for the gas context (cost is 0 initialized, hence, we cannot have < base), and made fields private.
| /// Converts a runtime type into a type tag. If the type is too complex (e.g., struct name size | ||
| /// too large, or type too deeply nested), an error is returned. | ||
| pub(crate) fn ty_to_ty_tag(&self, ty: &Type) -> PartialVMResult<TypeTag> { | ||
| let mut gas_context = PseudoGasContext { |
There was a problem hiding this comment.
maybe we should create another constructor for PseudoGasContext (there is one straight from config).
There was a problem hiding this comment.
Exactly! Did that
| // - maximum allowed cost, | ||
| // - base cost for any type to tag conversion, | ||
| // - cost for size of a struct tag. | ||
| max_cost: u64, |
There was a problem hiding this comment.
Can we encapsulate these params and re-use them across here and PseudoGasContext? I.e. pseudogascontext can have a CoW of the params (then when we create context in calls below we can take a reference of params)
There was a problem hiding this comment.
Added a constructor into gas context, now encapsulated.
| /// If thread 3 reads the tag of this enum, the read result is always **deterministic** for the | ||
| /// fixed type parameters used by thread 3. | ||
| pub(crate) struct TypeTagCache { | ||
| cache: RwLock<HashMap<StructNameIndex, HashMap<Vec<Type>, PricedStructTag>>>, |
There was a problem hiding this comment.
why a two-layer cache and not just key (StructNameIndex, Vec)?
Say if we used DashMap it could be for sharding on the first key, but here I don't see a difference other than having to chain calls?
There was a problem hiding this comment.
Dashmap cannot work because if you have (&idx, &ty_args), you cannot construct &(idx, ty_args)? What we can do is use hashbrown::HashMap which for get() requires key Q: Equivalent<K> of an actual K, so we can get by both (&idx, &ty_args) and &(idx, ty_args)?
| gas_context.charge(size * gas_context.cost_per_byte)?; | ||
|
|
||
| // Cache the struct tag. Record its gas cost as well. | ||
| let priced_tag = PricedStructTag { |
There was a problem hiding this comment.
does it make sense to let the max cost go to max cost + cur_cost temporarily, so we can cache the struct and not discard work, but then we can still return the error if it went above max cost.
There was a problem hiding this comment.
@vgao1996 interesting question in general I think.
| /// # Speculative execution safety | ||
| /// | ||
| /// A struct name corresponds to a unique [StructNameIndex]. So all non-generic structs with same | ||
| /// names have the same struct tags. If structs are generic, the number of type parameters cannot |
There was a problem hiding this comment.
I think a missing piece here is that types also can't change, right?
and maybe worth saying what the tag depends on as an abstract function, and that arguing those don't change (or when they change, e.g. enums, below)
There was a problem hiding this comment.
Tags do not change, correct, because they depend on names -> if we have an index and if we have a name, tag is uniquely defined by these two + type arguments, which are types which also cannot changed because those contain indices.
|
|
||
| // Calculate and charge the cost for the struct tag, proportionally to its size. | ||
| let size = | ||
| (struct_tag.address.len() + struct_tag.module.len() + struct_tag.name.len()) as u64; |
There was a problem hiding this comment.
oh, we must be paying so much efficiency in resource groups using these tags as keys @igor-aptos 😢
| }) | ||
| } | ||
|
|
||
| fn struct_name_to_ty_tag( |
There was a problem hiding this comment.
technically it's struct name idx, so we could either call it struct_to_ty_tag, or struct_name_idx_to_ty_tag.
|
@gelash V1 loader defines it in its huge |
georgemitenkov
force-pushed
the
george/struct-tag-cache
branch
from
5102271 to
a4b33f5
Compare
Description
This PR moves type tag cache out of the other type cache because it is conceptually different: type tags can be cached speculatively, same as indexed struct names. As a result:
RuntimeEnvironment.TypeTagBuilderAPI to encapsulate conversions from types to tags. Under the hood, it uses a pseudo gas meter. TestsHow Has This Been Tested?
Key Areas to Review
Type of Change
Which Components or Systems Does This Change Impact?
Checklist