Skip to content

C++: Fix performance of bbEntryReachesLocally (1.19)#576

Merged
geoffw0 merged 1 commit into
github:rc/1.19from
jbj:bbEntryReachesLocally-perf
Nov 29, 2018
Merged

C++: Fix performance of bbEntryReachesLocally (1.19)#576
geoffw0 merged 1 commit into
github:rc/1.19from
jbj:bbEntryReachesLocally-perf

Conversation

@jbj

@jbj jbj commented Nov 29, 2018

Copy link
Copy Markdown
Contributor

This predicate was fast with the queries and engine from 1.18. With the queries from master it got a bad join order in the UninitializedLocal.ql query, which made it take 2m34s on Wireshark. This commit decomposes bbEntryReachesLocally into two predicates that together take only 4s.

This predicate was fast with the queries and engine from 1.18. With the
queries from `master` it got a bad join order in the
`UninitializedLocal.ql` query, which made it take 2m34s on Wireshark.
This commit decomposes `bbEntryReachesLocally` into two predicates that
together take only 4s.
@jbj jbj added the C++ label Nov 29, 2018
@jbj jbj added this to the 1.19 milestone Nov 29, 2018
@jbj jbj requested a review from a team as a code owner November 29, 2018 14:21
@geoffw0

geoffw0 commented Nov 29, 2018

Copy link
Copy Markdown
Contributor

LGTM, though at this stage I'd definitely like to give it a try before merging...

@geoffw0 geoffw0 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I have two wireshark snapshots on my machine. On one of them this speeds it up from 807s to 633s, by making bbEntryReachesLocally disappear from the list of most expensive predicates. The other snapshot apparently hangs around 520s with much less contribution from bbEntryReachesLocally (perhaps it's missing something pathological like a giant test case). In any case this seems to be doing what it promises.

@geoffw0 geoffw0 merged commit 4744cec into github:rc/1.19 Nov 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants