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

perf: Switch LensVM to wasmtime runtime #2030

Merged
merged 3 commits into from
Nov 16, 2023

Conversation

AndrewSisley
Copy link
Contributor

@AndrewSisley AndrewSisley commented Nov 7, 2023

Relevant issue(s)

Resolves #2029

Description

Switches LensVM to the wasmtime runtime.

Should be a fair bit faster than wazero, and supports all the important build targets. Also doesn't suffer from tetratelabs/wazero#1818

Note on wasmtime-go:

This Go library uses CGO to consume the C API of the Wasmtime project which is written in Rust. Precompiled binaries of Wasmtime are checked into this repository on tagged releases so you won't have to install Wasmtime locally, but it means that this project only works on Linux x86_64, macOS x86_64 , and Windows x86_64 currently. Building on other platforms will need to arrange to build Wasmtime and use CGO_* env vars to compile correctly.

This is different to wazero, which as a pure Go project supports pretty much anything Go does without messing about.

Note: I had to change a test module, the new Lens version has closed a memory leak upon which the defra test module was relying on.

Todo

  • Have a chat about cross compilation issues

How has this been tested?

Specify the platform(s) on which this was tested:

  • Debian Linux
  • Mac arm64
  • Windows

@AndrewSisley AndrewSisley added area/schema Related to the schema system perf Performance issue or suggestion labels Nov 7, 2023
@AndrewSisley AndrewSisley added this to the DefraDB v0.9 milestone Nov 7, 2023
@AndrewSisley AndrewSisley self-assigned this Nov 7, 2023
Copy link

codecov bot commented Nov 7, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (177ca01) 74.03% compared to head (a32a92c) 73.78%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2030      +/-   ##
===========================================
- Coverage    74.03%   73.78%   -0.25%     
===========================================
  Files          248      248              
  Lines        24801    24801              
===========================================
- Hits         18359    18298      -61     
- Misses        5189     5236      +47     
- Partials      1253     1267      +14     
Flag Coverage Δ
all-tests 73.78% <100.00%> (-0.25%) ⬇️

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

Files Coverage Δ
lens/registry.go 79.51% <100.00%> (ø)

... and 7 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

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

@AndrewSisley AndrewSisley requested a review from a team November 8, 2023 16:39
@AndrewSisley AndrewSisley marked this pull request as ready for review November 8, 2023 16:39
@AndrewSisley AndrewSisley force-pushed the 2029-wasmtime-lens branch 4 times, most recently from 6884f19 to 0fd88e0 Compare November 8, 2023 18:40
Copy link
Collaborator

@fredcarle fredcarle left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@shahzadlone shahzadlone left a comment

Choose a reason for hiding this comment

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

Comment from discord after passing make test:lens on windows:

Perhaps can add in README:

  • Enable CGO
  • Need GCC (for windows need mingw, i.e. choco install mingw)

@jsimnz
Copy link
Member

jsimnz commented Nov 8, 2023

@shahzadlone

CGO is needed regardless with this wasmtime integration. Is there something more specific you are referring to with the windows build?

@shahzadlone
Copy link
Member

@shahzadlone

CGO is needed regardless with this wasmtime integration. Is there something more specific you are referring to with the windows build?

Yea so normally on linux I have never had to worry about enabling cgo manually.

However on windows, by default cgo is disabled. So it needs to be enabled and Inaddition the mingw dependency is needed for gcc tool chain.

Would be nice to document this in build requirements in README explicitly for windows.

@fredcarle
Copy link
Collaborator

fredcarle commented Nov 8, 2023

The need for cgo really sucks. Too bad Wasmer isn't more actively maintained.

@jsimnz
Copy link
Member

jsimnz commented Nov 9, 2023

The need for cgo really sucks. Too bad Wasmer isn't more actively maintained.

CGO applied to wasmer as well. Unless you mean wazero. But yes, not a fan of it in general, as it does some wonky things to the go runtime.

It shouldn't be hard to get wazero back to a place we can use it with the enumerator work. Then we can add a option as to which build users want. Wasmtime and speed, but with cgo vs wazero and slower, but no cgo

@fredcarle
Copy link
Collaborator

I think I mixed the two yes 🙃

Copy link
Contributor

@islamaliev islamaliev left a comment

Choose a reason for hiding this comment

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

LGTM

@AndrewSisley AndrewSisley force-pushed the 2029-wasmtime-lens branch 5 times, most recently from 9944c58 to ed55acc Compare November 16, 2023 17:33
@AndrewSisley AndrewSisley merged commit bf63b0a into sourcenetwork:develop Nov 16, 2023
@AndrewSisley AndrewSisley deleted the 2029-wasmtime-lens branch November 16, 2023 18:24
shahzadlone pushed a commit to shahzadlone/defradb that referenced this pull request Jan 22, 2024
## Relevant issue(s)

Resolves sourcenetwork#2029

## Description

Switches LensVM to the wasmtime runtime.

Should be a fair bit faster than wazero, and supports all the important
build targets. Also doesn't suffer from
tetratelabs/wazero#1818

Note on [wasmtime-go](https://github.com/bytecodealliance/wasmtime-go):
> This Go library uses CGO to consume the C API of the [Wasmtime
project](https://github.com/bytecodealliance/wasmtime) which is written
in Rust. Precompiled binaries of Wasmtime are checked into this
repository on tagged releases so you won't have to install Wasmtime
locally, but it means that this project only works on Linux x86_64,
macOS x86_64 , and Windows x86_64 currently. Building on other platforms
will need to arrange to build Wasmtime and use CGO_* env vars to compile
correctly.

This is different to wazero, which as a pure Go project supports pretty
much anything Go does without messing about.
shahzadlone pushed a commit to shahzadlone/defradb that referenced this pull request Feb 23, 2024
## Relevant issue(s)

Resolves sourcenetwork#2029

## Description

Switches LensVM to the wasmtime runtime.

Should be a fair bit faster than wazero, and supports all the important
build targets. Also doesn't suffer from
tetratelabs/wazero#1818

Note on [wasmtime-go](https://github.com/bytecodealliance/wasmtime-go):
> This Go library uses CGO to consume the C API of the [Wasmtime
project](https://github.com/bytecodealliance/wasmtime) which is written
in Rust. Precompiled binaries of Wasmtime are checked into this
repository on tagged releases so you won't have to install Wasmtime
locally, but it means that this project only works on Linux x86_64,
macOS x86_64 , and Windows x86_64 currently. Building on other platforms
will need to arrange to build Wasmtime and use CGO_* env vars to compile
correctly.

This is different to wazero, which as a pure Go project supports pretty
much anything Go does without messing about.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/schema Related to the schema system perf Performance issue or suggestion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Switch Defra Lens to wasmtime runtime
5 participants