-
Notifications
You must be signed in to change notification settings - Fork 50
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
Improve Static FSM Compilation #2215
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, these tests are too big and probably never actually checked. @nathanielnrn is there a game plan to eventually deprecate them (nothing for @calebmkim to do on this in this PR).
Hey @calebmkim, can we get this PR in a mergeable state soon? I'm assuming this is the PR that adds the code that needs to be handed off to @parthsarkar17? |
Yep; I think I just need to add a few extra test cases, let @parthsarkar17 look over the code (if he wants and ask questions) and I think it'll be pretty close to mergeable. (I will add the test cases within the next couple of days) |
There’s a lot in this PR: the main files to look at (in decreasing order of importance) are: The code is quite complex. While some of this is “necessary complexity”, i.e., can be explained by the fact that we are just generating more complicated guards/wiring now. Here are some other sources of code complexity:
I think that once we merge this, it would be good for someone (maybe me) to do another iteration on this code and just clean it up a bit. I'm hesitant to do so in this PR because it will add yet more code to an already gigantic PR. |
@calebmkim thought on what the state of this PR is? We should get this merged before OOPSLA in case people end up playing with it afterwards. |
So there are some places where the code is perhaps more awkward/more verbose than it needs to be, but I also left a lot of comments in the code explaining (in detail) the decisions, so I think it should be decently readable for whoever works on this after me. I think it's probably fine to merge, though? |
Sounds good! Merging! |
Implements the ideas from #2207.
Apologies for the gigantic PR, there are a couple reasons why it is so big:
There are some minor changes to
static_inline.rs
(in particular, inliningstatic par
blocks is more complicated now because we can't merge always just merge all threads of astatic par
into the same group). There are some changes tocompile_static.rs
, but the main contribution of this PR is the filestatic_tree.rs
.I'm still going to write some tests to make sure I'm getting all edge cases for this new tree-looking FSM compilation.