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.

Sign up for GitHub

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

Jump to bottom

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 •
edited
Loading

Upgrade WasmVM to v0.13.0

@alpe alpe marked this pull request as ready for review January 7, 2021 11:06
@alpe alpe requested a review from webmaster128 January 7, 2021 11:06
This was referenced Jan 7, 2021
Closed
Merged
ethanfrey
ethanfrey approved these changes Jan 7, 2021
View reviewed changes
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

x/wasm/internal/keeper/keeper.go
@@ -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

@alpe alpe mentioned this pull request Jan 7, 2021
Merged
@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
webmaster128
webmaster128 reviewed Jan 7, 2021
View reviewed changes
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

@ethanfrey ethanfrey mentioned this pull request Jan 7, 2021
Merged
@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.

This was referenced Jan 7, 2021
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ethanfrey ethanfrey ethanfrey approved these changes

@webmaster128 webmaster128 webmaster128 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@alpe @ethanfrey @webmaster128