refactor: remove GasOps from the Kernel #1951
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #1951 +/- ##
===========================================
- Coverage 75.79% 54.52% -21.27%
===========================================
Files 157 157
Lines 15441 15445 +4
===========================================
- Hits 11703 8422 -3281
- Misses 3738 7023 +3285
|
It's not really a proper "ops" trait, it's more "basic functionality". Additionally, the concept of a global price list is going to go away as we make the kernel more pluggable. 1. Move `GasOps::gas_available`/`GasOps::charge_gas` to the main `Kernel` trait as all kernels require them. 2. Remove `GasOps::gas_used`, it's only used for testing. 3. Remove `GasOps::price_list`. We're still exposing it on the CallManager, but one step at a time).
Stebalien
force-pushed
the
steb/cleanup-gas-ops
branch
from
fbee6fb
to
a627137
Compare
| @@ -286,6 +284,8 @@ lazy_static! { | |||
| // Charge 0.4gas/byte for copying/fill. | |||
| memory_copy_per_byte_cost: Gas::from_milligas(400), | |||
| memory_fill_per_byte_cost: Gas::from_milligas(400), | |||
|
|
|||
| host_call_cost: Gas::new(14000), | |||
There was a problem hiding this comment.
So, the "wasm" prices are generally just the prices used for wasm instrumentation. But the host call cost is so tightly bound to the wasm runtime that it kind of makes sense to just put it in this structure (and means that the syscall binding logic doesn't need to know about the global price list.
There was a problem hiding this comment.
yes, that make sense! qq, do we need to adjust these if we upgrade the runtime version?
There was a problem hiding this comment.
You mean the VM (wasmtime)? We should, but we haven't bothered re-measuring them for a while.
But I should expand on this a bit more: The rest of the prices here are actually baked into the compiled wasm modules when we instrument the code with gas accounting. That means they get cached in the engine (along with the compiled wasm modules) between epochs (across multiple FVM instances).
The host-call cost doesn't quite belong here because we charge for that on the outside so it doesn't become a part of the compiled wasm modules. Although, TBH, we should charge for it inside wasm as we generally avoid "after the fact" charges like this.
| @@ -4,10 +4,11 @@ use std::str; | |||
|
|
|||
| use super::Context; | |||
| use crate::gas::Gas; | |||
| use crate::kernel::{ClassifyResult, GasOps, Result}; | |||
| use crate::kernel::{ClassifyResult, Result}; | |||
| use crate::Kernel; | |||
|
|
|||
| pub fn charge_gas( | |||
There was a problem hiding this comment.
This syscall will go away once we get rid of the original prove-commit method: filecoin-project/FIPs#820.
| @@ -21,6 +22,6 @@ pub fn charge_gas( | |||
| .map(|_| ()) | |||
| } | |||
|
|
|||
| pub fn available(context: Context<'_, impl GasOps>) -> Result<u64> { | |||
| pub fn available(context: Context<'_, impl Kernel>) -> Result<u64> { | |||
There was a problem hiding this comment.
This syscall will hopefully go away and turn into a read of a global variable. See filecoin-project/FIPs#845
| @@ -286,6 +284,8 @@ lazy_static! { | |||
| // Charge 0.4gas/byte for copying/fill. | |||
| memory_copy_per_byte_cost: Gas::from_milligas(400), | |||
| memory_fill_per_byte_cost: Gas::from_milligas(400), | |||
|
|
|||
| host_call_cost: Gas::new(14000), | |||
There was a problem hiding this comment.
yes, that make sense! qq, do we need to adjust these if we upgrade the runtime version?
| @@ -32,7 +32,7 @@ pub struct CallResult { | |||
| /// | |||
| /// Actors may call into the kernel via the syscalls defined in the [`syscalls`][crate::syscalls] | |||
| /// module. | |||
| pub trait Kernel: GasOps + SyscallHandler<Self> + 'static { | |||
| pub trait Kernel: SyscallHandler<Self> + 'static { | |||
It's not really a proper "ops" trait, it's more "basic functionality".
Additionally, the concept of a global price list is going to go away as we make the kernel more pluggable.
GasOps::gas_available/GasOps::charge_gasto the mainKerneltrait as all kernels require them.GasOps::gas_used, it's only used for testing.GasOps::price_list. We're still exposing it on the CallManager, but one step at a time).