-
Notifications
You must be signed in to change notification settings - Fork 10
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 an access-concatenate-varargs operator #86
base: main
Are you sure you want to change the base?
Conversation
17dd8b7
to
c5b41e4
Compare
c5b41e4
to
2cbc79c
Compare
I've added a new integration test which, after every egg runner iteration, determines which vararg lengths to create rewrites for, and adds those new rewrites to the list of rewrites to run. That works fine, however the rewrites never seem to fire. Perhaps I'm missing rewrites? Or I'm creating rewrites w/ the wrong number of variadic args? |
It seems like this is related to how I'm running egg. I am currently manually running one iteration at a time. Egg doesn't seem to like that. If I throw a bunch of vararg rewrites with different vaalues of |
Also, it seems like the slice-concatenate rewrite that uses vararg concat is super slow, which is something I ran into before. That is a pretty significant problem. It's why I switched to an incremental slice-concatenate rewrite, which would discover a single slice per iteration (or so I think). I thought the performance problem might be because I was using a tree of concatenates, which I thought we'd solve with the varargs approach, but apparently that's not the core issue. |
I'm not sure where to go with this. Maybe there's some way to make this more efficient. Maybe we need to take a more radical route and consider ways to represent things that don't require manifesting blocking so explicitly. |
It seems like the slowness is coming from the clone on this line: https://docs.rs/egg/0.6.0/src/egg/egraph.rs.html#375 |
It may just be that egg doesn't like nodes to be large. The larger a node is, the more children which need to be updated in that step, leading to more clones. |
Ideally I'd add a new end-to-end test that checks/proves (at least to me) that this method is faster than the old way.