Skip to content

Comments

C++: Rewrite reachable without mutual recursion#170

Closed
jbj wants to merge 1 commit intogithub:masterfrom
jbj:reachable-perf
Closed

C++: Rewrite reachable without mutual recursion#170
jbj wants to merge 1 commit intogithub:masterfrom
jbj:reachable-perf

Conversation

@jbj
Copy link
Contributor

@jbj jbj commented Sep 7, 2018

This gives a measurable speedup, and I think it makes the code clearer. Before this, the reachable and successors_adapted predicates were mutually recursive together with four other predicates over two files.

I measured these benchmarks, where the numbers came from subtracting timestamps in the log rather than trusting the "Clause timing report" at the end:

  • wireshark: reduced from 204s to 121s.
  • linux: reduced from 59s to 36s.
  • postgres: reduced from 28s to 12s.

I tested correctness and performance with this query, which gives the same results before and after:

select strictcount(ControlFlowNode node | reachable(node))

These changes were supposed to be a refactoring to enable even more ambitious speedups of the reachable predicate, but they bring large enough improvements on their own that I'd like to submit them as a separate PR. It's not clear whether it's worth following up with changes to make the code faster but more complex.

The performance issue with reachable is that it has a recursive occurrence on both sides of an and. That was hidden before but is visible after this refactoring. It's more expensive to compute such a general case of recursion than the usual case where one side of every and is constant.

This gives a measurable speedup, and I think it makes the code clearer.
Before this, the `reachable` and `successors_adapted` predicates were
mutually recursive together with four other predicates over two files.

This adds `successors_adapted` to the global name space, which is not
really desired but not harmful either.
@jbj jbj added the C++ label Sep 7, 2018
@jbj
Copy link
Contributor Author

jbj commented Sep 8, 2018

The test failure looks like a real problem. I'll look into it.

@jbj
Copy link
Contributor Author

jbj commented Sep 9, 2018

Notes to self:

The successors_adapted can't work the way I've written it here. It has to check pred, not succ for the sake of code like if (...) { aborting() } else { not_aborting() } next_stmt;. Maybe it should use the aborting predicate.

Since aborting and abortingFunction are not cached, might this cause ConstantExprs.qll to be recomputed in the BasicBlocks stage under memory pressure? That's both before and after this PR.

@jbj jbj added the WIP This is a work-in-progress, do not merge yet! label Sep 9, 2018
@jbj
Copy link
Contributor Author

jbj commented Sep 10, 2018

This branch was not quite ready for PR. Closing.

@jbj jbj closed this Sep 10, 2018
aibaars pushed a commit that referenced this pull request Oct 14, 2021
Add `rb/overly-permissive-file` query
smowton pushed a commit to smowton/codeql that referenced this pull request Jan 17, 2022
Java/Kotlin: Tweak consistency queries
MathiasVP pushed a commit to MathiasVP/ql that referenced this pull request Aug 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C++ WIP This is a work-in-progress, do not merge yet!

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant