Conversation
06c0e24 to
d8616bc
Compare
|
Current performance comparison of individual forms with a priori (main branch) performance (repeated the same analysis that was done a little while back): Summary: The criteria for displaying a relative performance factor other than 1 was r < 0.75x or r > 1.5x. (This was done on the on the latest |
To be fair, the one's that got slower mostly got slow by large factors (>=4, >=40)! |
|
Yeah it's just to give us an idea of progress rather than a report card. I'd love to get them all to <= 1 prior to merging this! |
|
Alrighty, we're getting the ball rolling on this again. I'll go ahead and merge #95 and then we can return to #76 and aim to Beat Racket™️ on one benchmark. That would bring us to the close of the compiler project where we can do the final review and merge this work, and that would then set us up for further optimizations over time on the main branch 🙂 |
|
Hello everyone! You may have noticed that the We are targeting the solstice, i.e. Dec 22, as the release date for Qi 4 -- a little under two weeks -- and it coincides with a Qi meeting as it's on a Friday. If you have time to review this PR, that would be much appreciated. Since it's such a huge PR, here's a quick overview of things as a refresher -- you could probably also refer to the meeting notes over the last year for additional context and the general story of how we got here. Overview of new operation: At a high level, when we write a flow, it first expands that expression to the Qi core language (e.g. Where is all this happening in the code, you might ask? Well, I've updated the source code layout wiki to include a comprehensive overview of every module in the source tree, for your reference. You could try things locally if you like (after checking out this branch): If you have any Qi codebases (esp. @benknoble and @NoahStoryM ), now is a great time to test those against this compiler branch. There are some known backwards incompatibilities, and if you happen to run into any unknown ones that would be good to know 😆 Here are some specific items that may need review:
There are still a few items we'd like to do before release:
That's all I can think of for the moment. I'll aim to keep the PR description up to date with outstanding items. Thanks! Let's get this compiler out into the wild! 🐺 🐅 🌲 |
|
The two major things I'm going to go look for are
Of course, I've got to get my install switched over to this branch for that to happen and, well, last time I recall that took me some effort. But this time I'll try out the command that converts a package install to a Git clone, if I can point it to my existing clone. P.S. It would probably be good to take care of the merge conflicts sooner rather than later. |
|
@benknoble If you can also look at normalization that'd be great! Last time you found a bunch of issues with it, soooo, I think it would give us more confidence if you signed off on it 😄 I'm also curious how the switch-over goes with the install. IME it's been a matter of uninstalling any existing version of Qi (likely with And re: merge conflicts, agreed, I'm planning to rebase this branch soon (I will give a heads up). |
benknoble
left a comment
There was a problem hiding this comment.
Some normalization comments.
qi-lib/flow/core/normalize.rkt
Outdated
| [(thread _0 ... (amp f) (amp g) _1 ...) | ||
| #'(thread _0 ... (amp (thread f g)) _1 ...)] |
There was a problem hiding this comment.
This is not generally sound (I'm not sure if this is the smallest example):
> (~> (3) (>< (-< add1 sub1)) (>< (-< sub1 add1)))
3
5
1
3
> (~> (3) (>< (~> (-< add1 sub1) (-< sub1 add1))))
sub1: arity mismatch;
the expected number of arguments does not match the given number
expected: 1
given: 2
[,bt for context]
qi-lib/flow/core/normalize.rkt
Outdated
| [(relay (~datum _) ...) | ||
| #'_] |
There was a problem hiding this comment.
This is more permissive than current Qi:
> (~> (3) (relay _ _ _))
relay: arity mismatch;
the expected number of arguments does not match the given number
expected: 0
given: 2
[,bt for context]
qi-lib/flow/core/normalize.rkt
Outdated
| [(thread _0 ... (~datum _) _1 ...) | ||
| #'(thread _0 ... _1 ...)] |
There was a problem hiding this comment.
I believe this is covered by a previous case on line 46.
qi-lib/flow/core/deforest.rkt
Outdated
| (define (prettify-flow-syntax stx) | ||
| (syntax-parse stx | ||
| #:datum-literals (#%host-expression esc #%blanket-template #%fine-template) | ||
| (((~literal thread) |
There was a problem hiding this comment.
I'd like to know why this thread has to be literal; in the rest of the compiler it's matched by datum AFAICT. Though perhaps
qi/qi-lib/flow/core/compiler.rkt
Lines 85 to 89 in 98c82c1
thread with a binding (from racket)?
I remembered the trick by checking the docs. I went to Qi clone and checked out the main branch (really the commit hash needs to match, or it needs to be possible to |
Address code review comments
|
Hi folks, if you're still planning to review this, please get your reviews in by tomorrow. If there are no further blockers, we will aim to merge this on Friday morning PST 😁 . At this stage, if you have comments that aren't blockers, they may be addressed after release, but they are still appreciated. Thanks! |
|
We don't anticipate further code changes before tomorrow's release, so now is a great time to test against this branch with Frosthaven (@benknoble ), Qi-Cat (@NoahStoryM ), Relation (@countvajhula ) and Qi-Circuit (@chansey97 ). For instructions on how to test against a specific repo branch, this comment (and the quoted text) may be helpful. Thanks! 😄 |
|
I had to pass I can confirm that, after also |
|
Ah, please ignore the dependency warnings. I misunderstood the output of |
Update docs for Qi 4
Summary of Changes
Integration branch for the compiler work. This will eventually be merged into main when the compiler is ready.
See Qi Compiler Meeting Notes.
Work Remaining
for-labelkw-helperis being correctly used in left- and right-currying cases (see#%blanket-templatecode generation incompiler.rkt) [Ben/Noah?]WIP
Already Done
(reverse-chronological, most recently completed at the top)
relationlibrary [Sid]esc(and implicitly escaped like just usingdisplayln),effectandgenshould be honored (do these interact with deforestation or normalization?) [Ben/Dominik?]- [ ] Add docs for syntax-specPre-Merge Checklist
all-defined-out-- see all-defined-out does not work with for-space racket/racket#4154)relationlibrary)Public Domain Dedication