Simplify gas metering #5121

olonho · 2021-11-03T07:57:09Z

Rework the logic of gas counting to ensure that there's a single limit to be compared with.
It allows to speed up gas metering significantly and opens possibility of intrinsified gas counting.

Rework the logic of gas counting to ensure that there's a single limit to be compared with. It allows to speed up gas metering significantly and opens possibility of intrinsified gas counting.

runtime/near-vm-logic/src/gas_counter.rs

matklad · 2021-11-04T13:20:51Z

runtime/near-vm-runner/src/tests.rs

@@ -63,7 +63,8 @@ fn make_simple_contract_call_with_gas_vm(
    let mut fake_external = MockedExternal::new();
    let mut context = create_context(vec![]);
    context.prepaid_gas = prepaid_gas;
-    let config = VMConfig::default();
+    let mut config = VMConfig::default();
+    config.limit_config.max_gas_burnt = prepaid_gas;


It seems that this makes near-vm-runner tests less useful, as they now can only hit the cases where the two limits are identical. Can we keep the original version?

matklad · 2021-11-04T13:46:28Z

runtime/near-vm-logic/src/gas_counter.rs

+            // if max_gas_burnt == prepaid_gas we must use GasExceeded error.
+            if new_burnt_gas > self.max_gas_burnt && self.max_gas_burnt != self.prepaid_gas {
+                self.fast_counter.burnt_gas = self.max_gas_burnt;
+                self.promises_gas = 0;


If I comment-out this line, the tests seem to pass. That's suspicious. Can we add a test that shows that this line is needed?

We can add this line when making protocol change.

EgorKulikov · 2021-11-04T15:07:17Z

runtime/near-vm-logic/src/gas_counter.rs

+    pub fn burn_gas(&mut self, value: Gas) -> Result<()> {
+        let new_burnt_gas =
+            self.fast_counter.burnt_gas.checked_add(value).ok_or(HostError::IntegerOverflow)?;
+        if new_burnt_gas <= self.fast_counter.gas_limit {


Theoretically in view mode we could have promised something close to u64::max() gas by now and with new burnt gas that could have overflown integer in deduct_gas. Not sure if we care enough about this case

Don't think that (checked) overflow in view mode is critical/important.

Anyway, fixed it.

EgorKulikov

Good work

olonho · 2021-11-04T15:24:15Z

WasmInstruction 442_940

matklad · 2021-11-04T16:28:00Z

Results for me

Before:
WasmInstruction                                       709_273
After:
WasmInstruction                                       459_905

That's sort-of the motivation here -- we can measurably speed the stuff up by just writing simpler code for the common case.

runtime/near-vm-logic/src/gas_counter.rs

olonho · 2021-11-05T09:43:15Z

Latest version gives WasmInstruction 442_952.

addresses near#5121 (comment)

bowenwang1996 · 2021-11-05T23:06:04Z

Results for me
Before:
WasmInstruction                                       709_273
After:
WasmInstruction                                       459_905
That's sort-of the motivation here -- we can measurably speed the stuff up by just writing simpler code for the common case.

@matklad this seems to be higher than what we projected. Do we know why that is the case?

olonho · 2021-11-08T07:38:57Z

Results for me
Before:
WasmInstruction                                       709_273
After:
WasmInstruction                                       459_905
That's sort-of the motivation here -- we can measurably speed the stuff up by just writing simpler code for the common case.
@matklad this seems to be higher than what we projected. Do we know why that is the case?

It's only part of the optimization - second one is fully intrinsified counter on Wasmer side.

Add vm logic tests targeting gas counting logic specifically. In particular, `test_cant_burn_more_than_max_gas_burnt_gas` I believe passes the current master, but fails with #5121 (which passes all other tests).

matklad

I think this now preserves behavior, so that's good to go!

I am still a little bit on the fence about adding #[repr(C)] bit in this PR and not in the one which actually enables wasmer change, but we can always remove that later if it proves to be unnecessary.

Could you confirm that the this final version with all bugs fixed still gives us an improvement?

olonho · 2021-11-08T17:28:50Z

Sure, we haven't changed fast path since last measurement.

runtime/near-vm-logic/src/gas_counter.rs

As our measurements show that gas counting is significant part of execution cost, we need to optimize it as much as possible. This PR provides support for shared fast gas counter structure, second part is implemented in near/nearcore#5121 Fast gas counter has three fields: current counter, limit and op cost. When we see call to host function called `gas` with single argument we do not actually emit such a call, but instead emit inline instruction sequence to increment gas counter per requested amount and if limit is hit - GasExceed trap happens. For fast gas counter to work VM embedder shall provide gas counter context, and VM will trap on call to `gas` function if such context wasn't provided.

* Gas intrinsic support As our measurements show that gas counting is significant part of execution cost, we need to optimize it as much as possible. This PR provides support for shared fast gas counter structure, second part is implemented in near/nearcore#5121 Fast gas counter has three fields: current counter, limit and op cost. When we see call to host function called `gas` with single argument we do not actually emit such a call, but instead emit inline instruction sequence to increment gas counter per requested amount and if limit is hit - GasExceed trap happens. For fast gas counter to work VM embedder shall provide gas counter context, and VM will trap on call to `gas` function if such context wasn't provided. Co-authored-by: Aleksey Kladov <aleksey.kladov@gmail.com>

olonho force-pushed the simplify_counter branch from 7d87464 to 48d1aa4 Compare November 4, 2021 11:06

Simplify gas metering

7166e24

Rework the logic of gas counting to ensure that there's a single limit to be compared with. It allows to speed up gas metering significantly and opens possibility of intrinsified gas counting.

olonho force-pushed the simplify_counter branch from 48d1aa4 to 7166e24 Compare November 4, 2021 12:37

Merge branch 'master' into simplify_counter

0133367

olonho marked this pull request as ready for review November 4, 2021 12:39

olonho requested review from ailisp, bowenwang1996, EgorKulikov, matklad and mzhangmzz as code owners November 4, 2021 12:39

Fix few nits

1853a88

matklad reviewed Nov 4, 2021

View reviewed changes

runtime/near-vm-logic/src/gas_counter.rs Show resolved Hide resolved

matklad reviewed Nov 4, 2021

View reviewed changes

Review feedback

5a582f7

EgorKulikov reviewed Nov 4, 2021

View reviewed changes

EgorKulikov approved these changes Nov 4, 2021

View reviewed changes

Merge branch 'master' into simplify_counter

5b8bedb

olonho force-pushed the simplify_counter branch from 077ea5f to 5b8bedb Compare November 4, 2021 15:39

Merge branch 'master' into simplify_counter

c2a2dee

bowenwang1996 reviewed Nov 4, 2021

View reviewed changes

runtime/near-vm-logic/src/gas_counter.rs Outdated Show resolved Hide resolved

runtime/near-vm-logic/src/gas_counter.rs Outdated Show resolved Hide resolved

olonho mentioned this pull request Nov 5, 2021

Rethink gas spending logic #5148

Open

olonho and others added 3 commits November 5, 2021 11:35

Review feedback

805dafe

Merge branch 'master' into simplify_counter

e597eb4

Fix bug, refactoring

aab1fe8

Improve readability.

aa2a690

matklad mentioned this pull request Nov 5, 2021

test: add more cases for gas overflow behavior #5150

Merged

Fix a corner case.

c011f0a

matklad mentioned this pull request Nov 5, 2021

Estimate test coverage for our runtime code #5152

Open

matklad added a commit to matklad/nearcore that referenced this pull request Nov 5, 2021

test: add couple of more tests

5fcc5e4

addresses near#5121 (comment)

olonho and others added 3 commits November 6, 2021 02:08

More compatibility tests

f7241c7

Formatting

b78a4b0

Merge branch 'master' into simplify_counter

a3de3a5

olonho force-pushed the simplify_counter branch from c6cbb1b to a3de3a5 Compare November 7, 2021 09:51

olonho and others added 2 commits November 8, 2021 10:22

Merge branch 'master' into simplify_counter

9c01da2

Simplify

906e78f

Merge branch 'master' into simplify_counter

dfcabbf

olonho mentioned this pull request Nov 8, 2021

Gas intrinsic proto near/wasmer#64

Merged

Merge branch 'master' into simplify_counter

c494334

olonho and others added 4 commits November 8, 2021 16:34

Merge branch 'master' into simplify_counter

ce33161

Merge branch 'master' into simplify_counter

fce534d

Typo

bdfe01c

Merge branch 'master' into simplify_counter

461f110

matklad approved these changes Nov 8, 2021

View reviewed changes

olonho merged commit 468d806 into master Nov 8, 2021

olonho deleted the simplify_counter branch November 8, 2021 17:29

mina86 reviewed Nov 10, 2021

View reviewed changes

runtime/near-vm-logic/src/gas_counter.rs Show resolved Hide resolved

mina86 mentioned this pull request Nov 10, 2021

runtime: prefer reporting GasLimitExceeded #5187

Merged

gmilescu mentioned this pull request Mar 10, 2023

Estimate test coverage for our runtime code #9947

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify gas metering #5121

Simplify gas metering #5121

olonho commented Nov 3, 2021

matklad Nov 4, 2021

olonho Nov 4, 2021

matklad Nov 4, 2021

olonho Nov 4, 2021

EgorKulikov Nov 4, 2021

olonho Nov 4, 2021

olonho Nov 5, 2021

EgorKulikov left a comment

olonho commented Nov 4, 2021

matklad commented Nov 4, 2021

olonho commented Nov 5, 2021

bowenwang1996 commented Nov 5, 2021

olonho commented Nov 8, 2021

matklad left a comment

olonho commented Nov 8, 2021

Simplify gas metering #5121

Simplify gas metering #5121

Conversation

olonho commented Nov 3, 2021

matklad Nov 4, 2021

Choose a reason for hiding this comment

olonho Nov 4, 2021

Choose a reason for hiding this comment

matklad Nov 4, 2021

Choose a reason for hiding this comment

olonho Nov 4, 2021

Choose a reason for hiding this comment

EgorKulikov Nov 4, 2021

Choose a reason for hiding this comment

olonho Nov 4, 2021

Choose a reason for hiding this comment

olonho Nov 5, 2021

Choose a reason for hiding this comment

EgorKulikov left a comment

Choose a reason for hiding this comment

olonho commented Nov 4, 2021

matklad commented Nov 4, 2021

olonho commented Nov 5, 2021

bowenwang1996 commented Nov 5, 2021

olonho commented Nov 8, 2021

matklad left a comment

Choose a reason for hiding this comment

olonho commented Nov 8, 2021