-
Notifications
You must be signed in to change notification settings - Fork 13
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
Add Bindings to Qi #75
Conversation
Can I recommend using a Similarly, it might be nice to use ((flow (~> list (-< vs (as vs))))) gives an error about |
e1199ea
to
7488804
Compare
06c0e24
to
d8616bc
Compare
These changes are what we did in last time's Qi meetup. There are still a few issues and cases to work out (as noted in the meeting notes on the wiki) but it roughly works.
Based on some of the tests, it looks like there's a couple of breaking (?) changes:
I'm not sure I grok the changes to existing tests and why they are related here… if you have time to explain it, I'll happily review :) |
Only parenthesized expressions are affected, so naked identifiers should continue to work unless I've introduced a bug.
If this were invoked with no arguments, it would be fine, but otherwise it would be an error since it's essentially treated as This was done mainly to avoid ambiguity in cases like these. These changes aren't really related to bindings, except that the rule for partial application was failing with bindings because the implementation with
... as that would try something like We changed it to a So, I guess there are two changes conflated here that were undertaken mainly because partial application was being rewritten anyway. Btw note this PR is still WIP due to the issues mentioned in the notes here, but of course input at any stage is always welcome. |
Ok. I still need to understand the ramifications, but I haven't totally grokked it yet. For the moment I'll probably just wait 'til it's in a more functional state? |
Yeah, that sounds like a good idea. I appreciate your keeping a watchful eye! FYI this latest commit was from today's Qi meetup (was moved from the usual time this time around): https://github.com/drym-org/qi/wiki/Qi-Compiler-Sync-Nov-29-2022 |
Great! Once I finish resolving an unrelated problem with https://github.com/benknoble/frosthaven-manager (which uses Qi!), I'll try to install this copy and run my https://github.com/benknoble/advent2021 tests, since that's a lot of Qi. I'll also try to run the Frosthaven Manager, too, if the tests pass (but I don't have many automated tests for FHM). (Why? I'd like to see if I used Qi in any way that turns out to be broken in the new version with bindings.)
Hm, so
Yeah, my comments mostly had to do with the fact that I think Racket and Chez are more likely to do the opposite or nothing at all—the presence of the |
Sounds good, do what you need to do! ☃️
Yes, having it propagate the value by default is an option too. With the grounding version,
I don't really know much about boxes, but it sounds like they are Racket's way of allowing us to be explicit about mutation, and I assume it's allowing us to do that for a reason. Maybe it's saying, look, I'm not going to optimize things if you use mutation since that's too much trouble. But if you meet me halfway and let me know when you're going to be mutating things, I'll see what I can do. Is it anything like this? It reminds me of how we're thinking about accidental side effects in the Qi compiler and making no promises about them, in which case, I can definitely empathize with this point of view. But in any case, yeah it'd be great if we could come up with a small code example that we could run to compare the two. In general, too, having such "proofs" of performance optimizations would be very useful as we optimize things. |
I think this is close, but I'm no expert here. Effectively I think the optimizer sees mutation and backs off: "whoa, I'm not sure if these optimizations are allowed anymore around this variable." On the other hand, a box wraps a value: the box is mutable in the sense that you can change what you put in it, but the contents of the box need not be; it also hides the mutation from the compiler (since the box's identity doesn't change, either). |
I added more tests for bindings. Looks like we still have some work to do 🙂 |
This is the same issue as we were seeing with partial application's use of the `curry` form in its implementation, which required that the arguments be available at compile time. We fixed it in the same way, by wrapping the implementation in a lambda that accepts the runtime arguments, allowing the use of bound identifiers in the feedback specification.
Amazingly enough, we fixed all the issues that the new tests uncovered in today's meeting (moved up from Friday). Meeting notes. |
Update: I completed testing and necessary tweaks to run https://github.com/benknoble/advent2021 with this branch of Qi. You can do it yourself, too, if you're interested, by cloning the repo and (inside it) running The following commits are of interest:
|
@benknoble Nice, that's great data to have! I haven't tried your install scripts but they look handy. Re: the need for Re: slowness, it's good to know that the performance difference is apparent in actual code using Qi! The criteria for merging the compiler integration branch into main include (1) at least as good as a priori performance on individual forms, and (2) beat Racket on at least one nonlocal benchmark (I'm thinking deforestation will probably be it here). So, I wouldn't expect the performance to remain a problem when this is all merged into main. Worst case, we promote some forms back to core forms, although that would complicate writing optimizations of course. Better case, use a series of "peephole" optimizations to do one-off restorations, but those may not suffice to restore performance to a priori levels. Even better case, we jump through hoops (maybe attach syntax properties?) to identify specific cases where the expander expanded a non-core but standard form, and use that information to reliably restore the a priori implementation of that form in the compiler. On the one hand this seems like just undoing work that we did, but on the other hand it does seem to allow us to both have good local performance of individual forms while also having a small and tractable core language for writing nonlocal optimizations, so it could be worth doing. Other ideas welcome! Re: the for/fold and cartesian product, think there's a compiler optimization to be found in there? |
I think mostly what happened is that I expressed certain cases via curried functions, when with Qi it's quite natural to not explicitly write curried functions, and let Qi take care of the partial application instead. Many of those
There's a lot of interesting ideas in there. I think your last idea in that paragraph is a reasonable "filler," but isn't what I would hope for long term. (Why? It requires us to have been smart enough with the original expansions in terms of performance. As we evolve the language or the compiler, we ought to learn or develop new information that improves the performance beyond its origins.)
Unclear. I think my issue mostly was keeping the whole product in memory (it was large), and searching a large space of such products. The streaming solution via |
use new syntax-spec racket-expr feature
That's another interesting explanation.
Good point. In today's meeting @michaelballantyne pointed out that annotating the forms with information during expansion is exactly the same as inflating our core language, just calling it a different name, since the same information would be there but in a different form. @AlexKnauth pointed out that one case where such annotations could make sense is if some compilers treat it as a core language and use the information, while other compilers disregard the annotations, in which case we could still consider it as not inflating the core language. But that's not our case, so in the end we concluded that it makes the most sense to just bring these forms back in as core forms. That's outside the scope of this PR though so I can start a separate PR for it.
Yeah that sounds like it would be overreach into the Racket compiler's domain at that point. Anyhow thank you for checking all that! In the meeting we decided to merge this PR and defer a more in-depth review stage to the end on the integration branch. Michael is planning to add docs for |
Summary of Changes
A WIP branch to explore adding bindings to Qi. Specifically, an
as
form.Bindings could be useful in simple cases of eliminating incidental nonlinearity in the flow, and may also support cases where flow paths would otherwise "intersect" one another and so could not easily be done without bindings.
This is work towards #36 .
Public Domain Dedication