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

chore(lib/runtime/wasmer): refactor Exec and exec methods #2686

Merged
merged 9 commits into from
Jul 22, 2022
Merged

chore(lib/runtime/wasmer): refactor Exec and exec methods #2686

merged 9 commits into from
Jul 22, 2022

Conversation

qdm12
Copy link
Contributor

@qdm12 qdm12 commented Jul 21, 2022 •
edited
Loading

Changes

Simple refactor to adapt exec to be more independent, in order to have an instance-independent check runtime function.

  • Remove nil storage check for temporary instances (some calls don't need it, and anyway we should panic if this happens imo)
  • Fix for bigger than max int32 pointer and data length (before > int32 pointers/data length would crash it)
  • Minor simplifications
    • Only use Exec instead of both directly wrapping exec
    • Inline single/2-lines functions content in exec: malloc, clear, store, load
    • Add sentinel errors and error wrappings
    • Add named returns

Tests

Issues

#2418

Primary Reviewer

@EclesioMeloJunior

@qdm12 qdm12 force-pushed the qdm12/wasmer/refactor-exec branch from ce1e2f5 to 3be46af Compare July 21, 2022 13:26
@qdm12 qdm12 force-pushed the qdm12/wasmer/refactor-exec branch from 3be46af to 23151bf Compare July 21, 2022 14:06
@codecov
Copy link

codecov bot commented Jul 21, 2022

Codecov Report

Merging #2686 (23151bf) into development (70181cd) will increase coverage by 0.02%.
The diff coverage is 88.00%.

@@               Coverage Diff               @@
##           development    #2686      +/-   ##
===============================================
+ Coverage        63.02%   63.04%   +0.02%     
===============================================
  Files              211      211              
  Lines            27056    27043      -13     
===============================================
- Hits             17052    17050       -2     
+ Misses            8455     8450       -5     
+ Partials          1549     1543       -6
Flag Coverage Δ
unit-tests 63.04% <88.00%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
lib/runtime/wasmer/instance.go 56.14% <78.57%> (-1.36%) ⬇️
lib/runtime/wasmer/exports.go 57.62% <100.00%> (ø)
dot/state/block_notify.go 81.90% <0.00%> (-8.58%) ⬇️
dot/network/host.go 64.31% <0.00%> (-1.07%) ⬇️
lib/runtime/wasmer/imports.go 55.46% <0.00%> (+0.17%) ⬆️
dot/network/notifications.go 65.29% <0.00%> (+1.37%) ⬆️
dot/network/service.go 58.13% <0.00%> (+1.39%) ⬆️
dot/telemetry/mailer.go 61.19% <0.00%> (+4.47%) ⬆️
dot/network/inbound.go 100.00% <0.00%> (+7.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 70181cd...23151bf. Read the comment docs.

@qdm12 qdm12 marked this pull request as ready for review July 21, 2022 15:27
@qdm12 qdm12 mentioned this pull request Jul 21, 2022
Merged
@jimjbrettj
Copy link
Contributor

Would error -> graceful shutdown be better than panicking?

jimjbrettj
jimjbrettj reviewed Jul 22, 2022
View reviewed changes
lib/runtime/wasmer/instance.go
@@ -264,67 +264,45 @@ func (in *Instance) Stop() {
}
}

// Store func
func (in *Instance) store(data []byte, location int32) {
Copy link
Contributor

Choose a reason for hiding this comment

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

why did we want to remove these functions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was me trying to understand everything that was needed in that Exec method, and those tiny methods made it just hard to understand tbh.

And these 1 or 2-lines functions were only used once (in prod code, and once in a test), so I just inlined them.

So I took the EXECUTIVE DECISION to inline them pretty much 😄

jimjbrettj
jimjbrettj reviewed Jul 22, 2022
View reviewed changes
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.

looks good just few questions

@qdm12
Copy link
Contributor Author

qdm12 commented Jul 22, 2022

@jimjbrettj

Would error -> graceful shutdown be better than panicking?

Well in standard Go, panic should really be used only for programming errors.
But in this case, we have no way to return errors given the CGO function signature, so we agreed to panic in case of errors. Anyway these errors are very unlikely except if for example we run out of memory or our disk is physically bad for example, so it's fine to panic. Maybe one day once our service architecture is reworked, we could signal through a channel on the instance we encountered a fatal error and recover from it... but not today 😄

@qdm12 qdm12 merged commit 491af95 into development Jul 22, 2022
@qdm12 qdm12 deleted the qdm12/wasmer/refactor-exec branch July 22, 2022 20:24
@qdm12 qdm12 mentioned this pull request Sep 29, 2022
Merged
rrtti pushed a commit to polkadot-fellows/seeding that referenced this pull request Sep 29, 2022
@qdm12
Member Request (#28)
a37485d 
I am Quentin Mc Gaw, a software engineer working the Go Polkadot host **Gossamer**.
I have been working full time on Gossamer since October 2021, mostly on the state trie and storage.
I have also made a [few minor pull requests](https://github.com/w3f/polkadot-spec/pulls?q=is%3Apr+is%3Aclosed+author%3Aqdm12) to the Polkadot specification repository.

I am requesting to join the Fellowship at rank 1.

## Main contributions

### Gossamer

- Fix memory leaks
  - Trie encoding buffer pools usage fixed [#2009](ChainSafe/gossamer#2009)
  - Fix state map of tries memory leak [#2286](ChainSafe/gossamer#2286)
  - Fix sync benchmark [#2234](ChainSafe/gossamer#2234)
- Trie proof fixes ([#2604](ChainSafe/gossamer#2604), [#2661](ChainSafe/gossamer#2661))
- Fix end to end tests orchestration ([#2470](ChainSafe/gossamer#2470), [#2452](ChainSafe/gossamer#2452), [#2385](ChainSafe/gossamer#2385), [#2370](ChainSafe/gossamer#2370))
- State trie statistics ([#2378](ChainSafe/gossamer#2378), [#2310](ChainSafe/gossamer#2310), [#2272](ChainSafe/gossamer#2272))
- State trie fixes and improvements
  - Only deep copy nodes when mutation is certain [#2352](ChainSafe/gossamer#2352) and [#2223](ChainSafe/gossamer#2223)
  - Only deep copy necessary fields of a node [#2384](ChainSafe/gossamer#2384)
  - Use Merkle values for database keys instead of always hash [#2725](ChainSafe/gossamer#2725)
  - Opportunistic parallel Merkle value commputing [#2081](ChainSafe/gossamer#2081)
- Grandpa capped number of tracked messages ([#2490](ChainSafe/gossamer#2490), [#2485](ChainSafe/gossamer#2485))
- Add pprof HTTP service for profiling [#1991](ChainSafe/gossamer#1991)

Ongoing work:

- State trie lazy loading and caching
- State trie v1 support ([#2736](ChainSafe/gossamer#2736), [#2747](ChainSafe/gossamer#2747), [#2687](ChainSafe/gossamer#2687), [#2686](ChainSafe/gossamer#2686), [#2685](ChainSafe/gossamer#2685), [#2673](ChainSafe/gossamer#2673), [#2611](ChainSafe/gossamer#2611), [#2530](ChainSafe/gossamer#2530))

### Polkadot specification

➡️ [Pull requests from qdm12](https://github.com/w3f/polkadot-spec/pulls?q=is%3Apr+is%3Aclosed+author%3Aqdm12)
@github-actions
Copy link

🎉 This PR is included in version 0.7.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jimjbrettj jimjbrettj jimjbrettj left review comments

@EclesioMeloJunior EclesioMeloJunior EclesioMeloJunior approved these changes

@timwu20 timwu20 timwu20 approved these changes

@noot noot Awaiting requested review from noot

@edwardmack edwardmack Awaiting requested review from edwardmack

@kishansagathiya kishansagathiya Awaiting requested review from kishansagathiya

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

Successfully merging this pull request may close these issues.
4 participants
@qdm12 @jimjbrettj @timwu20 @EclesioMeloJunior