-
Notifications
You must be signed in to change notification settings - Fork 4
Implement linear fees (a + bx) for aggregate instructions #79
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
base: main
Are you sure you want to change the base?
Conversation
upstream interpreter doesn't even support this.
665b282
to
80989c0
Compare
pub(crate) fn checked_add(self, other: Fee) -> Option<Self> { | ||
Some(Self { | ||
linear: (self.linear == 0 && other.linear == 0).then_some(other.linear)?, | ||
constant: self.constant.checked_add(other.constant)?, | ||
}) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Double-checked this and this is fine. Whether instrumentation point can be merged or not is primarily determined by the InstrumentationKind
. In past code whether gas can be added was only important for handling of possible overflow.
In principle this could be relaxed somewhat to allow for the 1st instrumentation point with linear fee to merge in any further constant fees into itself, but that makes this function not commutative, and I don't want to review all of the implications of that at this point in time, so we won't allow merging in any case where fees are non-constant for now.
This has adjusted the finite-wasm analysis and instrumentation to collect fees not as constant values for each operation, but in form of
a + bx
. However, using linear fees extensively is not recommended, as fees that haveb ≠ 0
are not mergeable and will incur a significant instrumentation overhead.As a future change it may be possible to still merge some instances where
b ≠ 0
, but since a lot of care will be necessary to make sure thatx
stays the same, merging logic will need to be somewhat more complicated than it currently is.Even then, since all of the bulk_memory operations for which linear fees are useful are trapping merging across the instruction is not applicable at all.
This has required somewhat extensive changes to the interpreter we use in our specification as well as to our test runner to allow for situations where the aggregate instruction may trap in the middle of its execution. In particular, say,
memory.copy
may trap after processing 5 elements (thus charging 5 gas according to the reference interpreter,) but at instrumentation level we only have an instruction-level granularity available, so we must pre-charge gas assuming the entire operation would succeed. This means that the actual gas consumption observed from the interpreter can be anywhere betweengas_before_aggregate <= gas_before_aggregate + steps
if a trap occurs.Recommended review strategy is to look at the "implement linear fees for aggregate instructions" in isolation and only skim the 2nd commit to verify instrumentation changes appear sane. Reminder that users aren't required to use
instrument.rs
module in production and neither does nearcore (though not for wasmtime...) -- they can get by with just analysis results and introduce instrumentation directly into the generated machine code.Based on top of #78
Fixes #49