C++: Only propagate smallest/largest range bound in conditional expressions #18461
Conversation
paldepind
force-pushed
the
cpp-conditional-expr-range-analysis
branch
from
5dbc273 to
dcdc439
Compare
There was a problem hiding this comment.
Copilot reviewed 2 out of 6 changed files in this pull request and generated no comments.
Files not reviewed (4)
- cpp/ql/lib/semmle/code/cpp/rangeanalysis/SimpleRangeAnalysis.qll: Language not supported
- cpp/ql/test/library-tests/rangeanalysis/SimpleRangeAnalysis/ternaryLower.expected: Language not supported
- cpp/ql/test/library-tests/rangeanalysis/SimpleRangeAnalysis/ternaryUpper.expected: Language not supported
- cpp/ql/test/library-tests/rangeanalysis/SimpleRangeAnalysis/test.c: Language not supported
Tip: Copilot only keeps its highest confidence comments to reduce noise and keep you focused. Learn more
| @@ -389,6 +389,28 @@ unsigned int test_ternary02(unsigned int x) { | |||
| return y1 + y2 + y3 + y4 + y5; | |||
| } | |||
|
|
|||
| // Test that nested ternary expressions of literals doesn't cause performance blow up. | |||
| double test_ternary_nested_of_literals(double m, double n, double o, double p, double q) { | |||
There was a problem hiding this comment.
Do we have another way to test against performance regressions? This test adds around 2 minutes of execution time on my machine prior to the PR, and runs with "normal" performance after. So the idea is that any regression would be noticed in the test time.
There was a problem hiding this comment.
This test is perfectly fine. By default there's a 5 min timeout limit on codeql test run so you could make the test take ~5 min prior to your fix (which would write something like "timeout" in the expected file) which would surely make it clear that there's a regression if we ever broke this again.
You can rewrite this into a macro and nest it a couple of times to make the test arbitrarily enormous :)
We normally don't add regression tests for performance issues. Ideally, we would add a DCA project that exhibit these performance problems, but this was only seen internally at Microsoft this is a bit more tricky.
There was a problem hiding this comment.
I've now added two more lines of nested ternary operators, which should grow the old execution time by a factor of 36 and get us well above the timeout.
In order to reproduce the blow up it's important that the constants are all very different and doesn't repeat. Otherwise the sums of the bounds can end up producing duplicates, and the blow up gets reduced. Given that I'm not sure about how to use a macro, so I've left the repetition be :)
There was a problem hiding this comment.
That's a good point! If you want to get really creative you can use __COUNTER__ macro to get a fresh constant for each macro invocation 😂 But since it didn't take many more iterations to get above the 5 min limit it's probably not worth it.
| @@ -1594,7 +1603,7 @@ private module SimpleRangeAnalysisCached { | |||
| * the lower bound of the expression after all the casts have been applied, | |||
| * call `lowerBound` like this: | |||
| * | |||
| * `lowerBound(expr.getFullyConverted())` | |||
| * lowerBound(expr.getFullyConverted()) | |||
There was a problem hiding this comment.
The indentation already turns on code highlighting, so the `'s just end up looking wonky in the rendered markdown.
paldepind
force-pushed
the
cpp-conditional-expr-range-analysis
branch
from
c3184aa to
e9f2a8b
Compare
The goal of this PR is to fix a performance problem related to the ternary operator. The added test demonstrates the issue.
As a fix, this PR changes how bounds are propagated for the ternary operator: Currently, for
c ? e1 : e2the union of the bounds fore1ande2are propagated. With this PR the bounds are combined, with the smallest/greatest (for lower bounds/upper bounds respectively) being propagated.Using
|e|to denote the numbers of bounds for an expression, this PR changes|c ? e1 : e2|from|e1| + |e2|to|e1| * |e2|. This is good when the number of bounds is 1, and worse when the number of bounds is>= 3. This solves the original problem, as the nested ternary operators of literals ends up with a single bound.This could make certain cases perform worse, but I'm not too worried as this change just gives
? :the same characteristics as+,-, etc. DCA doesn't show anything bad either.