refactor: ExtCostsConfig as simple map of costs #8115
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is the next step in the refactoring steps for changing gas profiles to track gas by parameter (near#8033). Here we make `ExtCostsConfig` opaque and look up parameters by ```rust pub fn cost(&self, param: ExtCosts) -> Gas ``` instead of using a specific field inside. There are side-effects for this in 1. parameter definition 2. JSON RPC 3. parameter estimator 1) We no longer load the parameters through "parameter table -> JSON -> serde_deser" steps because `ExtCostsConfig` no longer has serde derives. Instead each `ExtCosts` maps to a `Parameter` that allows looking up the value directly from `ParameterTable`. This explicit mapping also replaces the `Parameter::ext_costs()` iterator previously used to find all parameters that are ext costs. We used to define `wasm_read_cached_trie_node` in `53.txt` and fill old values with serde default. Serde was removed here, so I changed it to define the parameter the base files. This is equivalent to the old behavior, only it is less clear when we added the parameter. 2) JSON RPC must keep the old format. Thus, I added `ExtCostsConfigView` and `VMConfigView` there. It is a direct copy-paste of the old structs in the old format but without serde magic to fill in missing values. 3) The estimator generates a `ExtCostsConfig` from estimations. This is now done through a mapping from estimated costs to `ExtCosts`. # Testing The exact JSON output is checked in existing tests `test_json_unchanged`.
|
ping @Longarithm |
Longarithm
approved these changes
Dec 6, 2022
| EnumCount, | ||
| Display, | ||
| strum::EnumIter, | ||
| enum_map::Enum, | ||
| )] | ||
| #[allow(non_camel_case_types)] | ||
| pub enum ExtCosts { |
There was a problem hiding this comment.
Not related to the PR, but I think ExtCost is a better name, because a single entity corresponds to a single cost, not many of them.
There was a problem hiding this comment.
Yes it has been bothering me ever since I joined haha. Same for ActionCosts. I'm just not a huge fan of renaming things, as it messes with git history badly.
nikurt
pushed a commit
to nikurt/nearcore
that referenced
this pull request
Dec 14, 2022
This is the next step in the refactoring steps for changing gas profiles to track gas by parameter (near#8033). Here we make `ExtCostsConfig` opaque and look up parameters by ```rust pub fn cost(&self, param: ExtCosts) -> Gas ``` instead of using a specific field inside. There are side-effects for this in 1. parameter definition 2. JSON RPC 3. parameter estimator 1) We no longer load the parameters through "parameter table -> JSON -> serde_deser" steps because `ExtCostsConfig` no longer has serde derives. Instead each `ExtCosts` maps to a `Parameter` that allows looking up the value directly from `ParameterTable`. This explicit mapping also replaces the `Parameter::ext_costs()` iterator previously used to find all parameters that are ext costs. We used to define `wasm_read_cached_trie_node` in `53.txt` and fill old values with serde default. Serde was removed here, so I changed it to define the parameter in the base files. This is equivalent to the old behavior, only it is less clear when we added the parameter. 2) JSON RPC must keep the old format. Thus, I added `ExtCostsConfigView` and `VMConfigView` there. It is a direct copy-paste of the old structs in the old format but without serde magic to fill in missing values. 3) The estimator generates a `ExtCostsConfig` from estimations. This is now done through a mapping from estimated costs to `ExtCosts`. # Testing The exact JSON output is checked in existing tests `test_json_unchanged`.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is the next step in the refactoring steps for changing gas profiles
to track gas by parameter (#8033).
Here we make
ExtCostsConfigopaque and look up parameters byinstead of using a specific field inside.
There are side-effects for this in
"parameter table -> JSON -> serde_deser" steps because
ExtCostsConfigno longer has serde derives. Instead each
ExtCostsmaps to aParameterthat allows looking up the value directly fromParameterTable.This explicit mapping also replaces the
Parameter::ext_costs()iterator previously used to find all parameters that are ext costs.
We used to define
wasm_read_cached_trie_nodein53.txtand fillold values with serde default. Serde was removed here, so I changed it
to define the parameter in the base files. This is equivalent to the
old behavior, only it is less clear when we added the parameter.
JSON RPC must keep the old format. Thus, I added
ExtCostsConfigViewand
VMConfigViewthere. It is a direct copy-paste of the old structsin the old format but without serde magic to fill in missing values.
The estimator generates a
ExtCostsConfigfrom estimations. This isnow done through a mapping from estimated costs to
ExtCosts.Testing
The exact JSON output is checked in existing tests
test_json_unchanged.