-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
bug fix: stop all kinds of expressions from cnf-exploding #14585
Conversation
Signed-off-by: Andres Taylor <andres@planetscale.com>
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to look at benchmarks for this @systay, the O complexity of this new code is pathological because after each rewrite, you need to walk the whole AST to count the nodes.
Since we control the rewrite patterns, I think it would be much more efficient to keep track of the amount of added & removed nodes during the rewrite. The tracking doesn't need to be fully accurate, just enough to detect unbounded growth, so it should be very easy to implement!
Yeah, you are right. This was sloppy. I'll work on it |
Signed-off-by: Andres Taylor <andres@planetscale.com>
Signed-off-by: Andres Taylor <andres@planetscale.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…cases Signed-off-by: Andres Taylor <andres@planetscale.com>
Hello! 👋 This Pull Request is now handled by arewefastyet. The current HEAD and future commits will be benchmarked. You can find the performance comparison on the arewefastyet website. |
Signed-off-by: Andres Taylor <andres@planetscale.com>
…4585) Signed-off-by: Andres Taylor <andres@planetscale.com>
…4585) Signed-off-by: Andres Taylor <andres@planetscale.com>
…4585) Signed-off-by: Andres Taylor <andres@planetscale.com>
…4585) Signed-off-by: Andres Taylor <andres@planetscale.com>
Description
In a recent change (#14560) we fixed a bug in the CNF rewriter that stopped it from rewriting a lot of cases.
Unfortunately, we succeeded too well, and the newly rewritten expressions are timing out in expression fuzzing on CI. This is because CNF rewriting in some situations result in huge outputs (see Wikipedia)
This PR stops the rewriting once the produced expression has grown to 10 times the size of the original expression.
Related Issue(s)
Fixes #14569
Checklist