C++: inline arithTypesMatch predicate#3316
Conversation
This predicate is effectively a Cartesian product between all enum types. It's infeasible to compute it in full, so luckily the optimizer has been able to apply enough magic to make it feasible. That's not a robust solution, and it has indeed broken on at least one version of the 1.24 release candidate. On a Chromium snapshot where I ran the LGTM suite overnight, the `m#MistypedFunctionArguments::arithTypesMatch#bb` predicate (magic for `arithTypesMatch`) took 170m5s. That was commit b69fdf5 from the internal repo. I tried to reproduce it in VSCode, this time with commit 646646, but it wasn't quite as bad: the predicate took only 38 seconds. In any case, making the problematic predicate `pragma[inline]` removes the slow magic and makes the `MistypedFunctionArguments.ql` query faster.
|
The predicate was slow again on this night's run of the full query suite. Even though I still can't reproduce the problem when running just a single query, I now think the safest thing to do is to merge this PR. I've started a CPP-Differences job to check for regressions: https://jenkins.internal.semmle.com/job/Changes/job/CPP-Differences/1061/ |
|
Without this commit, this is what the profile viewer shows for Chromium with one thread and 32 GB RAM. Yellow is
This PR should remove the giant blue column in the middle. Among the blue columns on the right, the three tallest are the three stages of the IR. The others mostly correspond to the handful of unpaired yellow columns spread around the chart, plus a few new cached stages introduced by #3189 and #2204. |
geoffw0
left a comment
There was a problem hiding this comment.
LGTM.
We've been discussing this on Slack - I cannot reproduce a case where arithTypesMatch is slow, but the change looks sensible and has no negative effects in my testing. @jbj has reproduced the improvement several times as I understand it.
|
The CPP-Differences job has finished. I think it only shows noise (meaning no change) since the per-query stats show that the only two affected queries are effectively as fast/slow as they were before. |
|
I'm running the full suite overnight on Chromium with this PR (and #3339). That should provide direct evidence of whether this PR solves the problem in the one setup where I can reproduce it. |
|
This PR had the intended effect on my overnight Chromium test. |
WAIT: I'm starting to wonder whether this bug is real or whether it was just my laptop that froze up for 170 seconds. I wasn't at the keyboard at the time. I'll repeat the experiment tonight and only take this PR out of draft if the problem reproduces.This predicate is effectively a Cartesian product between all enum types. It's infeasible to compute it in full, so luckily the optimizer has been able to apply enough magic to make it feasible. That's not a robust solution, and it has indeed broken on at least one version of the 1.24 release candidate.
On a Chromium snapshot where I ran the LGTM suite overnight, the
m#MistypedFunctionArguments::arithTypesMatch#bbpredicate (magic forarithTypesMatch) took 170m5s. That was commit b69fdf5 from the internal repo. I tried to reproduce it in VSCode, this time with commit 646646, but it wasn't quite as bad: the predicate took only 38 seconds. In any case, making the problematic predicatepragma[inline]removes the slow magic and makes theMistypedFunctionArguments.qlquery faster.