Conversation
| expr = nd.asExpr() and | ||
| this.checks(expr, branch) and | ||
| exists(boolean branch | | ||
| this.checks(nd.asExpr(), branch) and |
There was a problem hiding this comment.
You previously factored this out in the noopt version. Do you need the pragma for the first argument of checks here, or does it perform fine without?
There was a problem hiding this comment.
The factoring out was to get the QL to compile as when using pragma[noopt] you have to fully specify the join order. It was not important for performance and, therefore, if we don't use that pragma and aren't required to do it we can use the neater QL where it is inlined.
There was a problem hiding this comment.
Ah! Got it. Indeed noopt cannot handle the inline expressions. Thanks for the explanation :)
| p.checkOn(pragma[only_bind_into](check), outcome, resNode) and | ||
| guard.ensures(pragma[only_bind_into](check), outcome) |
There was a problem hiding this comment.
Do you observe a performance problem without these last two pragmas? The previous change you made didn't touch the checkOn and ensures calls.
Attaching the pragma to both uses of a variable (here check) looks off to me, because the way I think of it, at least one place must be allowed to bind check.
There was a problem hiding this comment.
I might be misunderstanding the direction the pragma applies in, but isn't it the case that check is still being bound, but we aren't getting binding information out of check? In any case, I've done some profiling and it looks like the pragma on checkOn isn't essential for performance. The one on ensures, however, is - without it the evaluation time for this predicate jumps from 12.9s to 18.3s.
9166562 to
65a34b4
Compare
As @adityasharad pointed out on Slack, my solution of adding
pragma[noopt]on #547 was perhaps a bit too drastic a way to get the right join order. This changes to usepragma[only_bind_into]instead, which means that other potential optimisations that the compiler might want to do will not be inhibited, and we still get the desired join order with the new optimiser.cc. @github/codeql-core