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

Removing wasmtime's cache feature also removes the dependency on zstd which has a significant effect on compile times. #752

Merged
merged 2 commits into from
Aug 28, 2023

Conversation

Pombal
Copy link
Contributor

@Pombal Pombal commented Aug 25, 2023

No description provided.

… which has a significant effect on compile times.
Base automatically changed from revert_optivorbis to main August 25, 2023 20:49
@Pombal Pombal requested a review from philpax August 25, 2023 21:29
@ten3roberts
Copy link
Contributor

This will likely conflict with #661 as it changes the wasmtime dependency altogether and no longer uses it

Copy link
Contributor

@philpax philpax left a comment

Choose a reason for hiding this comment

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

TIL about cache. It would be nice to get that working at some point to avoid the JIT compilation hit if you're launching the same modules again, but it seems underdocumented so I'm guessing it's not really there yet. LGTM otherwise.

@philpax
Copy link
Contributor

philpax commented Aug 28, 2023

This will likely conflict with #661 as it changes the wasmtime dependency altogether and no longer uses it

Doesn't the native client still include wasmtime in that PR?

# NOTE: accessible through `wasm-bridge`. This is used to enforce specific features on the `wasmtime` crate
wasmtime = { workspace = true  }

@Pombal
Copy link
Contributor Author

Pombal commented Aug 28, 2023

@ten3roberts Are you ok with this change landing and resolving the conflict once client-wasm in merged?

@ten3roberts
Copy link
Contributor

ten3roberts commented Aug 28, 2023

Yes, I can resolve the merge conflict absollutely. It'll just unfortunately noop when my branch is merged and we will have to add it to wasm-bridge instead, but I'll migrate the side effects of this change over to that crate 😊

@Pombal Pombal merged commit a0cf577 into main Aug 28, 2023
13 of 14 checks passed
@Pombal Pombal deleted the wasmtime_no_cache branch August 28, 2023 08:44
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.

3 participants