Skip to content

Conversation

@pavpanchekha
Copy link
Contributor

Test https://github.com/paulzzy/egg in Herbie. This is the "Undo Scheduler" described on Zulip which should make egg faster and hopefully also better.

@pavpanchekha
Copy link
Contributor Author

The unit tests are failing because I didn't update Cargo.lock, no worries there.

@pavpanchekha
Copy link
Contributor Author

@paulzzy

@pavpanchekha
Copy link
Contributor Author

Ok, here's the branch results vs baseline. @paulzzy you can use the diff feature to compare them if you'd like. Basically, there's almost no change in either results or runtime, which is a bummer.

This might be because Herbie egraphs are quite small (8k nodes) so perhaps the undo scheduler just never gets a chance to fire, I don't know.

@paulzzy
Copy link
Collaborator

paulzzy commented Oct 8, 2024

Thanks for trying it out! I'll spend some time looking into what's going on

@paulzzy
Copy link
Collaborator

paulzzy commented Oct 8, 2024

I'm surprised the tests passed because I haven't yet implemented support for explanations (which suggests it wasn't actually running, so I'll figure out how to fix that). Are explanations required by Herbie?

@pavpanchekha
Copy link
Contributor Author

Herbie uses explanations but with them unimplemented I imagine you just return empty explanations? Which Herbie knows to ignore.

@paulzzy
Copy link
Collaborator

paulzzy commented Oct 9, 2024

Currently it just panics with todo! (hence why I know for sure it's not running, because otherwise we'd get a bunch of errors), but if Herbie can accept empty explanations that'll make my life a lot easier :)

@pavpanchekha
Copy link
Contributor Author

Oh. Huh, then I wonder what this PR even did. Do you want to take a look at teh code? The egg side is tiny / trivial.

@paulzzy
Copy link
Collaborator

paulzzy commented Oct 10, 2024

Currently taking a look! Looks like it panics without explanations being enabled, so I'm figuring out how to bypass that

@pavpanchekha
Copy link
Contributor Author

@paulzzy I'm going to close this PR, since it's not going to be merged and to keep our PRs clean, but please feel free to fork & make PR when you're ready on your end.

@paulzzy
Copy link
Collaborator

paulzzy commented Oct 15, 2024

Thanks Pavel! I'll submit a PR once I get it working.

@pavpanchekha pavpanchekha deleted the undo-scheduler branch December 2, 2024 16:57
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