Skip to content

Conversation

aschackmull
Copy link
Contributor

The local flow relation was accidentally made mutually recursive with AdditionalValueStep. It makes sense to allow the use for local flow to define AdditionalValueStep::step, so removing the latter from the former allows this without recursion.

A non-monotonic dummy reference is added to prevent re-introduction of recursion here, and this highlighted the need for a minor refactor in FlowSummaryImpl (since ReturnNodeExt depends on local flow).

The local flow relation is also fixed to force the use of the fastTC hop, as this otherwise was observed to result in a very slow materialisation.

@aschackmull aschackmull added the no-change-note-required This PR does not need a change note label May 17, 2022
@aschackmull aschackmull requested review from a team as code owners May 17, 2022 13:30
Copy link
Contributor

@atorralba atorralba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks plausible to me, just one minor comment.

There are a couple of tricks (namely fastTC and (any() or strictcount(Node n1, Node n2, AdditionalValueStep a | a.step(n1, n2)) < 0)) that I would like to learn more about at some point, though :)

@aschackmull
Copy link
Contributor Author

There are a couple of tricks (namely fastTC and (any() or strictcount(Node n1, Node n2, AdditionalValueStep a | a.step(n1, n2)) < 0)) that I would like to learn more about at some point, though :)

fastTC just means the same as the + suffix on a predicate, but forces the compiler to use the specialised code for this (using + leaves the choice to the optimiser, which will often also result in using fastTC, but didn't in this case (and that happened to be bad)).
The other ugly line of obscure QL is a hack. It doesn't really matter much what it contains as and (any() or ...) optimises away to nothing at all, but a predicate reference inside a strictcount can't be recursive, so we ensure that we don't introduce accidental recursion in the future through this hack.

@aschackmull aschackmull force-pushed the java/perf-local-flow branch from f682f3e to a4a004a Compare May 18, 2022 07:28
@hvitved
Copy link
Contributor

hvitved commented May 18, 2022

There are some large regressions for Ruby; could be because of the missing pragmas mentioned above.

@aschackmull
Copy link
Contributor Author

DCA looks good now.

@aschackmull aschackmull merged commit 8beef45 into github:main May 20, 2022
@aschackmull aschackmull deleted the java/perf-local-flow branch May 20, 2022 10:38
owen-mc added a commit to owen-mc/codeql that referenced this pull request Nov 9, 2022
owen-mc added a commit to owen-mc/codeql that referenced this pull request Nov 9, 2022
owen-mc added a commit to owen-mc/codeql that referenced this pull request Nov 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants