Stream: git-wasmtime

Topic: wasmtime / PR #4989 Cleanup cranelift-frontend

Wasmtime GitHub notifications bot (Sep 30 2022 at 19:28):

jameysharp requested cfallin for a review on PR #4989.

Wasmtime GitHub notifications bot (Sep 30 2022 at 19:28):

jameysharp opened PR #4989 from cleanup-frontend to main:

This PR eliminates most of the per-block state tracked by the frontend module in cranelift-frontend. I think I can eliminate the last bits of state while preserving all the debug-assertions, but I want to think about that more.

I tried to keep the individual commits well-separated so it may be worth reviewing them individually.

This saves a tiny bit of memory allocations and traffic, according to DHAT, but has basically no impact on performance.

Wasmtime GitHub notifications bot (Sep 30 2022 at 20:27):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Sep 30 2022 at 20:53):

bjorn3 created PR review comment:

Saltwater uses this function. (and so does it fork crabwater that aims to revive saltwater).

Wasmtime GitHub notifications bot (Sep 30 2022 at 20:53):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Sep 30 2022 at 20:58):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Sep 30 2022 at 21:01):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Sep 30 2022 at 21:01):

jameysharp created PR review comment:

I gather you mean https://github.com/nulla-git/crabwater/? I didn't know that existed.

Wasmtime GitHub notifications bot (Sep 30 2022 at 21:08):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Sep 30 2022 at 21:08):

bjorn3 created PR review comment:

Yes

Wasmtime GitHub notifications bot (Sep 30 2022 at 21:11):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Sep 30 2022 at 21:11):

jameysharp created PR review comment:

After discussion with Chris, I'm going to merge this despite that breakage. For anyone who's using this API, I'd be happy to help with migrating to something else after a discussion about what they actually need from it.

Wasmtime GitHub notifications bot (Sep 30 2022 at 21:11):

jameysharp merged PR #4989.

Wasmtime GitHub notifications bot (Sep 30 2022 at 21:21):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Sep 30 2022 at 21:21):

cfallin created PR review comment:

More specifically, to give some reasoning for this:

• We don't guarantee stability of Cranelift APIs between releases (each release is a semver break, and we haven't declared any sort of long-term stability guarantee generally);
• Thus, it's technically allowed to alter APIs arbitrarily;
• All the same, we want to minimize churn, as it's definitely in the project's interest to bring users along to new versions with minimal work. So we should have a reason to remove or alter APIs;
• In this case, @jameysharp 's reasoning (with which I think I agree) is that the function just seems a bit redundant or unnecessary: it lets the user query whether they have already added instructions, but it seems better either for the user to know that directly, or (possibly) to get that via more general IR introspection.

So on balance, I think this API cleanup makes sense, but we also don't want to cause arbitrary and unfixable breakage so please do let us know (anyone) if there are compatibility concerns we can try to address some other way.

Wasmtime GitHub notifications bot (Sep 30 2022 at 22:58):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Sep 30 2022 at 22:58):

abrown created PR review comment:

(also, is no one else chuckling about the puns in all these names?!)

Last updated: Nov 22 2024 at 16:03 UTC