Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

perf: add query optimizer #829

Merged
merged 5 commits into from
Nov 9, 2021
Merged

perf: add query optimizer #829

merged 5 commits into from
Nov 9, 2021

Conversation

williballenthin
Copy link
Collaborator

@williballenthin williballenthin commented Nov 8, 2021

please review and merge #828 and #827 before this.

This PR adds a rule optimizer that re-orders the nodes in the rule logic tree to try simpler/faster cases before complex cases. For example, it prefers OS checks before mnemonic checks before regex checks.

In practice, this seems to make a small, but measurable difference in execution time:

label count(evaluations) avg(time) min(time) max(time)
18c30e4 base 108,121 0.38s 0.34s 0.55s
a6e2cfc base short circuiting 69,401 0.23s 0.22s 0.26s
152d0f3 de-optimizer 78,900 0.28s 0.26s 0.36s
e287dc9 optimizer 68,275 0.22s 0.21s 0.25s

(via: PMA01-01, 30 iterations)

Note that originally, in 152d0f3, I had the sign of the cost function inverted, so the optimizer was actually a de-optimizer: it picked approximately the worst possible order of evaluation. This led to a 13% increase in feature evaluations, whereas the correct ordering improves evaluation performance by about 2%. The mistake demonstrates that evaluation order can have a substantial impact on performance, though our rules are already fairly well structured (e.g. we typically have OS checks as the first line).

My opinion is that we should probably merge this PR because it does provide some performance benefit, code is very localized, and it doesn't change any of our public APIs/behaviors.

Further perf metrics, using k32 (2 iterations):

label count(evaluations) avg(time) min(time) max(time)
18c30e4 base 66,561,622 177.59s 172.27s 182.92s
6909d6a optimizer 41,772,519 100.44s 99.38s 101.49s

About 44% faster with 38% fewer feature evaluations.

Checklist

  • No CHANGELOG update needed
  • No new tests needed
  • No documentation update needed

@williballenthin williballenthin added the enhancement New feature or request label Nov 8, 2021
Copy link
Collaborator

@mr-tz mr-tz left a comment

Choose a reason for hiding this comment

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

nice, only thing worth considering would be some test cases showing the optimizer works as expected

@mr-tz
Copy link
Collaborator

mr-tz commented Nov 9, 2021

I think for samples with many functions and large functions this could make a big difference (see K32 results).

@williballenthin williballenthin merged commit 84ba32a into master Nov 9, 2021
@williballenthin williballenthin deleted the perf/query-optimizer branch November 9, 2021 23:25
from capa.features.common import Arch, Bytes, Substring


def test_optimizer_order():
Copy link
Collaborator

Choose a reason for hiding this comment

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

great, thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants