gossamer icon indicating copy to clipboard operation
gossamer copied to clipboard

chore(wasmer): add helpers.go file with helper functions

Open qdm12 opened this issue 2 years ago • 2 comments

Changes

  • Split out (CGO related) helper functions from imports.go to lib/runtime/wasmer/helpers.go
  • Move pointer size helper functions to lib/runtime/wasmer/helpers.go
  • Change toWasmMemorySized to NOT take a size argument (unneeded)
  • Clarify all comments for helper functions
  • Update all error wrappings
  • Review variable names
    • Use ptr instead of out for 32 bit pointers
    • Use pointerSize instead of span for 64 bit pointers size
    • Name return values
    • Other minor renamings such as res to result, enc to encodedResult
  • Optimizations:
    • storageAppend: use slice capacity allocation, copy and remove unneeded appends
    • toKillStorageResultEnum: use copy instead of append
    • toWasmMemoryOptional: remove unneeded variable copy

Tests

go test -tags integration github.com/ChainSafe/gossamer

Issues

Friday evening fun. Also aiming to make the migration to wazero easier.

Primary Reviewer

@EclesioMeloJunior

qdm12 avatar Aug 12 '22 22:08 qdm12

Codecov Report

Merging #2749 (040c139) into development (62d750d) will increase coverage by 0.23%. The diff coverage is 68.96%.

Additional details and impacted files
@@               Coverage Diff               @@
##           development    #2749      +/-   ##
===============================================
+ Coverage        63.02%   63.25%   +0.23%     
===============================================
  Files              213      213              
  Lines            26965    26957       -8     
===============================================
+ Hits             16994    17051      +57     
+ Misses            8425     8358      -67     
- Partials          1546     1548       +2     

codecov[bot] avatar Aug 15 '22 14:08 codecov[bot]

LGTM, just a suggestion: since all the operations relies on the wasmer.InstanceContext could we rename the file from helpers.go to context.go or instance_context.go?

Good suggestion, although storageAppend isn't really about context, and I also sneaked in pointer size conversion functions in there, so helpers.go seems appropriate for now.

qdm12 avatar Aug 16 '22 22:08 qdm12

Looks like Deepsource doesn't like having two cgo files in the same package and fails compiling, I'll convert this back to draft and fix this later. Maybe split this in another subpackage for the sake of deepsource 🤔

qdm12 avatar Aug 18 '22 13:08 qdm12

FYI, I noticed this was in prep of a possible use of wazero, and thanks for thinking of us.

wazero just cut its first beta tag (v1.0.0-beta.1) and also opened a gophers slack #wazero channel for support, updates and conversation! Note: You may need an invite to join gophers.

We've also been working harder on our website for things people usually can't find https://wazero.io/languages/

Wish you well and hope to see you around.

codefromthecrypt avatar Aug 30 '22 08:08 codefromthecrypt

Awesome @codefromthecrypt thanks for letting us know! We're probably a few months away from switching to wazero due to other priorities, but it's definitely something we would all like to jump to for sure!

qdm12 avatar Sep 07 '22 13:09 qdm12