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 an access-concatenate-varargs operator #86

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

gussmith23
Copy link
Owner

@gussmith23 gussmith23 commented Nov 13, 2020

  • Add integration test which uses new syntax
    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.

@gussmith23 gussmith23 force-pushed the concatenate-varargs branch 5 times, most recently from 17dd8b7 to c5b41e4 Compare November 24, 2020 14:32
@gussmith23
Copy link
Owner Author

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?

@gussmith23
Copy link
Owner Author

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 num_args in to the rewrite list and set the iter limit >1, more rewrites get run.

@gussmith23
Copy link
Owner Author

image

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.

@gussmith23
Copy link
Owner Author

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.

@gussmith23
Copy link
Owner Author

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

@gussmith23
Copy link
Owner Author

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.

@gussmith23 gussmith23 added language The design of the Glenside language itself rewrites Improve Glenside's egraph rewrites labels Dec 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
language The design of the Glenside language itself rewrites Improve Glenside's egraph rewrites
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant