Skip to content

feat(runtime): borsh deserialized wasmer 0.x - #4448

Merged
near-bulldozer[bot] merged 23 commits into
masterfrom
use-borsh-deser-wasmer0
Jul 26, 2021
Merged

feat(runtime): borsh deserialized wasmer 0.x#4448
near-bulldozer[bot] merged 23 commits into
masterfrom
use-borsh-deser-wasmer0

Conversation

@ailisp

@ailisp ailisp commented Jul 1, 2021

Copy link
Copy Markdown
Contributor

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

  • CI
  • wasmer's bench code which covers the code path to ser/de cache with borsh
  • vm standalone test: evm_slow_deserialize_repro, test with no_cache feature (means no in memory cache), which will do a full serialize to disk cache, load and deserialize with borsh and contract call

@ailisp

ailisp commented Jul 1, 2021

Copy link
Copy Markdown
Contributor Author

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

@ailisp ailisp changed the title Draft PR to test borsh-deserialized wasmer 0.x on CI feat(runtime): borsh deserialized wasmer 0.x Jul 2, 2021
@ailisp
ailisp marked this pull request as ready for review July 2, 2021 08:22

@olonho olonho left a comment

Copy link
Copy Markdown
Contributor

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.

@bowenwang1996

Copy link
Copy Markdown
Contributor

Then it likely means we shall change the way how key is computed in cache.

How are they affected?

) -> CryptoHash {
let _span = tracing::debug_span!(target: "vm", "get_key").entered();
let key = ContractCacheKey::Version2 {
let key = ContractCacheKey::Version3 {

Copy link
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@matklad

matklad commented Jul 5, 2021

Copy link
Copy Markdown
Contributor

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.

@bowenwang1996

Copy link
Copy Markdown
Contributor

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

Comment thread runtime/near-vm-runner/src/cache.rs
@ailisp
ailisp requested review from matklad and olonho July 12, 2021 07:26
@matklad

matklad commented Jul 14, 2021

Copy link
Copy Markdown
Contributor

Use borsh deserialized wasmer 0.x, which provides a about 30% increase in wasmer 0.x deserialization.

Hm, seems like it's a much better improvement actually? Running

cargo test -q --package near-vm-runner-standalone --bin near-vm-runner-standalone --features no_cache --release -- script::test_evm_slow_deserialize_repro --exact --nocapture

I get 2.46ms deserialize_wasmer after this change and 13.40ms deserialize_wasmer before it, which actually is a rather sizable improvement!

@matklad

matklad commented Jul 14, 2021

Copy link
Copy Markdown
Contributor

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:

  • send borsh 0.9 upgrade as a separate PR, such that we can merge it and forget about it
  • fix the above issues in near/wasmer
  • publish wasmer 0.18 (without pre)
  • rebase this PR

@ailisp

ailisp commented Jul 15, 2021

Copy link
Copy Markdown
Contributor Author

I get 2.46ms deserialize_wasmer after this change and 13.40ms deserialize_wasmer before it, which actually is a rather sizable improvement!

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

@ailisp

ailisp commented Jul 20, 2021

Copy link
Copy Markdown
Contributor Author

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

@ailisp

ailisp commented Jul 20, 2021

Copy link
Copy Markdown
Contributor Author

@matklad my result also shows borsh is significantly faster.

borsh:
3.84ms deserialize_wasmer
3.79ms deserialize_wasmer
master:
18.96ms deserialize_wasmer
20.59ms deserialize_wasmer

@matklad

matklad commented Jul 21, 2021

Copy link
Copy Markdown
Contributor

Ok, so lets rebase this on master, and upgrade wasmer crates to 0.18 then!

@ailisp

ailisp commented Jul 22, 2021

Copy link
Copy Markdown
Contributor Author

@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 Memory, which is expected to have additional bytes. This is why deserialize works while try_from_slice doesn't.
Although it's technically more safe to find the ending index of CacheImage, and do ending check, wasmer 0.17 uses bincode::deserialize, which also doesn't check ending index too, so it's not more unsafe to use borsh::deserialize. I'll make a wasmer 0.18.1 to do that.

@ailisp

ailisp commented Jul 23, 2021

Copy link
Copy Markdown
Contributor Author

Talked in meeting with @matklad we agree with above comment, and the PR on wasmer side is near/wasmer#41

@matklad

matklad commented Jul 23, 2021

Copy link
Copy Markdown
Contributor

status: wasmer 0.18.1 is published, this needs rebase & update.

ailisp added 3 commits July 26, 2021 09:57