-
Notifications
You must be signed in to change notification settings - Fork 663
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(runtime): borsh deserialized wasmer 0.x #4448
Conversation
Try to first deserialize with borsh then fallback doesn't really safe, there can be logical error from no error deserialization. I'll introduce a new version of cache to handle backward compatibility |
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.
You suggest to use it in prod? Then it likely means we shall change the way how key is computed in cache.
How are they affected? |
@@ -52,7 +59,7 @@ pub fn get_contract_cache_key( | |||
config: &VMConfig, | |||
) -> CryptoHash { | |||
let _span = tracing::debug_span!(target: "vm", "get_key").entered(); | |||
let key = ContractCacheKey::Version2 { | |||
let key = ContractCacheKey::Version3 { |
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.
@olonho This change invalidated old keys in cache and make it pass the upgradable test. Without this change it fails of deserialization cache failed
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.
Also @bowenwang1996
I wonder if it makse sense to send borsh 0.8 -> borsh 0.9 as a separate PR? Both borsh and wasmer changes feel medium risky to me, would be great if we can check them independently. |
Yeah that sounds like a good idea |
Hm, seems like it's a much better improvement actually? Running
I get |
The changes here look ok, however I've took another glance at the wasmer side of things, and found two potential correctness issues:
I suggest the following here:
|
It depends on contract, but I do haven't see this much improvement in my benchmarks. Mine usually shows 10%-50% faster on different contracts. Make sure you're comparing release build, and maybe the contract you're testing borsh is significantly faster |
The issue mentioned by @matklad is fixed and merged in near/wasmer#40. I'm retrying the same bench to see if I can get similar result to @matklad |
@matklad my result also shows borsh is significantly faster. borsh: |
Ok, so lets rebase this on master, and upgrade wasmer crates to 0.18 then! |
@matklad weirdly, wasmer 0.18 doesn't pass tests, a few deserilization error. By debugging locally, I found deserialize -> try_from_slice is the cause. This matches my memory that wasmer 0.x has intentional remaining bytes after CacheImage, Actually it happens on every contract, including evm and test_contract_rs. test_evm_slow_deserialize_repro didn't catch it due to there's defect in the test, i didn't check the second time (call from deserialized contract) success. Adding that, then only wasmer 0.18.0-pre.1 pass the test. After further examine, this is due to wasmer try to deserialize CacheImage from a serialized |
Talked in meeting with @matklad we agree with above comment, and the PR on wasmer side is near/wasmer#41 |
status: wasmer 0.18.1 is published, this needs rebase & update. |
store_update.commit().unwrap(); | ||
|
||
set_store_version(&store, 26); | ||
} |
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.
I think it's fine to just completely bust the precompilation cache at this time, as we haven't yet deployed compilation on contract deployment to the main net, and the code is written in such a way that empty cache causes slowdowns, rather then errors, so testnet should be fine.
That is, it's fine to land this without tackling #4473 first. @bowenwang1996 please veto this if I am mistaken in my assumptions here.
You've already approved this, so I'll auto-merge, but wanted to call this out just in case.
Use borsh deserialized wasmer 0.x, which provides about 30% time saving in wasmer's several testing contract bench and 80% in near evm contract, and other contract bench shows 25%-50% improve on a cloud instance: #3847. This uses a pre release wasmer-near. Will replace with a formal released wasmer-near after it's released
Test Plan