-
Notifications
You must be signed in to change notification settings - Fork 567
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
filter-irrelevant-rule causes memory/perf blowup on some rules #7839
Comments
emjin
changed the title
filter-irrelevant-rule causes memory blowup on some rulesed
filter-irrelevant-rule causes memory/perf blowup on some rules
May 19, 2023
emjin
pushed a commit
that referenced
this issue
May 19, 2023
Closes #7839 In Analyze_rule, we distribute out formulas like `(x0 ^ x1) v (y0 ^ y1)` into cnf. Let's call `x0 ^ x1` p and `y0 ^ y1` q. Currently, we check that `p` and `q` are individually under 1,000,000 elements to prevent memory blowup. However, `p` and `q` produce a formula that is `p * q` elements long. So, even under these constraints, if `p` and `q` are both large, this step can still take too long. Instead, what we need to do is guard the result. This PR currently requires that that result be under 10,000,000 elements. It leaves the `p` and `q` restrictions intact just in case. This allows us to take advantage of `List.compare_length` in case we get an incredibly long `p` or `q`. To consider: do we still need the gates for `p` and `q`? Is it possible to get a list for `p` or `q` so large that traversing it is a problem? Test plan: will add tests
5 tasks
aryx
pushed a commit
that referenced
this issue
May 19, 2023
Closes #7839 In Analyze_rule, we distribute out formulas like `(x0 ^ x1) v (y0 ^ y1)` into cnf. Let's call `x0 ^ x1` p and `y0 ^ y1` q. Currently, we check that `p` and `q` are individually under 1,000,000 elements to prevent memory blowup. However, `p` and `q` produce a formula that is `p * q` elements long. So, even under these constraints, if `p` and `q` are both large, this step can still take too long. Instead, what we need to do is guard the result. This PR currently requires that that result be under 10,000,000 elements. It leaves the `p` and `q` restrictions intact just in case. This allows us to take advantage of `List.compare_length` in case we get an incredibly long `p` or `q`. To consider: do we still need the gates for `p` and `q`? Is it possible to get a list for `p` or `q` so large that traversing it is a problem? Test plan: will add tests PR checklist: - [x] Purpose of the code is [evident to future readers](https://semgrep.dev/docs/contributing/contributing-code/#explaining-code) - [x] Tests included or PR comment includes a reproducible test plan - [x] Documentation is up-to-date - [x] A changelog entry was [added to changelog.d](https://semgrep.dev/docs/contributing/contributing-code/#adding-a-changelog-entry) for any user-facing change - [x] Change has no security implications (otherwise, ping security team) If you're unsure about any of this, please see: - [Contribution guidelines](https://semgrep.dev/docs/contributing/contributing-code)! - [One of the more specific guides located here](https://semgrep.dev/docs/contributing/contributing/) --------- Co-authored-by: Emma Jin <--get>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Describe the bug
https://semgrep.dev/playground/s/E2B5 takes forever (> 13 minutes).
So does https://semgrep.dev/playground/s/LbgX (rule =
kotlin_faster_but_longer.yaml
, target =kotlin_slow_import.kt
). However, though this one is longer, it takes less time (2 minutes)The two rules are pretty similar; the shorter rule has deleted some patterns compared to the longer rule.
To Reproduce
Create files from the playground example.
Takes about 2 minutes.
Then, try running without filter-irrelevant-rules.
Expected behavior
Expect the optimized version to not be significantly slower.
What is the priority of the bug to you?
Environment
If not using semgrep.dev: are you running off docker, an official binary, a local build?
Use case
What will fixing this bug enable for you?
The text was updated successfully, but these errors were encountered: