Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Track gas spending per parameter #8033

Closed
Tracked by #8032
jakmeier opened this issue Nov 10, 2022 · 3 comments · Fixed by #8148
Closed
Tracked by #8032

Track gas spending per parameter #8033

jakmeier opened this issue Nov 10, 2022 · 3 comments · Fixed by #8148
Assignees
Labels
A-transaction-runtime Area: transaction runtime (transaction and receipts processing, state transition, etc) T-contract-runtime Team: issues relevant to the contract runtime team T-core Team: issues relevant to the core team

Comments

@jakmeier
Copy link
Contributor

jakmeier commented Nov 10, 2022

Currently we only track gas spending for gas profiles. The list is defined by core/primitives-core/src/profile.rs::Cost which is not in a one-to-one relation to gas parameters. But we want gas weights to be per gas parameter.

The specific task here is to make sure we at track gas spending per parameter. In this first step, we don't want to change the output of the gas profiles, we just want to make the reduction to Cost at a later stage in the code.

Follow-up work on this could be to also update gas profiles, but let's keep that as a separate discussion.
Further, this probably makes #5719 more straight-forward, as we can always reconstruct the counters from profiles once gas profiles are detailed enough.

@jakmeier jakmeier added A-transaction-runtime Area: transaction runtime (transaction and receipts processing, state transition, etc) T-core Team: issues relevant to the core team T-contract-runtime Team: issues relevant to the contract runtime team labels Nov 10, 2022
@jakmeier jakmeier self-assigned this Nov 10, 2022
@jakmeier
Copy link
Contributor Author

This is a bit tricky to do without making the code base even more horrible. I think it is worth doing major refactor and deleting a bunch of types. It will require multiple PRs. But here is an outline of where it should lead in the end.

Current situation:

  • Gas burning is done by looking up specific field in RuntimeConfig.
  • Gas profiles are Cost -> Gas mappings, where Cost only exists for profiling.
  • From a profile, it is impossible to reconstruct the per-parameter gas costs for actions.
  • We also have a feature "costs_counting" that counts without multiplying by gas values. Only used for estimator, basically because we cannot easily go back to Parameter from Cost.
  • The source of truth for RuntimeConfig is ParameterTable and the Parameter values inside. But we after the ParameterTable -> JSON -> RuntimeConfig conversion we lose the ability to index gas costs by Parameter.
  • RuntimeConfig cannot be changed because it uses serde derived deserialization to JSON in ProtocolConfigView that is returned by the "EXPERIMENTAL_protocol_config" RPC call

Goal

  • Detach the RPC output from RuntimeConfig struct.
  • Index both profiles and the runtime config by Parameter -> Gas, and delete enum Cost used in profiles today. Also delete ExtCostsConfig but probably still keep ActionsCosts to combine send/exec fee tripple parameters to a single variant.
  • Change all places where gas is charged to be defined by Parameter only instead of idependently looking up Cost and a specific field in config.
  • Bonus: Delete feature "costs_counting"

Refactoring steps

  1. Expand ActionsCosts to have a 1:1 mapping with action parameter tripples.
  2. Replace RuntimeConfig in ProtocolConfigView with e.g. json::Value that is constructed manually rather than through serde.
  3. Replace structured ExtCostsConfig with Map<Parameter,Gas> and replace (ExtCostsConfig,Parameter) -> Gas boilerplate code with simple map lookup.
  4. Simplify RuntimeConfig to allow action fee lookup by ActionCosts, e.g.:
 struct RuntimeConfig {
    // used to be `RuntimeFeesConfig` with several levels of nesting down
    pub transaction_costs: Map<ActionCosts,Fee>,

    // unchanged
    pub storage_amount_per_byte: Balance,
    pub account_creation_config: AccountCreationConfig,
    pub wasm_config: VMConfig,

    // lifted up from `RuntimeFeesConfig`
    pub storage_usage_config: StorageUsageConfig,
    pub burnt_gas_reward: Rational,
    pub pessimistic_gas_price_inflation_ratio: Rational,
}

then use ActionCosts to lookup action fees and update profile.

/// before
let action_cost = ActionCosts::add_key;
let fee = transaction_costs.action_creation_config.add_key_cost.full_access_cost;
deduct_gas(fee.send_fee(sir), fee.exec_fee());
update_profile_action(action_cost, fee.send_fee(sir));

// after
let action_cost = ActionCosts::add_full_access_key;
let fee = transaction_costs[action_cost];
deduct_gas(fee.send_fee(sir), fee.exec_fee());
update_profile_action(action_cost, fee.send_fee(sir));
  1. Change profile to track by Parameter instead of by Cost, without breaking backwards-compatibility with existing profiles on DB. (Serialize and deserialize the same as today.)
  2. Optional: Introduce ExecutionMetadata::V3(ProfileDataV3) that keeps more detailed profiles than today.
  3. Optional: Delete feature "costs_counting" and instead get counters by dividing profiles by parameter values.

@matklad
Copy link
Contributor

matklad commented Nov 14, 2022

Tangentially related, but today profile exists only in the action_function_call and doesn't account for costs we charge elsewhere. Making gas profile a runtime concept (rathre than a vm_runner concept) seems like a good idea.

@jakmeier
Copy link
Contributor Author

That's a great point, thanks for pointing it out! I didn't even consider it but if we want parameter weights to work everywhere, this is actually required.

jakmeier added a commit to jakmeier/nearcore that referenced this issue Nov 14, 2022
So far several actions fees were conflated right when they are added to
the profile. This is a first step to make profiles universal by tracking
 in more fine-grained manner at the profile interface. Inside, it's
 still conflated but it's easier to refactor in steps. (see near#8033)
near-bulldozer bot pushed a commit that referenced this issue Nov 15, 2022
So far, several actions fees were conflated right when they are added to
the profile. Here is a first step to make profiles universal by tracking
in more fine-grained manner at the profile interface. Inside, it's
still conflated but it's easier to refactor in steps. (see #8033)
jakmeier added a commit to jakmeier/nearcore that referenced this issue Nov 22, 2022
Sorry. This is a humongous change.
But it's really just a lot of the same boilerplate code changes.

The key changes are:
- Action fees are now accessed through one single function
  `RuntimeFeesConfig::fee(&self, cost: ActionCosts)` instead of
  selecting a specific field deep down in the structure
- Action fees are stored in an EnumMap instead of by hard-coded fields
- The JSON RPC now has `RuntimeConfigView` to keep compatibility
- Creation of `RuntimeConfig` no longer goes thorough serde JSON
  serialization + deserialization

The goal of this PR is to make gas profiling based on parameters
possible, which in turn makes parameter weights possible to implement
much more easily. The key here is that we no longer have to lookup the
gas value up independently from the profile key.

Today:
```rust
self.gas_counter.pay_action_base(
    &self.fees_config.action_creation_config.deploy_contract_cost,
    sir,
    ActionCosts::deploy_contract_base,
)?;
```
After PR:
```rust
self.pay_action_base(ActionCosts::deploy_contract_base, sir)?;
```

Also it gives us simplified fee lookup on `RuntimeFeesConfig`.
Before, we need things like:
```rust
let exec_gas = self.cfg.action_receipt_creation_config.exec_fee()
    + self.cfg.action_creation_config.add_key_cost.function_call_cost.exec_fee()
    + num_bytes
        * self
            .cfg
            .action_creation_config
            .add_key_cost
            .function_call_cost_per_byte
            .exec_fee();
```

which becomes

```rust
let exec_gas = self.cfg.fee(ActionCosts::new_action_receipt).exec_fee()
    + self.cfg.fee(ActionCosts::add_function_call_key_base).exec_fee()
     + num_bytes * self.cfg.fee(ActionCosts::add_function_call_key_byte).exec_fee();
```

The final state after all refactoring is done is described in near#8033.
This PR contains step 2 and 4 combined because I was not able to
separate them due to unforeseen dependencies we have in the code.

The hardest part should be done after the current PR and what follows
should be small PRs that get rid of types one by one.

# Test
We have to make sure this is a pure refactoring change without
side-effects. Luckily, we have `test_json_unchanged` that detects
changes in any protocol version.
Otherwise, we rely on tests that have gas profile snapshots to make
sure the gas fee lookup translation did not have errors.
near-bulldozer bot pushed a commit that referenced this issue Nov 24, 2022
refactor: cleaning up RuntimeConfig

Sorry. This is a humongous change.
But it's really just a lot of the same boilerplate code changes.

The key changes are:
- Action fees are now accessed through one single function 
  `RuntimeFeesConfig::fee(&self, cost: ActionCosts)` instead of
  selecting a specific field deep down in the structure
- Action fees are stored in an EnumMap instead of by hard-coded fields
- The JSON RPC now has `RuntimeConfigView` to keep compatibility
- Creation of `RuntimeConfig` no longer goes thorough serde JSON
  serialization + deserialization

The goal of this PR is to make gas profiling based on parameters
possible, which in turn makes parameter weights possible to implement
much more easily. The key here is that we no longer have to lookup the
gas value up independently from the profile key.

Today:
```rust
self.gas_counter.pay_action_base(
    &self.fees_config.action_creation_config.deploy_contract_cost,
    sir,
    ActionCosts::deploy_contract_base,
)?;
```
After PR:
```rust
self.pay_action_base(ActionCosts::deploy_contract_base, sir)?;
```

Also it gives us simplified fee lookup on `RuntimeFeesConfig`.
Before, we need things like:
```rust
let exec_gas = self.cfg.action_receipt_creation_config.exec_fee()
    + self.cfg.action_creation_config.add_key_cost.function_call_cost.exec_fee()
    + num_bytes
        * self
            .cfg
            .action_creation_config
            .add_key_cost
            .function_call_cost_per_byte
            .exec_fee();
```

which becomes

```rust
let exec_gas = self.cfg.fee(ActionCosts::new_action_receipt).exec_fee()
    + self.cfg.fee(ActionCosts::add_function_call_key_base).exec_fee()
     + num_bytes * self.cfg.fee(ActionCosts::add_function_call_key_byte).exec_fee();
```

The final state after all refactoring is done is described in #8033.
This PR contains step 2 and 4 combined because I was not able to
separate them due to unforeseen dependencies we have in the code.

The hardest part should be done after the current PR and what follows
should be small PRs that get rid of types one by one.

# Test
We have to make sure this is a pure refactoring change without
side-effects. Luckily, we have `test_json_unchanged` that detects
changes in any protocol version.
Otherwise, we rely on tests that have gas profile snapshots to make
sure the gas fee lookup translation did not have errors.
jakmeier added a commit to jakmeier/nearcore that referenced this issue Nov 24, 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 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`.
nikurt pushed a commit that referenced this issue Nov 26, 2022
refactor: cleaning up RuntimeConfig

Sorry. This is a humongous change.
But it's really just a lot of the same boilerplate code changes.

The key changes are:
- Action fees are now accessed through one single function 
  `RuntimeFeesConfig::fee(&self, cost: ActionCosts)` instead of
  selecting a specific field deep down in the structure
- Action fees are stored in an EnumMap instead of by hard-coded fields
- The JSON RPC now has `RuntimeConfigView` to keep compatibility
- Creation of `RuntimeConfig` no longer goes thorough serde JSON
  serialization + deserialization

The goal of this PR is to make gas profiling based on parameters
possible, which in turn makes parameter weights possible to implement
much more easily. The key here is that we no longer have to lookup the
gas value up independently from the profile key.

Today:
```rust
self.gas_counter.pay_action_base(
    &self.fees_config.action_creation_config.deploy_contract_cost,
    sir,
    ActionCosts::deploy_contract_base,
)?;
```
After PR:
```rust
self.pay_action_base(ActionCosts::deploy_contract_base, sir)?;
```

Also it gives us simplified fee lookup on `RuntimeFeesConfig`.
Before, we need things like:
```rust
let exec_gas = self.cfg.action_receipt_creation_config.exec_fee()
    + self.cfg.action_creation_config.add_key_cost.function_call_cost.exec_fee()
    + num_bytes
        * self
            .cfg
            .action_creation_config
            .add_key_cost
            .function_call_cost_per_byte
            .exec_fee();
```

which becomes

```rust
let exec_gas = self.cfg.fee(ActionCosts::new_action_receipt).exec_fee()
    + self.cfg.fee(ActionCosts::add_function_call_key_base).exec_fee()
     + num_bytes * self.cfg.fee(ActionCosts::add_function_call_key_byte).exec_fee();
```

The final state after all refactoring is done is described in #8033.
This PR contains step 2 and 4 combined because I was not able to
separate them due to unforeseen dependencies we have in the code.

The hardest part should be done after the current PR and what follows
should be small PRs that get rid of types one by one.

# Test
We have to make sure this is a pure refactoring change without
side-effects. Luckily, we have `test_json_unchanged` that detects
changes in any protocol version.
Otherwise, we rely on tests that have gas profile snapshots to make
sure the gas fee lookup translation did not have errors.
near-bulldozer bot pushed a commit that referenced this issue Dec 7, 2022
This is the next step in the refactoring steps for changing gas profiles
to track gas by parameter (#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`.
nikurt pushed a commit to nikurt/nearcore that referenced this issue 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`.
near-bulldozer bot pushed a commit that referenced this issue Jan 11, 2023
This resolves #8033, completing the gas profile by parameter effort.

The main achievement here is that we can now  look at gas profiles and
know exactly for which parameter it has been spent.

Code wise, the old `enum Cost` used in profiles only is finally gone.

The tricky parts in this PR are to make sure that old profiles stored on DB
are still understood and presented correctly in RPC calls.
nikurt pushed a commit to nikurt/nearcore that referenced this issue Jan 15, 2023
This resolves near#8033, completing the gas profile by parameter effort.

The main achievement here is that we can now  look at gas profiles and
know exactly for which parameter it has been spent.

Code wise, the old `enum Cost` used in profiles only is finally gone.

The tricky parts in this PR are to make sure that old profiles stored on DB
are still understood and presented correctly in RPC calls.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-transaction-runtime Area: transaction runtime (transaction and receipts processing, state transition, etc) T-contract-runtime Team: issues relevant to the contract runtime team T-core Team: issues relevant to the core team
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants