Stream: git-wasmtime

Topic: wasmtime / issue #4066 ISLE: investigate always building ...


view this post on Zulip Wasmtime GitHub notifications bot (Apr 22 2022 at 06:05):

cfallin opened issue #4066:

In our most recent Cranelift meeting, we discussed the question of whether we want to continue to rely on checked-in, pre-generated ISLE DSL compiler output, or else move back to a build strategy that always builds from the DSL.

The two main advantages of the current situation are:

On the other hand, I believe that the following downsides are becoming more relevant:

The above factors are a mix of friction for newcomers (required understanding and complexity) and regular contributors (rebase pain). If the goal of the DSL is to make modifying the compiler easy, then the above factors I think are working against the goal of the DSL.

A quick survey of some other compilers and their build systems: of LLVM, SpiderMonkey and Golang, all of which have some meta-build steps, only Golang checks in generated source (and in the Go community this seems to be a bit more idiomatic).

So, to me, the question becomes how to support the needs above (build time, debugging) in a different way, if we can. I'd like to see if we can:

Thoughts? cc @abrown @fitzgen

view this post on Zulip Wasmtime GitHub notifications bot (Apr 22 2022 at 17:54):

fitzgen labeled issue #4066:

In our most recent Cranelift meeting, we discussed the question of whether we want to continue to rely on checked-in, pre-generated ISLE DSL compiler output, or else move back to a build strategy that always builds from the DSL.

The two main advantages of the current situation are:

On the other hand, I believe that the following downsides are becoming more relevant:

The above factors are a mix of friction for newcomers (required understanding and complexity) and regular contributors (rebase pain). If the goal of the DSL is to make modifying the compiler easy, then the above factors I think are working against the goal of the DSL.

A quick survey of some other compilers and their build systems: of LLVM, SpiderMonkey and Golang, all of which have some meta-build steps, only Golang checks in generated source (and in the Go community this seems to be a bit more idiomatic).

So, to me, the question becomes how to support the needs above (build time, debugging) in a different way, if we can. I'd like to see if we can:

Thoughts? cc @abrown @fitzgen

view this post on Zulip Wasmtime GitHub notifications bot (Apr 22 2022 at 17:55):

github-actions[bot] commented on issue #4066:

Subscribe to Label Action

cc @cfallin, @fitzgen

<details>
This issue or pull request has been labeled: "isle"

Thus the following users have been cc'd because of the following labels:

To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

view this post on Zulip Wasmtime GitHub notifications bot (Apr 22 2022 at 17:55):

fitzgen commented on issue #4066:

Nitpick: this isn't 100% true, since we tell github to automatically hide diffs for these files. You have to expand it yourself if you want to see the clutter.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 22 2022 at 17:58):

fitzgen commented on issue #4066:

A quick survey of some other compilers and their build systems: of LLVM, SpiderMonkey and Golang, all of which have some meta-build steps, only Golang checks in generated source (and in the Go community this seems to be a bit more idiomatic).

The difference is that most people using those compilers will be downloading pre-built .sos and aren't generally building from source and dealing with compile times themselves. With Rust and cargo, every embedder of Cranelift and Wasmtime is building this stuff from source themselves.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 22 2022 at 18:00):

fitzgen commented on issue #4066:

If we have a "developer mode", I'm assuming this would be a new cargo feature? But isn't that the same state of developer experience we are currently in, that you list as a downside earlier, where developers have to know to enable a cargo feature when hacking on ISLE?

view this post on Zulip Wasmtime GitHub notifications bot (Apr 22 2022 at 18:01):

fitzgen commented on issue #4066:

Add a means to print "tracing" messages (off by default) in generated code, to ease debugging of why rules did or did not match.

I think this is a good idea either way. Probably means yet another cargo feature, though.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 22 2022 at 18:08):

cfallin commented on issue #4066:

The difference is that most people using those compilers will be downloading pre-built .sos and aren't generally building from source and dealing with compile times themselves. With Rust and cargo, every embedder of Cranelift and Wasmtime is building this stuff from source themselves.

That's certainly true! This is why I think we can only really do this if we can cut down the build time of cranelift-isle. At least when initially developing it though, I found it to be really fast to build -- it didn't yet have deps on thiserror or miette, so the only external dep was log -- and I think that would be viable.

If we have a "developer mode", I'm assuming this would be a new cargo feature? But isn't that the same state of developer experience we are currently in, that you list as a downside earlier, where developers have to know to enable a cargo feature when hacking on ISLE?

Yeah, I'm going back and forth on this one, maybe we just don't need a special mode here.


Overall I think the strongest argument in favor of this is the rebase pain. I feel quite strongly actually that this is a speedbump that will turn people away, and it is not a process that will scale up if we want to scale up to more contributors. Checking in generated artifacts to source control is in general an antipattern that should be avoided unless the other advantages are really really compelling; so in my mind if we can cut down the build time delta then the main reason to do so evaporates.

Another argument I hadn't realized until today: there is a security/trust argument to be made against checked-in source. RIght now, the code that everyone builds with is generated by whoever last updated the ISLE, on their local machine. Since we don't review the generated source deltas, nothing is stopping a malicious user from tweaking something after making some other innocuous change to ISLE that forces a regenerate step. This alone is IMHO a reason why the current situation needs to change.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 22 2022 at 18:10):

fitzgen commented on issue #4066:

Ultimately, I agree that the pains you've listed are real and painful.

However, I'm not convinced that the proposed changes will ultimately lead to better outcomes. I'm especially wary of making changes that make the developer experience a little better for Cranelift developers at the cost of the developer experience for Cranelift embedders, since it will be pain we aren't feeling directly and therefore will be easy to unintentionally ignore, and we won't feel pressure to improve.

That said, we can remove thiserror from ISLE by copying in its cargo expand output and then removing the dependency. Not sure if miette also pulls in derive macro goop. I wanted to start using actual spans all over the place in ISLE rather than the stubbed, single-character spans we have now. My plan was to use miette everywhere for this. We could define our own thing, and hopefully it would do less than miette and have less code and get faster compiles as a result. That might work out.

But before we get into any of that, I think we should define what an acceptable build time hit to Cranelift would be acceptable, if we are going to seriously pursue this. I think <5% might be reasonable.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 22 2022 at 18:13):

cfallin commented on issue #4066:

However, I'm not convinced that the proposed changes will ultimately lead to better outcomes. I'm especially wary of making changes that make the developer experience a little better for Cranelift developers at the cost of the developer experience for Cranelift embedders, since it will be pain we aren't feeling directly and therefore will be easy to unintentionally ignore, and we won't feel pressure to improve.

So here I think it's a bit of a subjective call, but the way I see it is:

view this post on Zulip Wasmtime GitHub notifications bot (Apr 22 2022 at 18:33):

cfallin commented on issue #4066:

Addendum to above: not just "rebase pain", but the "how to add an instruction lowering for a newcomer" pain in general. If we have a convoluted process for all of this then it will be seen as arcane and inaccessible to someone who just wants to make a tweak. If the cost of making us friendlier to new contributors is ~5s of build time (let's predicate all of this on the assumption we can get there), then IMHO it's worth it. Consider what a newcomer goes through now:

Contrast to:

That's the experience I worry about above! We should be blind to adding 5 seconds to build time in our embedders, sure; but we should also not be blind to how friendly we are in our processes.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 22 2022 at 18:35):

cfallin edited a comment on issue #4066:

Addendum to above: not just "rebase pain", but the "how to add an instruction lowering for a newcomer" pain in general. If we have a convoluted process for all of this then it will be seen as arcane and inaccessible to someone who just wants to make a tweak. If the cost of making us friendlier to new contributors is ~5s of build time (let's predicate all of this on the assumption we can get there), then IMHO it's worth it. Consider what a newcomer goes through now:

Contrast to:

That's the experience I worry about above! We should not be blind to adding 5 seconds to build time in our embedders, sure; but we should also not be blind to how friendly we are in our processes.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 22 2022 at 18:36):

bjorn3 commented on issue #4066:

Can we check in sources for releases and not for the main branch?

view this post on Zulip Wasmtime GitHub notifications bot (Apr 22 2022 at 18:41):

cfallin commented on issue #4066:

@bjorn3 I appreciate the intent behind the suggestion but IMHO that's probably the worst of both worlds: we would retain the complexity of the current situation (because it's needed in some cases) and compound it with "something different happens in different builds". I'd prefer we develop and test with the same thing that end-users get, if we can make that viable.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 22 2022 at 19:37):

tschneidereit commented on issue #4066:

Another argument I hadn't realized until today: there is a security/trust argument to be made against checked-in source. RIght now, the code that everyone builds with is generated by whoever last updated the ISLE, on their local machine. Since we don't review the generated source deltas, nothing is stopping a malicious user from tweaking something after making some other innocuous change to ISLE that forces a regenerate step. This alone is IMHO a reason why the current situation needs to change.

Without taking a position on any of the other arguments here: I remember that at some point we talked about explicitly checking this in CI: we'd rebuild the source and verify that it's identical to what's checked in. If we do end up keeping checked in generated code, we absolutely should add this kind of check if we don't have it right now.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 22 2022 at 22:55):

abrown commented on issue #4066:

@tschneidereit, if I understand you correctly, I'm pretty sure that check exists today:

https://github.com/bytecodealliance/wasmtime/blob/6a36a1d15df303f533fc0dddc89012b1358b52d7/.github/workflows/main.yml#L199-L204

Catching up on this thread, I am actually torn: I don't really like the current rebase pain and extra hassle to execute the rebuild-isle feature but I definitely still remember the pain of the old backend code-generation-at-build. When I first stepped into that old backend, it was very difficult to understand what was going on. The most difficult part, I think, was how the meta-generated code interacted with the in-tree code; I think we have a bit of that same problem today with ISLE in that it is not quite obvious that a *.isle file generates a *.rs file that uses other *.rs files, etc. In summary: I think the "pain angle" that I have felt most clearly is of the "how does this code interact with that code?!?!" variety.

A suggestion: I wonder if several of these things disabled/enabled automatically not by explicitly using features but by either building in debug or release mode. I don't know how common this is in the Rust ecosystem, but it seems reasonable that we might want match logging for debug builds of Wasmtime but not for release builds. There might be other features (like the miette dependency tree) that could also be available only in debug mode?

view this post on Zulip Wasmtime GitHub notifications bot (Apr 22 2022 at 23:48):

cfallin commented on issue #4066:

Hmm, I'm not sure about debug-vs-release differences for the same reason I'm not sure about the suggestion above to do something different on a release branch: too much magic based on implicit state leads to hard-to-debug issues, IMHO. I'd rather have the thing behave predictably across configurations.

that check exists today

Indeed, I completely missed that we already have that. Sorry for the alarm there and I think today's status quo is acceptable, given that!

"how does this code interact with that code?!?!"

This question is I think super-important though, thank you for bringing it up -- we worked hard to make the output of the DSL compiler at least mostly human-readable, and I think we sort of have a split mind here about whether we want the generated Rust to be an opaque intermediate form (like an object file) or a real part of our codebase that we refer to. The answer to that question also affects how we might do other things in the future, fwiw (e.g. if there are questions around code optimizations that make the generated code harder to read but faster).

I'll also share that back in the day I kept a gen-cranelift symlink around to wherever in target/ the meta crate put the generated code, so I could read it more easily. It was confusing to me as well.

I'm happy to table this for now if it seems there isn't clear consensus to change; I am very worried about the speedbumps, and I find the rebases annoying and painful locally, but maybe we can wait to listen for stronger signal from others having difficulties before making changes.

view this post on Zulip Wasmtime GitHub notifications bot (May 04 2022 at 20:02):

cfallin labeled issue #4066:

In our most recent Cranelift meeting, we discussed the question of whether we want to continue to rely on checked-in, pre-generated ISLE DSL compiler output, or else move back to a build strategy that always builds from the DSL.

The two main advantages of the current situation are:

On the other hand, I believe that the following downsides are becoming more relevant:

The above factors are a mix of friction for newcomers (required understanding and complexity) and regular contributors (rebase pain). If the goal of the DSL is to make modifying the compiler easy, then the above factors I think are working against the goal of the DSL.

A quick survey of some other compilers and their build systems: of LLVM, SpiderMonkey and Golang, all of which have some meta-build steps, only Golang checks in generated source (and in the Go community this seems to be a bit more idiomatic).

So, to me, the question becomes how to support the needs above (build time, debugging) in a different way, if we can. I'd like to see if we can:

Thoughts? cc @abrown @fitzgen

view this post on Zulip Wasmtime GitHub notifications bot (May 05 2022 at 13:17):

sparker-arm commented on issue #4066:

I definitely find the current work flow painful, especially when I have multiple isle porting patches in flight. The diff produced is also mainly generated code, which as @cfallin pointed out, is probably just ignored in review. But it also has the negative side-effect of making it more awkward to view a patch, before pushing a PR, too.

I also wonder whether developers would be more inclined to prefer to add code to a specific backend, instead of in prelude.isle, just because of the extra rebase effort across all backends.

view this post on Zulip Wasmtime GitHub notifications bot (May 11 2022 at 00:25):

cfallin commented on issue #4066:

Alright, so my viewpoints are swinging back toward my original instincts here, after the recent confusion where our prebuilt-source mechanism caused a compiler change not to be picked up, and a bug to hide on main (and all of this not be caught by CI). @sparker-arm also reports here that he has spent as much time dealing with understanding the build system as actually writing patches. That's the wrong balance, I think, and we can expect others to feel this way too (and in the worst case, get confused and lost after trying something and never even show up on the GitHub project).

I really do think that the high-order bit is that we should follow the "principle of least surprise", and that would suggest here that source code in the tree is built when we build the project. Anything else is extra cognitive load and potential for bugs, confusion, lost contributors, etc.

So I want to take another look at what it would take to overcome the engineering problems of (i) build time, and (ii) hidden generated-source when one actually does want to look at it.

  1. I'm going to spend some time tomorrow seeing if I can factor miette out of the core ISLE compiler and into the commandline wrapper, so we have no dependencies other than log during the main build. Perhaps a Cargo feature could re-enable it so build.rs-sourced errors are prettified as they are now.
  2. @abrown re: "finding the source" and "setting breakpoints" concerns above, what would you think about a Cargo feature that puts the source in its current location (rather than target/...), but is off by default? We would respect the feature in two places: build.rs, and wherever we include! the file (for the latter perhaps taking an env var from build.rs for the "ISLE path"). I'm not sure why we didn't come across this point in the design space above, but it does seem quite simple and workable.

view this post on Zulip Wasmtime GitHub notifications bot (May 11 2022 at 17:37):

abrown commented on issue #4066:

Yup, the idea of being able to configure where the generated code is placed sounds like it resolves the debugging question. An alternate approach might be to add a CRANELIFT_GENERATED_ISLE_DIR environment variable for ultimate control. Either way, feature or environment variable, the tricky part of this is documenting the build option: in https://github.com/bytecodealliance/wasmtime/issues/4135, I propose we add a section with a "how to build Cranelift" guide which has this kind of thing (and why you would want to do it).

view this post on Zulip Wasmtime GitHub notifications bot (May 11 2022 at 17:38):

abrown edited a comment on issue #4066:

Yup, the idea of being able to configure where the generated code is placed sounds like it resolves the debugging question. An alternate approach might be to add a CRANELIFT_GENERATED_ISLE_DIR environment variable for ultimate control. Either way, feature or environment variable, the tricky part of this is documenting the build option in a place people actually look: in https://github.com/bytecodealliance/wasmtime/issues/4135, I propose we add a section with a "how to build Cranelift" guide which has this kind of thing (and why you would want to do it).

view this post on Zulip Wasmtime GitHub notifications bot (May 11 2022 at 20:55):

cfallin commented on issue #4066:

Small update: the core cranelift-isle crate with all deps (miette, thiserror, and even log) factored out under non-default features can build in 1.9s on my machine, vs. 4.9s with the current main. I'll incorporate this into the build.rs and remove the hashing, etc next (I expect it might even be break-even-ish since we won't need the hashing crate anymore).

view this post on Zulip Wasmtime GitHub notifications bot (May 12 2022 at 05:25):

cfallin closed issue #4066:

In our most recent Cranelift meeting, we discussed the question of whether we want to continue to rely on checked-in, pre-generated ISLE DSL compiler output, or else move back to a build strategy that always builds from the DSL.

The two main advantages of the current situation are:

On the other hand, I believe that the following downsides are becoming more relevant:

The above factors are a mix of friction for newcomers (required understanding and complexity) and regular contributors (rebase pain). If the goal of the DSL is to make modifying the compiler easy, then the above factors I think are working against the goal of the DSL.

A quick survey of some other compilers and their build systems: of LLVM, SpiderMonkey and Golang, all of which have some meta-build steps, only Golang checks in generated source (and in the Go community this seems to be a bit more idiomatic).

So, to me, the question becomes how to support the needs above (build time, debugging) in a different way, if we can. I'd like to see if we can:

Thoughts? cc @abrown @fitzgen


Last updated: Nov 22 2024 at 16:03 UTC