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

Push Pop API #300

Draft
wants to merge 21 commits into
base: main
Choose a base branch
from
Draft

Push Pop API #300

wants to merge 21 commits into from

Conversation

dewert99
Copy link
Contributor

This is another attempt at #290 that builds on #296.

I first added a push/pop API to RawEGraph that depends on an additional generic parameter. I ended up with two implementation strategies, and as I wasn't sure which one was better I implemented both, the generic parameter can also be instantiated with () to disable push/pop statically, or Option to do it dynamically. The push function for RawEGraph returns a PushInfo which needs to be passed into pop so that the API can compose better with other similar APIs without each one needing its on Vec<PushInfo>.

While I was working on this implementation I noticed that I could easily make memo append-only by having it nodes to the first Id that is congruently equivalent instead of the most recent one. My first thought was to convert memo into an IndexMap so it could be reverted by calling pop until it reached its old size. Unfortunately I noticed this had slightly worse performance than a HashMap. Instead I implemented my own DHashMap using a hashbrown::HashTable, it has performance characteristics similar to a HashMap but it stores the insertion order index along with each key/value pair. This allows it to efficiently remove the last element (given it's hash value). It can also iterate in insertion order, but this is more expensive that IndexMap and requires allocation.

I added push and pop to EGraph and made it controlled dynamically with with_push_pop_enabled and with_push_pop_disabled (similar to explanations). I currently added a feature to pick between the two implementations I created, although it might make more sense just to pick one. These methods require N::Data: Default so I could temporarily have classes without data without making it an Option or MaybeUninit. I needed N::Data: Clone so I could keep a log of copies old versions to revert to during pop. Unfortunately I had to make this a bound in Analysis (Breaking change ⚠️) since clone is called in union and set_analysis_data (not push or pop).

To test the new API I added a test-push-pop feature (similar to test-explanations) that also adds a hook to call push and make a clone of the EGraph at each iteration. Afterwards it repeatedly calls pop and each times checks if the EGraph matches its corresponding clone.

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.

1 participant