perf(signatures): memoise the evented dispatch set, plus three correctness fixes - #3234
Open
doomedraven wants to merge 1 commit into
Open
doomedraven wants to merge 1 commit into
doomedraven wants to merge 1 commit into
Conversation
…tness fixes
RunSignatures.run() rebuilt the same set algebra on every API call: two
unions and three intersections, four intermediate sets, per call. Three of
the operands are loop-invariant and the result depends only on
(api, category) - a few hundred API names by twenty-odd categories - while
the loop runs once per API call, commonly 100k-5M times per analysis.
The cache for exactly this was already declared. self.api_sigs is
initialised in __init__ and documented as "Cache of signatures to call per
API name", but nothing ever read or wrote it. It is now wired up. The cache
is cleared per process because `sigs` is process-dependent.
Measured with the real RunSignatures.run(), 200k calls, 200 no-op evented
signatures (bench drives the actual dispatcher, not a reimplementation):
original 4.14 s / 4.14 s / 4.41 s
patched 2.42 s / 2.39 s / 2.40 s
That is dispatch overhead only. Real on_call bodies are unchanged, so the
end-to-end gain is proportionally smaller.
The filter buckets are built once in __init__ from class attributes that are
never mutated at runtime, so a memoised entry cannot go stale.
Also fixed, all in the same area:
- statistics.signatures[].time reported `round(timediff, 3)`, a variable
leaking out of the preceding on_complete loop, instead of `value`, the
signature's own accumulated time. Every evented signature was reported
with whatever duration the last measured signature happened to take.
- TTP de-duplication tested `{"ttp": ..., "signature": ...} not in self.ttps`,
a linear scan of a growing list of dicts, building the dict literal twice
per iteration, inside a list comprehension used only for its side effect.
Replaced with a set of (ttp, signature) pairs mirroring self.ttps.
- RunReporting.__init__ used `break` where `continue` belongs. The check is
per-process, so the first process whose calls came from a JSON reprocess
abandoned the whole loop and left any remaining ParseProcessLog instances
without begin_reporting() and unconverted.
- Removed the dead `if not hasattr(self.results, "debug")` guard in
RunProcessing.run(). self.results is a dict, so it was always False and the
append always ran. Rewriting it as `"debug" not in self.results` would
start dropping those errors once anything else had written to
results["debug"], so the condition is dropped and behaviour is unchanged.
TAG=agy
CONV=803339f8-9160-458e-b33a-f8319cfdb51e
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Sixth of the performance series (#3229 network, #3230 behavior, #3231 machinery, #3232 elastic/jsondump, #3233 mongo). This one is
lib/cuckoo/core/plugins.py.1. The evented dispatch loop recomputed loop-invariant set algebra per API call
RunSignatures.run():Four intermediate sets, two unions and three intersections, per API call. Three operands never change for the lifetime of the loop:
self.call_for_api.get("any", set())self.call_for_cat.get("any", set())evented_set.intersection(self.call_always)self.call_for_api.get(api, set())apiself.call_for_cat.get(cat, set())catThe result depends solely on
(api, category)— a few hundred API names by twenty-odd categories — while the loop runs once per API call, commonly 100k–5M times per analysis.Note
The cache for exactly this is already in the code.
self.api_sigs = {}is initialised in__init__and described in the class docstring as "Cache of signatures to call per API name", but nothing in the repository ever read or wrote it. It was designed and never wired up. This PR wires it up.self.api_sigs.clear()per process is required:sigsis process-dependent, so a cache carried across processes would dispatch the wrong signature set for the second process onward. There is a test for that specifically.Measured
The benchmark drives the real
RunSignatures.run()— not a reimplementation of the algebra — with 200,000 calls, 400 distinct API names, 20 categories and 200 evented signatures whoseon_callis a no-op:~1.73x, i.e. ~8.7 µs of dispatch overhead removed per call.
Important
This is dispatch overhead only. Real
on_callbodies are unchanged, so the end-to-end gain on a live analysis is proportionally smaller — the more work the signatures themselves do, the smaller the share.Risk: the filter buckets (
call_for_api,call_for_cat,call_for_processname,call_always) are built once in__init__fromfilter_apinames/filter_categories/filter_processnames, which are class attributes defaulting toset()inabstracts.pyand never mutated in place anywhere in the repo. A memoised entry therefore cannot go stale. The cached value is afrozenset; the inner loop only iterates it.2. Bug: every evented signature reported the same execution time
valueis the signature's accumulated time. It is computed, tested in theif, then discarded: the appendedtimeistimediff, a variable leaking out of the precedingon_completeloop. Every evented signature was reported with whatever duration the last measured signature happened to take, which makes these numbers useless for finding slow signatures.round(timediff, 3)→round(value, 3).Also dropped the
if sig.name not in stats: stats[sig.name] = 0re-check inside the hot loop —stats[sig.name] = 0is already set for every evented signature immediately above the loop, and every signature reachable throughcall_sigscomes fromevented_list.3. O(n²) TTP de-duplication
[ self.ttps.append({"ttp": ttp, "signature": sig.name}) for ttp in sig.ttps if {"ttp": ttp, "signature": sig.name} not in self.ttps ]Each membership test is a linear scan of a growing list of dicts, the dict literal is built twice per iteration, and a list comprehension is used purely for its side effect. Two identical copies of this, one in the evented branch and one in the non-evented branch.
Replaced with
_add_ttps()backed by asetof(ttp, signature)pairs mirroringself.ttps. Insertion order ofself.ttpsis preserved (tested). Measured 8.50 ms → 0.27 ms at 300 matched signatures — small in absolute terms, worth doing while the file is open.4.
breakwherecontinuebelongsRunReporting.__init__:The condition identifies one process whose calls came from a JSON reprocess rather than a live
ParseProcessLog.breakabandons the whole loop on the first such process. Processes are populated uniformly in practice, so today this is equivalent tocontinue— but only by accident. If the list is ever mixed, the remainingParseProcessLoginstances never getbegin_reporting()and reach the reporting modules unconverted.5. Dead guard removed
RunProcessing.run()hadif not hasattr(self.results, "debug"):.self.resultsis a plain dict, so that is alwaysFalseand the append always ran.Warning
The apparent fix —
if "debug" not in self.results— would be a behaviour change: it would start dropping the "behavioral log too big" errors whenever anything else had already written toresults["debug"], which is common. The condition is removed instead, keeping thesetdefaultchain and the current behaviour, with a comment recording why.Tests
tests/test_plugins_dispatch.py, 7 tests. They monkeypatchlist_pluginsrather than callingregister_plugin, so the fake signatures cannot leak intotests/test_signature.py's global registry.RunSignaturesinstance buckets; the dispatch recorded by each signature during a realrun()must match it exactly. 3 processes x 60 calls x 8 signatures covering every filter combination (none, api, cat, proc, api+cat, api+proc, cat+proc, all three).(api, category)pair.statistics.signatures[].timereports each signature's own time — fails on master._add_ttpsdedups, preserves order, and handles the empty and single cases.Negative control: with
lib/cuckoo/core/plugins.pyreverted to master, 5 of the 7 fail.The existing
tests/test_signature.py(21 end-to-end cases overtests/test_data/*/reports/report.json) passes unchanged, which is the real equivalence check on the filter semantics.ruff checkandblack --checkclean.TAG=agy