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

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

Merged
merged 11 commits into from
Sep 9, 2022

Conversation

qdm12
Copy link
Contributor

@qdm12 qdm12 commented Aug 12, 2022

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 qdm12 force-pushed the qdm12/wasmer/helpers-file branch 2 times, most recently from 011acc6 to 4316030 Compare August 15, 2022 14:27
@codecov
Copy link

codecov bot commented Aug 15, 2022

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     

Copy link
Member

@EclesioMeloJunior EclesioMeloJunior left a comment

Choose a reason for hiding this comment

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

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?

lib/runtime/wasmer/helpers.go Outdated Show resolved Hide resolved
Copy link
Contributor

@jimjbrettj jimjbrettj left a comment

Choose a reason for hiding this comment

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

lgtm

@qdm12
Copy link
Contributor Author

qdm12 commented Aug 16, 2022

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 qdm12 force-pushed the qdm12/wasmer/helpers-file branch 3 times, most recently from 869fd3c to 38ae18f Compare August 17, 2022 16:02
@qdm12
Copy link
Contributor Author

qdm12 commented Aug 18, 2022

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 qdm12 marked this pull request as draft August 18, 2022 13:56
@codefromthecrypt
Copy link

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.

@qdm12
Copy link
Contributor Author

qdm12 commented Sep 7, 2022

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!

- 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`
- Use slice capacity allocation
- Use `copy`
- Remove unneeded appends
@qdm12 qdm12 marked this pull request as ready for review September 8, 2022 19:05
@qdm12 qdm12 merged commit 7fa9196 into development Sep 9, 2022
@qdm12 qdm12 deleted the qdm12/wasmer/helpers-file branch September 9, 2022 13:52
timwu20 pushed a commit that referenced this pull request Sep 12, 2022
- 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 `append`s
  - `toKillStorageResultEnum`: use `copy` instead of `append`
  - `toWasmMemoryOptional`: remove unneeded variable copy
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.

5 participants