Conversation
|
I'd like to get this into 1.20 since it fixes a performance regression introduced in 1.20. |
|
To get this into 1.20, either wait for #1032 to be merged and then retarget; or rebase the branch onto |
cpp/ql/src/Critical/NewDelete.qll
Outdated
There was a problem hiding this comment.
What's the need for cached here? Before this PR, multiple uses of this library took very little extra time, so caching was not needed. See, for example, this log from https://lgtm.com/projects/g/fawkesrobotics/fawkes/logs/rev/c497588cba4b36ae7a24a8e387444d1837866f71/lang:cpp/stage:Build%20child_c497588cba4b36ae7a24a8e387444d1837866f71
[EVALUATION 29/145] Completed query semmlecode-cpp-queries/Critical/NewArrayDeleteMismatch.ql (198s)
[EVALUATION 30/145] Completed query semmlecode-cpp-queries/Critical/NewDeleteArrayMismatch.ql (8s)
[EVALUATION 31/145] Completed query semmlecode-cpp-queries/Critical/NewFreeMismatch.ql (5s)
There was a problem hiding this comment.
I don't know exactly why this helps but it seems to:
NewFreeMismatch.ql on fawkesrobotics_fawkes with cached: 226s
NewFreeMismatch.ql on fawkesrobotics_fawkes without cached: 410s
It may just be that it forces things to be computed in a better order, and if so there may be a better way to accomplish this.
There was a problem hiding this comment.
You can get most of the medicinal effects of cached by using pragma[nomagic] instead.
There was a problem hiding this comment.
Yep, that works just as well. Updated.
|
Retargetted onto rc/1.20. |
jbj
left a comment
jbj
left a comment
There was a problem hiding this comment.
LGTM, and performance is good.
2 participants
This PR rearranges some of the logic in
NewDelete.qllto obtain better performance. Correctness is also slightly better (in obscure cases).@jbj @nickrolfe