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

feat: stabilize LowerRegularOpCost2 #5365

Merged
merged 1 commit into from
Nov 19, 2021

Conversation

matklad
Copy link
Contributor

@matklad matklad commented Nov 18, 2021

Feature to stabilize

Lower wasm opcode cost from 2_207_874 to 822_756. See ae9241c for details, but the main contributor here is that we use intrinsics-based gas counter implementation, which preserves semantics, but makes things faster.

Testing and QA

The overall approach is to make gas counting faster, while preserving the semantics exactly, so we need only to verify that behavior stays the same. New gas counter implementation passes all our tests, and was fuzzed against the old implementation. Additionally, during the implementation we introduced a suite of new unit-tests to cover previously uncovered gas counter behavior. We haven't yet run the history replay tool, that's on todo list at the moment. The magnitude of improvement seems reasonable, and matches our back-of-the-envelope calculations (comparing with gas conting completely disabled). Additionally, we still have 3x safety multiplier for the wasm cost.

It's important to note that what this PR stabilizes is only the cost reduction. The change to the implementation of gas counter is enabled unconditionally, and is where the most risk lies.

Premortem

Suppose that we stabilized this feature, and things did go wrong. What could be the issue?

  • Gas intrinsification fails at compile time with some kind of panic.
  • Gas intrinsification fails at runtime with some kind of UB.
  • Behavior of gas counter is different for some edge cases.
  • Something breaks at the point where we integrate wasmer-side gas counter with nearcore

Checklist

  • Link to nightly nayduck run (./scripts/nayduck.py -t nightly/nightly.txt, docs): http://nayduck.near.org/
  • Update CHANGELOG.md to include this protocol feature in the Unreleased section.

@matklad matklad marked this pull request as draft November 18, 2021 19:19
@matklad matklad marked this pull request as ready for review November 19, 2021 14:14
@matklad matklad force-pushed the m/stabilize-lowered-gas-cost branch 2 times, most recently from 631a613 to 3302578 Compare November 19, 2021 14:24
@matklad
Copy link
Contributor Author

matklad commented Nov 19, 2021

Don't seem able to do a nayduck run:

Internal Server Error
The server encountered an internal error and was unable to complete your request. Either the server is overloaded or there is an error in the application.

Given that this is just a cost change, it's quite unlikely that nayduck uncovers some further problems.

@matklad
Copy link
Contributor Author

matklad commented Nov 19, 2021

Let's merge it to unblock #5361, but I would appreciate another look at the changes, to make sure I didn't mess up protocol version upgrade. cc @Longarithm

@bowenwang1996
Copy link
Collaborator

Don't seem able to do a nayduck run:

Internal Server Error
The server encountered an internal error and was unable to complete your request. Either the server is overloaded or there is an error in the application.

Given that this is just a cost change, it's quite unlikely that nayduck uncovers some further problems.

@mina86 please take a look

@mina86
Copy link
Contributor

mina86 commented Nov 19, 2021

Scheduling works:

$ ./scripts/nayduck.py -b master -s master -t nightly/pytest-sanity.txt
Sending request ...
Success. Success.  http://nayduck.near.org/#/run/2224

But there does seem to be an issue with login flow. Looking.

@mina86
Copy link
Contributor

mina86 commented Nov 19, 2021

Don't seem able to do a nayduck run:

Internal Server Error
The server encountered an internal error and was unable to complete your request. Either the server is overloaded or there is an error in the application.

Should work now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants