Skip to content
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

Merged
merged 40 commits into from
Jan 6, 2023
Merged

Conversation

countvajhula
Copy link
Collaborator

@countvajhula countvajhula commented Sep 16, 2022

Summary of Changes

A WIP branch to explore adding bindings to Qi. Specifically, an as form.

((☯ (~> (-< (~> list (as vs))
            +)
        (~a "The sum of " vs " is " _)))
 1 2)
;=> "The sum of (1 2) is 3"

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

  • In contributing, I relinquish any copyright claims on my contribution and freely release it into the public domain in the simple hope that it will provide value.

@countvajhula countvajhula mentioned this pull request Sep 16, 2022
29 tasks
@countvajhula countvajhula temporarily deployed to test-env September 16, 2022 05:09 Inactive
@countvajhula countvajhula temporarily deployed to test-env September 16, 2022 05:11 Inactive
@benknoble
Copy link
Collaborator

Can I recommend using a box (probably with make-variable-like-transformer for referencing later) for the expanded code? IIUC, some compiler optimizations are turned off when set! appears in expanded code, and using a box with related procedures means that set! won't be in the expansions.

Similarly, it might be nice to use undefined as the initial value, so that (assuming -< is evaluated left-to-right)

((flow (~> list (-< vs (as vs)))))

gives an error about vs being undefined rather than evaluating to #<void>.

@countvajhula countvajhula force-pushed the lets-write-a-qi-compiler branch from 06c0e24 to d8616bc Compare September 24, 2022 20:31
@countvajhula countvajhula temporarily deployed to test-env September 24, 2022 20:37 Inactive
@countvajhula countvajhula temporarily deployed to test-env September 30, 2022 16:59 Inactive
@countvajhula countvajhula temporarily deployed to test-env September 30, 2022 17:02 Inactive
@benknoble
Copy link
Collaborator

Based on some of the tests, it looks like there's a couple of breaking (?) changes:

  • (flow f) now needs to be written (flow (esc f)) so that f is not mistaken for a qi-introduced binding; is that right? Or do naked identifiers continue to work in flows?
  • (flow (g x)) where g has arity 1 is now an error?

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 :)

@countvajhula
Copy link
Collaborator Author

Only parenthesized expressions are affected, so naked identifiers should continue to work unless I've introduced a bug.

(flow (g)) is now an error since parenthesized expressions are always treated as partial application, and partially applying to zero arguments would be equivalent to not partially applying (so technically we needn't raise an error, but it may be more helpful to do so, if this is expected to be a mistake on the part of the user?).

(flow (g x)) where g has arity 1 is now an error?

If this were invoked with no arguments, it would be fine, but otherwise it would be an error since it's essentially treated as (flow (g __ x)).

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 curry was evaluating its arguments immediately, before the binding was actually available, in something like this:

((flow (~> (-< (~> 3 (as v))
               _)
           (+ v))) 5)

... as that would try something like (curry + v).

We changed it to a lambda to delay the evaluation of the partially supplied arguments, to something like (lambda args (apply + v args)).

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.

@benknoble
Copy link
Collaborator

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?

@countvajhula
Copy link
Collaborator Author

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

@benknoble
Copy link
Collaborator

@benknoble This PR is ready for review! Michael and I paired on it today and got the bindings implementation working for all the cases we could identify (viz binding single and multiple values, and not clobbering Racket forms that happen to be named as that may be embedded via esc 😃 ). [Today's meeting notes]

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.)

Re: your example from earlier:

((flow (~> list (-< vs (as vs)))))

... we handled this by reporting a nice error message whenever as is used outside ~>. Although, now that I'm looking at it again, we do have a containing ~> here and that should probably be acceptable, since we may wish to bind intermediate values in a flow by (~> (-< bind-it do-something-with-it) continue-doing-things ... use-the-bound-value-downstream). So, maybe reporting this error here was a bit hasty ...

Hm, so as is only legal as a direct child of ~>? That does seem restrictive, but maybe not in a bad way? I don't know the as semantics well, but if it also produces the bound value, then your example would be (~> bind-it do-something-with-it …). If not, then it does seem the only way to use and bind it is some version of (~> (fanout 2) (== (~> bind-it) use-it)) …)?

Re: using box instead of set!, Michael pointed out that this is the kind of optimization that would typically be the province of the Scheme compiler, and we agreed that we should only make the change if we are sure that it makes a difference, and otherwise do whatever the simplest thing is at our level of abstraction. Wdyt? We could also create an issue to investigate it down the line if we can't find concrete data on this right away.

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 set! binding in expanded code is supposed to be enough to disable some kinds of optimizations, and using boxes avoids the set! in the expansion. (Then again, doesn't letrec use set!?) I think the idea to check later is probably good, though it could be hard to compare the performance of the machine-code-compiled versions: one would set! directly, the other would call a library function.

@countvajhula
Copy link
Collaborator Author

Sounds good, do what you need to do! ☃️

Hm, so as is only legal as a direct child of >? That does seem restrictive, but maybe not in a bad way? I don't know the as semantics well, but if it also produces the bound value, then your example would be (> bind-it do-something-with-it …). If not, then it does seem the only way to use and bind it is some version of (> (fanout 2) (== (> bind-it) use-it)) …)?

Yes, having it propagate the value by default is an option too. With the grounding version, (effect (as v)) could be an idiom for propagating it.

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 set! binding in expanded code is supposed to be enough to disable some kinds of optimizations, and using boxes avoids the set! in the expansion. (Then again, doesn't letrec use set!?) I think the idea to check later is probably good, though it could be hard to compare the performance of the machine-code-compiled versions: one would set! directly, the other would call a library function.

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.

@benknoble
Copy link
Collaborator

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 set! binding in expanded code is supposed to be enough to disable some kinds of optimizations, and using boxes avoids the set! in the expansion. (Then again, doesn't letrec use set!?) I think the idea to check later is probably good, though it could be hard to compare the performance of the machine-code-compiled versions: one would set! directly, the other would call a library function.

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).

@countvajhula
Copy link
Collaborator Author

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.
@countvajhula countvajhula temporarily deployed to test-env December 15, 2022 20:06 — with GitHub Actions Inactive
@countvajhula countvajhula temporarily deployed to test-env December 15, 2022 20:06 — with GitHub Actions Inactive
@countvajhula countvajhula temporarily deployed to test-env December 16, 2022 01:57 — with GitHub Actions Inactive
@countvajhula countvajhula temporarily deployed to test-env December 16, 2022 01:57 — with GitHub Actions Inactive
@countvajhula
Copy link
Collaborator Author

Amazingly enough, we fixed all the issues that the new tests uncovered in today's meeting (moved up from Friday). Meeting notes.

@benknoble
Copy link
Collaborator

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 ./install-branch, assuming you don't already have Qi installed (a variant of the command in ./uninstall can remove it). The install-branch script accepts an optional "qi-branch" argument that defaults to this branch. Run ./uninstall to uninstall Qi and the repo, then ./install-main to install with "stable" Qi.

The following commits are of interest:

  • In benknoble/advent2021@f20d052, I make all the necessary changes to cope with the breaking changes introduced by this branch. Thoughts: I needed esc more than I would have liked, and the need to adjust for not and count becoming syntax errors when used as base functions was a mild inconvenience. Many changes do result in slightly clearer code, though. Technically all the changes I made work with stable Qi, too, so that's a plus.
  • In benknoble/advent2021@06c6985 I converted some Qi to equivalent Racket because the new Qi was significantly slower (I don't have numbers offhand). I can't tell if this is due to the compiler branch missing some optimizations present in the old version or not, but the major win was switching to filter-map. The for/sum conversion did not appear to significantly alter performance. I think the filter-map conversion is covered in part by Deforestation / "transducers" for values and lists #77, which could possibly mean my old code becomes equally performant. I don't have time right now to investigate the difference in expansions, though.
  • In benknoble/advent2021@8ec6a23 I tweak performance for an already slow algorithm. I point it out not because Qi was the problem, but because the easiest way to increase performance was to not hold the entire cartesian product in memory—this is easier to accomplish with for forms than with Qi.

@countvajhula
Copy link
Collaborator Author

@benknoble Nice, that's great data to have! I haven't tried your install scripts but they look handy.

Re: the need for esc, looking at those examples, I'm inclined to agree with your assessment that it's more than I would expect but also none of those cases is surprising upon inspection. I'm also wondering whether the pattern of a function that returns a function which you were formerly using without esc could be a good fit in general for being implemented as macros instead. That is, since the purpose of Qi syntax is to specify a function, it's possible that using a Racket function to return a function is, in a way, using Racket to do what Qi is supposed to do. So maybe in such cases, we should consider extending Qi syntax instead.

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?

@benknoble
Copy link
Collaborator

benknoble commented Jan 3, 2023

Re: the need for esc, looking at those examples, I'm inclined to agree with your assessment that it's more than I would expect but also none of those cases is surprising upon inspection. I'm also wondering whether the pattern of a function that returns a function which you were formerly using without esc could be a good fit in general for being implemented as macros instead. That is, since the purpose of Qi syntax is to specify a function, it's possible that using a Racket function to return a function is, in a way, using Racket to do what Qi is supposed to do. So maybe in such cases, we should consider extending Qi syntax instead.

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 escs would disappear, then, I think.

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!

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.)

Re: the for/fold and cartesian product, think there's a compiler optimization to be found in there?

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 for forms never has the whole product in memory (only the original lists and one element of each at a time), which is a big win. The product wasn't computed via Qi, but as an input to Qi (IIRC). So I'm not sure if there's room for Qi to optimize one to the other, since Qi would have to recognize racket/list's cartesian-product binding.

@countvajhula countvajhula temporarily deployed to test-env January 6, 2023 07:59 — with GitHub Actions Inactive
@countvajhula countvajhula temporarily deployed to test-env January 6, 2023 08:02 — with GitHub Actions Inactive
@countvajhula
Copy link
Collaborator Author

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 escs would disappear, then, I think.

That's another interesting explanation.

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.)

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.

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 for forms never has the whole product in memory (only the original lists and one element of each at a time), which is a big win. The product wasn't computed via Qi, but as an input to Qi (IIRC). So I'm not sure if there's room for Qi to optimize one to the other, since Qi would have to recognize racket/list's cartesian-product binding.

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 syntax-spec so that you and any other reviewers can actually be in a position to understand what it's supposed to be doing at that stage 😄

@countvajhula countvajhula merged commit 1df49ac into lets-write-a-qi-compiler Jan 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants