Skip to content

Enable default features for wasmtime #366

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rahulchaphalkar
Copy link

The default-features=false is not present in wasmtime v9.0.3 c-api ( LINK ) , but it was still present in Cargo.toml in this repo (bazel/cargo/wasmtime/Cargo.toml)
This PR removes that i.e. enables default features for wasmtime.
This required a cargo raze --generate-lockfile and other associated changes (like removing dups from generated build files).
Previous discussion for this is here #313

Signed-off-by: rahulchaphalkar <rahul.s.chaphalkar@intel.com>
@rahulchaphalkar
Copy link
Author

@PiotrSikora Any issue with enabling this? After enabling default features, a second PR will enable us to support VTune for wasmtime ( a small change in wasmtime.cc in this repo) which in turn will enable wasm code profiling with VTune for Envoy.

@PiotrSikora
Copy link
Member

  1. At the very least, you need to split this into 2 PRs and separate update of dependencies from enabling default features, otherwise this is unreviewable.
  2. What's the justification for enabling default features? It enables 6 features and I don't think all of them should be enabled.

@rahulchaphalkar
Copy link
Author

  1. What's the justification for enabling default features?

I can just enable one feature, 'vtune' instead of all default features. I just assumed Cargo.toml here is following wasmtime/crates/c-api/Cargo.toml in wasmtime repo, so followed the changes there.

  1. split this into 2 PRs

If i understand this correctly, I can have 1st PR where I just update deps with cargo raze --generate-lockfile, fix the dup deps issue. And in 2nd PR, update with feature vtune, fix any additional dep issues that might arise from it after rerunning cargo raze --generate-lockfile.

@PiotrSikora
Copy link
Member

I can just enable one feature, 'vtune' instead of all default features.

vtune is not c-api feature, so it wouldn't be enabled with this change anyway, would it?

I just assumed Cargo.toml here is following wasmtime/crates/c-api/Cargo.toml in wasmtime repo, so followed the changes there.

That was the case, but not all default features make sense in context of Proxy-Wasm C++ Host.

If i understand this correctly, I can have 1st PR where I just update deps with cargo raze --generate-lockfile, fix the dup deps issue. And in 2nd PR, update with feature vtune, fix any additional dep issues that might arise from it after rerunning cargo raze --generate-lockfile.

Correct. Ideally, you could update Wasmtime to the latest version in the process.

@rahulchaphalkar
Copy link
Author

vtune is not c-api feature

Right, I was planning on enabling it for the wasmtime dependency in c-api, similar to how cranelift is enabled (i would add back default features = false -

wasmtime = {version = "9.0.3", features = ['cranelift']}

Ideally, you could update Wasmtime

Sure, I can update wasmtime to its latest v13.0.0 and update deps in 1st PR, and then enable vtune in 2nd PR (and some associated bazel config options).

On the default features - I had hoped that features like pooling-allocator and parallel-compilation would be helpful for perf in some cases. Would it make sense to enable these on a case-by-case basis down the road (after some initial investigation/perf estimates in a separate issue)?

@PiotrSikora
Copy link
Member

On the default features - I had hoped that features like pooling-allocator and parallel-compilation would be helpful for perf in some cases. Would it make sense to enable these on a case-by-case basis down the road (after some initial investigation/perf estimates in a separate issue)?

Enabling features one-by-one is fine, if justified.

@rahulchaphalkar
Copy link
Author

PR (1/2) opened for updating wasmtime, #368

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants