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

Upgrade WasmVM to v0.13.0 #358

Merged
merged 3 commits into from
Jan 7, 2021
Merged

Upgrade WasmVM to v0.13.0 #358

merged 3 commits into from
Jan 7, 2021

Conversation

alpe
Copy link
Contributor

@alpe alpe commented Jan 7, 2021

Upgrade WasmVM to v0.13.0

Copy link
Member

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

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

Looks good, as soon as broken CI test fixed.
One idea for a follow up

@@ -42,6 +42,10 @@ const InstanceCost uint64 = 40_000
// CompileCost is how much SDK gas we charge *per byte* for compiling WASM code.
const CompileCost uint64 = 2

// contractMemoryLimit is the memory limit of each contract execution (in MiB)
// constant value so all nodes run with the same limit.
const contractMemoryLimit = 32
Copy link
Member

Choose a reason for hiding this comment

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

Let's make this a param in a later PR.
If not too much work, we can add that as a param (with default 32 MB) in a separate PR, but include that in the 0.14 release.

I am happy to merge this first and test it manually before that addition. (It can come later as well)

Copy link
Member

Choose a reason for hiding this comment

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

After reflection, that params issue is part of #202
Let's just do that with everything else

@codecov
Copy link

codecov bot commented Jan 7, 2021

Codecov Report

Merging #358 (70f35d8) into master (b8d9033) will increase coverage by 0.08%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #358      +/-   ##
==========================================
+ Coverage   69.64%   69.73%   +0.08%     
==========================================
  Files          28       28              
  Lines        2425     2425              
==========================================
+ Hits         1689     1691       +2     
+ Misses        618      617       -1     
+ Partials      118      117       -1     
Impacted Files Coverage Δ
x/wasm/internal/keeper/keeper.go 90.56% <100.00%> (+0.58%) ⬆️

@ethanfrey ethanfrey merged commit a66d773 into master Jan 7, 2021
@ethanfrey ethanfrey deleted the wasmvm_upgrade branch January 7, 2021 11:43
Copy link
Member

@webmaster128 webmaster128 left a comment

Choose a reason for hiding this comment

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

This PR updates the Go code, but not libwasmvm_muslc.a. Dockerfile needs to be updated as well.

@ethanfrey
Copy link
Member

Ah, I will check the Dockerfile. Thanks for the review

@ethanfrey
Copy link
Member

Interesting the .a file doubled in size from 0.12 to 0.13 from ~30MB to ~60MB

Not a major issue, but interesting if you know why

$ ls -l
total 99228
-rw-rw-r-- 1 ethan ethan 38236768 ene  7 13:40 012.a
-rw-rw-r-- 1 ethan ethan 63363772 ene  7 13:38 013.a

@webmaster128
Copy link
Member

Interesting the .a file doubled in size from 0.12 to 0.13 from ~30MB to ~60MB

One part of it is probably that we have the (unused) cranelift compiler in our dependencies now. This is a consequence of how Wasmer packages and features are organized now.

@ethanfrey
Copy link
Member

Ah, that makes sense.

Final docker image grows from 224MB to 237MB, so no real issue there. Just an oddity I wanted to flag.
Including cranelift makes sense.

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