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:
- The build time of
cranelift-codegen
is low: there is only a little bit ofbuild.rs
work (themeta
) crate, but we don't need to build the ISLE compiler every time.- The generated source is present in the main tree and can be examined and debugged easily, to see why rules are or are not matching.
On the other hand, I believe that the following downsides are becoming more relevant:
- The workflow in general is very awkward. If we want to be friendly to newcomers who might want to tweak some instruction lowerings -- one of the main goals of ISLE, long-term -- then consider what understanding we assume:
- We require the developer to build a mental model of the complex multi-step build with checked-in artifacts, which is unusual and unidiomatic in the Rust world;
- We require a special Cargo flag to force a generated-source rebuild;
- If there is ever more than one PR outstanding at a time to the ISLE DSL source, all but the first to merge will need to regenerate and rebase (most of us probably take understanding
git
for granted but this is a nontrivial speedbump for many);- The diffs that one sees on GitHub are cluttered with generated-source changes, which I have mostly learned to subconsciously ignore when reviewing but are potentially pretty confusing for onlookers.
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:
- Find a way to reduce the dependencies of
cranelift-isle
so it can be built in a few seconds at most. We usemiette
andthiserror
(all for good reasons, to be clear!) and these pull in a bunch of (i) derive-macro stuff and (ii) fancy terminal stuff formiette
. The latter can be removed when not in a "developer mode" but the derive-macros seem to go pretty deep. I don't think we should get rid ofmiette
-- its errors are fantastic -- but could we refactor its use out toislec
, propagating a more generic and minimal error-plus-span type out ofisle
? Ideally the crate tree used bybuild.rs
should have justcranelift-isle
and its one other deplog
.- Add a means to print "tracing" messages (off by default) in generated code, to ease debugging of why rules did or did not match.
Thoughts? cc @abrown @fitzgen
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:
- The build time of
cranelift-codegen
is low: there is only a little bit ofbuild.rs
work (themeta
) crate, but we don't need to build the ISLE compiler every time.- The generated source is present in the main tree and can be examined and debugged easily, to see why rules are or are not matching.
On the other hand, I believe that the following downsides are becoming more relevant:
- The workflow in general is very awkward. If we want to be friendly to newcomers who might want to tweak some instruction lowerings -- one of the main goals of ISLE, long-term -- then consider what understanding we assume:
- We require the developer to build a mental model of the complex multi-step build with checked-in artifacts, which is unusual and unidiomatic in the Rust world;
- We require a special Cargo flag to force a generated-source rebuild;
- If there is ever more than one PR outstanding at a time to the ISLE DSL source, all but the first to merge will need to regenerate and rebase (most of us probably take understanding
git
for granted but this is a nontrivial speedbump for many);- The diffs that one sees on GitHub are cluttered with generated-source changes, which I have mostly learned to subconsciously ignore when reviewing but are potentially pretty confusing for onlookers.
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:
- Find a way to reduce the dependencies of
cranelift-isle
so it can be built in a few seconds at most. We usemiette
andthiserror
(all for good reasons, to be clear!) and these pull in a bunch of (i) derive-macro stuff and (ii) fancy terminal stuff formiette
. The latter can be removed when not in a "developer mode" but the derive-macros seem to go pretty deep. I don't think we should get rid ofmiette
-- its errors are fantastic -- but could we refactor its use out toislec
, propagating a more generic and minimal error-plus-span type out ofisle
? Ideally the crate tree used bybuild.rs
should have justcranelift-isle
and its one other deplog
.- Add a means to print "tracing" messages (off by default) in generated code, to ease debugging of why rules did or did not match.
Thoughts? cc @abrown @fitzgen
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:
- cfallin: isle
- fitzgen: isle
To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.
Learn more.
</details>
fitzgen commented on issue #4066:
- The diffs that one sees on GitHub are cluttered with generated-source changes, which I have mostly learned to subconsciously ignore when reviewing but are potentially pretty confusing for onlookers.
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.
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
.so
s and aren't generally building from source and dealing with compile times themselves. With Rust andcargo
, every embedder of Cranelift and Wasmtime is building this stuff from source themselves.
fitzgen commented on issue #4066:
- The latter can be removed when not in a "developer mode"
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 acargo
feature when hacking on ISLE?
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.
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 onthiserror
ormiette
, so the only external dep waslog
-- 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.
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 itscargo expand
output and then removing the dependency. Not sure ifmiette
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 usemiette
everywhere for this. We could define our own thing, and hopefully it would do less thanmiette
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.
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:
- First, we should cut down the cost of rebuilding always; the more we can do this, the more the "pain to Cranelift embedders" side of the balance goes away, and it's an unambiguous win;
- Second, they are different kinds of pains. Several seconds of build time is real, for sure. But process friction, and need for human effort and managing complexity, is a whole different kind of pain. "Machines are cheap, humans are expensive"; if we make developing Cranelift harder, then we will have fewer people developing Cranelift, and in the end we'll have a worse compiler.
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:
- There's a pattern-matching DSL; cool, I can add a new pattern for my "multiply by pi with this one cute bit-twiddling trick" lowering
cargo test
; huh, I see an error message. I have to add a Cargo feature? OK, copy/paste that command- It works, cool, let's create a PR.
git add .../lower.isle; git commit
- Huh, that's weird, CI turns red. Oh, do I need to add the generated source too?
- commit that too, push
- In the meantime, this jerk @cfallin comes along and makes a change to unrelated lowerings, gets a review, merges it
- Now there's a merge conflict. Do I need to go through and fix the source somehow?
- No, OK, now I need to
git fetch upstream
, merge, see the message about merge conflicts, ignore it (we'll be regenerating that source anyway), remember how to regenerate source again (I don't get a nice error message telling me--features rebuild-isle
here), do that, commit- OK, finally, I can push, someone reviews, I merge
Contrast to:
- Change the ISLE DSL code, commit, create PR
- Someone reviews
- I can merge as long as there are no conflicts in the
.isle
file itselfThat'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.
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:
- There's a pattern-matching DSL; cool, I can add a new pattern for my "multiply by pi with this one cute bit-twiddling trick" lowering
cargo test
; huh, I see an error message. I have to add a Cargo feature? OK, copy/paste that command- It works, cool, let's create a PR.
git add .../lower.isle; git commit
- Huh, that's weird, CI turns red. Oh, do I need to add the generated source too?
- commit that too, push
- In the meantime, this jerk @cfallin comes along and makes a change to unrelated lowerings, gets a review, merges it
- Now there's a merge conflict. Do I need to go through and fix the source somehow?
- No, OK, now I need to
git fetch upstream
, merge, see the message about merge conflicts, ignore it (we'll be regenerating that source anyway), remember how to regenerate source again (I don't get a nice error message telling me--features rebuild-isle
here), do that, commit- OK, finally, I can push, someone reviews, I merge
Contrast to:
- Change the ISLE DSL code, commit, create PR
- Someone reviews
- I can merge as long as there are no conflicts in the
.isle
file itselfThat'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.
bjorn3 commented on issue #4066:
Can we check in sources for releases and not for the main branch?
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.
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.
abrown commented on issue #4066:
@tschneidereit, if I understand you correctly, I'm pretty sure that check exists today:
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?
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 intarget/
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.
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:
- The build time of
cranelift-codegen
is low: there is only a little bit ofbuild.rs
work (themeta
) crate, but we don't need to build the ISLE compiler every time.- The generated source is present in the main tree and can be examined and debugged easily, to see why rules are or are not matching.
On the other hand, I believe that the following downsides are becoming more relevant:
- The workflow in general is very awkward. If we want to be friendly to newcomers who might want to tweak some instruction lowerings -- one of the main goals of ISLE, long-term -- then consider what understanding we assume:
- We require the developer to build a mental model of the complex multi-step build with checked-in artifacts, which is unusual and unidiomatic in the Rust world;
- We require a special Cargo flag to force a generated-source rebuild;
- If there is ever more than one PR outstanding at a time to the ISLE DSL source, all but the first to merge will need to regenerate and rebase (most of us probably take understanding
git
for granted but this is a nontrivial speedbump for many);- The diffs that one sees on GitHub are cluttered with generated-source changes, which I have mostly learned to subconsciously ignore when reviewing but are potentially pretty confusing for onlookers.
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:
- Find a way to reduce the dependencies of
cranelift-isle
so it can be built in a few seconds at most. We usemiette
andthiserror
(all for good reasons, to be clear!) and these pull in a bunch of (i) derive-macro stuff and (ii) fancy terminal stuff formiette
. The latter can be removed when not in a "developer mode" but the derive-macros seem to go pretty deep. I don't think we should get rid ofmiette
-- its errors are fantastic -- but could we refactor its use out toislec
, propagating a more generic and minimal error-plus-span type out ofisle
? Ideally the crate tree used bybuild.rs
should have justcranelift-isle
and its one other deplog
.- Add a means to print "tracing" messages (off by default) in generated code, to ease debugging of why rules did or did not match.
Thoughts? cc @abrown @fitzgen
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.
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.
- 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 thanlog
during the main build. Perhaps a Cargo feature could re-enable it sobuild.rs
-sourced errors are prettified as they are now.- @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 weinclude!
the file (for the latter perhaps taking an env var frombuild.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.
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).
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).
cfallin commented on issue #4066:
Small update: the core
cranelift-isle
crate with all deps (miette
,thiserror
, and evenlog
) factored out under non-default features can build in 1.9s on my machine, vs. 4.9s with the currentmain
. I'll incorporate this into thebuild.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).
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:
- The build time of
cranelift-codegen
is low: there is only a little bit ofbuild.rs
work (themeta
) crate, but we don't need to build the ISLE compiler every time.- The generated source is present in the main tree and can be examined and debugged easily, to see why rules are or are not matching.
On the other hand, I believe that the following downsides are becoming more relevant:
- The workflow in general is very awkward. If we want to be friendly to newcomers who might want to tweak some instruction lowerings -- one of the main goals of ISLE, long-term -- then consider what understanding we assume:
- We require the developer to build a mental model of the complex multi-step build with checked-in artifacts, which is unusual and unidiomatic in the Rust world;
- We require a special Cargo flag to force a generated-source rebuild;
- If there is ever more than one PR outstanding at a time to the ISLE DSL source, all but the first to merge will need to regenerate and rebase (most of us probably take understanding
git
for granted but this is a nontrivial speedbump for many);- The diffs that one sees on GitHub are cluttered with generated-source changes, which I have mostly learned to subconsciously ignore when reviewing but are potentially pretty confusing for onlookers.
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:
- Find a way to reduce the dependencies of
cranelift-isle
so it can be built in a few seconds at most. We usemiette
andthiserror
(all for good reasons, to be clear!) and these pull in a bunch of (i) derive-macro stuff and (ii) fancy terminal stuff formiette
. The latter can be removed when not in a "developer mode" but the derive-macros seem to go pretty deep. I don't think we should get rid ofmiette
-- its errors are fantastic -- but could we refactor its use out toislec
, propagating a more generic and minimal error-plus-span type out ofisle
? Ideally the crate tree used bybuild.rs
should have justcranelift-isle
and its one other deplog
.- Add a means to print "tracing" messages (off by default) in generated code, to ease debugging of why rules did or did not match.
Thoughts? cc @abrown @fitzgen
Last updated: Nov 22 2024 at 16:03 UTC