Repository navigation
Speed up compute_simhash and correct executor placements - #3331
Conversation
waf_bypass was submitting compute_simhash on full response bodies through the process pool. xxhash releases the GIL and simhash truncates its input to 3 KB internally, so the pickle-to-worker cost dominated the actual work, and any wedged worker could stall the whole scan for 300 s before helper._reset_process_pool recovered. bevigil.collapse_urls had the same profile at smaller scale. Neither operation needs process isolation. Move both to run_in_executor_cpu so a hung callable can no longer trigger a pool reset. Kreuzberg PDF extraction and badsecrets retain run_in_executor_mp where crash isolation is load-bearing.
📊 Performance Benchmark Report
📈 Detailed Results (All Benchmarks)
🎯 Performance Summary+ 1 improvement 🚀
! 1 regression ⚠️
29 unchanged ✅🔍 Significant Changes (>10%)
🐍 Python Version 3.11.15 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #3331 +/- ##
=====================================
- Coverage 90% 90% -0%
=====================================
Files 450 450
Lines 46426 46428 +2
=====================================
- Hits 41682 41678 -4
- Misses 4744 4750 +6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
singlerider
left a comment
There was a problem hiding this comment.
Benchmarked both call sites on 4 cores with bbot's real pool sizes (_mp=3 procs, _cpu=8 threads). Wall time and peak whole-tree RSS measured in the same run, fresh pool per cell, median of 5 reps after a discarded warmup:
| workload | in-flight | _mp time |
_mp mem |
_cpu time |
_cpu mem |
|
|---|---|---|---|---|---|---|
| simhash 3KB | 1 | 51.9ms | +108.1M | 106.7ms | +11.5M | 2.05x slower |
| simhash 3KB | 16 | 125.9ms | +109.5M | 1701.6ms | +11.7M | 13.51x slower |
| simhash 3KB | 64 | 330.2ms | +109.5M | 6555.5ms | +11.9M | 19.86x slower |
| simhash 500KB | 1 | 54.6ms | +111.7M | 127.9ms | +12.0M | 2.34x slower |
| simhash 500KB | 16 | 80.6ms | +113.9M | 1696.9ms | +12.2M | 21.07x slower |
| simhash 500KB | 64 | 299.7ms | +114.1M | 6443.8ms | +12.3M | 21.50x slower |
| collapse_urls 1k | 1 | 232.1ms | +111.9M | 583.3ms | +14.4M | 2.51x slower |
| collapse_urls 1k | 4 | 366.8ms | +114.0M | 4046.3ms | +14.5M | 11.03x slower |
| collapse_urls 10k | 1 | 777.0ms | +125.3M | 5415.1ms | +26.0M | 6.97x slower |
Memory is the real win here, and the description doesn't mention it. ~110MB vs ~12MB, roughly 9x, since three spawned interpreters cost ~36MB each. That's a solid argument on its own and probably explains the -11.1% on Memory Use Parallel Chains in the benchmark bot's report.
Two claims in the description don't hold up:
IPC isn't the cost. 3KB and 500KB give the same times. A 167x larger payload costs the process pool nothing measurable, so pickle isn't what's being paid for.
The GIL isn't released. xxhash is a small fraction of the work. compute_simhash truncates to 3072B, which yields ~2500 shingles, and the vector[i] += 1 if bit else -1 loop then runs ~160k pure-Python iterations holding the GIL. That's what threads serialize on.
waf_bypass.check_ip runs under as_completed(coros, max_concurrent=100), so the concurrent rows are the production path. bevigil inherits _module_threads = 1, so only its n=1 row applies, where it's 2.5-7x slower.
Pool wedging is worth fixing, and 100MB is worth saving. But this pays for both with 2x at n=1 and up to 21x concurrent. Two options that keep the memory win without the throughput loss: a shorter _timeout= on the _mp call, or vectorizing the bit loop (numpy, or int.bit_count over packed hashes) so _cpu is actually correct rather than just cheaper to recover from.
Benchmark script
#!/usr/bin/env python
"""A/B: run_in_executor_mp (process pool) vs run_in_executor_cpu (thread pool).
Measures wall time AND peak tree RSS in the same run. Exits 0."""
import os, sys, time, asyncio, statistics, warnings
from concurrent.futures import ProcessPoolExecutor, ThreadPoolExecutor
warnings.filterwarnings("ignore")
import psutil
from bbot.core.helpers.simhash import compute_simhash
from bbot.core.helpers.validators import collapse_urls
CORES = os.cpu_count() or 4
MP_W = max(1, CORES - 1) # bbot _create_process_pool
TP_W = max(8, CORES) # bbot _cpu_executor
REPS = 5
PROC = psutil.Process()
def tree_rss():
total = PROC.memory_info().rss
for c in PROC.children(recursive=True):
try:
total += c.memory_info().rss
except psutil.Error:
pass
return total / 1048576
def body(kb):
c = "<div class='row'>the quick brown fox jumped over 12345 lazy dogs</div>"
return (c * ((kb * 1024) // len(c) + 1))[: kb * 1024]
def urls(n):
return [f"https://evilcorp{i % 40}.com/user/{i}/profile?id={i}&ref=abc{i % 7}" for i in range(n)]
async def measure(ex, fn, arg, n):
"""One rep: n concurrent calls. Returns (ms, peak_rss_mb)."""
loop = asyncio.get_running_loop()
t = time.perf_counter()
task = asyncio.gather(*[loop.run_in_executor(ex, fn, arg) for _ in range(n)])
peak = tree_rss()
while not task.done():
peak = max(peak, tree_rss())
await asyncio.sleep(0.02)
await task
return (time.perf_counter() - t) * 1000, max(peak, tree_rss())
def kill(mp):
procs = getattr(mp, "_processes", None) or []
if hasattr(procs, "values"):
procs = procs.values()
for p in list(procs):
try:
p.terminate()
except Exception:
pass
async def cell(fn, arg, n, base):
loop = asyncio.get_running_loop()
mp = ProcessPoolExecutor(max_workers=MP_W)
await asyncio.gather(*[loop.run_in_executor(mp, fn, arg) for _ in range(MP_W * 2)])
A = [await measure(mp, fn, arg, n) for _ in range(REPS + 1)][1:]
kill(mp)
mp.shutdown(wait=False)
await asyncio.sleep(1.0)
tp = ThreadPoolExecutor(max_workers=TP_W)
await asyncio.gather(*[loop.run_in_executor(tp, fn, arg) for _ in range(TP_W * 2)])
B = [await measure(tp, fn, arg, n) for _ in range(REPS + 1)][1:]
tp.shutdown(wait=True)
await asyncio.sleep(1.0)
at, am = statistics.median([x[0] for x in A]), max(x[1] for x in A)
bt, bm = statistics.median([x[0] for x in B]), max(x[1] for x in B)
return at, am - base, bt, bm - base
async def main():
t0 = time.perf_counter()
base = tree_rss()
print(f"cores={CORES} A=_mp(Process,{MP_W}w) B=_cpu(Thread,{TP_W}w) reps={REPS}")
print(f"parent baseline RSS={base:.0f}MB | RSS columns are peak whole-tree delta over baseline")
print()
hdr = f"{'workload':>22} {'n':>4} | {'A time':>9} {'A mem':>8} | {'B time':>9} {'B mem':>8} | verdict"
print(hdr)
print("-" * len(hdr))
jobs = []
for kb in (3, 500):
jobs += [(f"simhash {kb}KB", compute_simhash, body(kb), n) for n in (1, 16, 64)]
for c in (1000, 10000):
jobs += [(f"collapse_urls {c}", collapse_urls, urls(c), n) for n in (1, 4)]
for label, fn, arg, n in jobs:
at, ad, bt, bd = await cell(fn, arg, n, base)
v = f"B {at / bt:.2f}x faster" if bt < at else f"B {bt / at:.2f}x SLOWER"
print(f"{label:>22} {n:>4} | {at:8.1f}ms {ad:+7.1f}M | {bt:8.1f}ms {bd:+7.1f}M | {v}", flush=True)
print("-" * len(hdr))
print(f"total wall: {time.perf_counter() - t0:.0f}s")
sys.stdout.flush()
os._exit(0)
if __name__ == "__main__":
asyncio.run(main())
singlerider
left a comment
There was a problem hiding this comment.
The premise is that simhash work is short because compute_simhash truncates to ~3KB. Truncation caps the input but the ceiling is still high: any body at or above 3KB costs a fixed ~196k iterations of a pure-Python loop, measured at 28.7ms per call, holding the GIL throughout.
| input | truncated | features | GIL-holding iterations |
|---|---|---|---|
| 39 B | 39 | 37 | 2,368 |
| 3 KB | 3072 | 3070 | 196,480 |
| 29 KB | 3072 | 3070 | 196,480 |
| 500 KB | 3072 | 3070 | 196,480 |
Left inline numbers on both files. Short version: the process pool is faster at every concurrency I tested, and on the waf_bypass path the default of 100 concurrent checks makes it a 37x regression that also degrades unrelated regex work sharing the pool.
Two notes that may be more useful than the executor question:
Profiling compute_simhash, xxhash is 1.43ms and the per-bit Python loop is 63.9ms, so 98% of the time is the bit loop. That is where the win is if this path is hot. A popcount-style rewrite produces identical output.
virtualhost.py on dev already uses run_in_executor_cpu with this same rationale, so this PR is following existing precedent rather than inventing it. The n=2 measurement suggests that precedent is also wrong, so it may be worth fixing both together rather than treating this one as the outlier.
|
After re-running this against bbot's actual pool configuration rather than a bare
|
singlerider
left a comment
There was a problem hiding this comment.
Verified bit-identical output vs the old algorithm across 714 cases (truncation-boundary sizes, bits>64, all tie-breaks, both truncate modes, normalization filters). Zero mismatches.
extract_params_html deletion is safe: no callers anywhere, and misc.extract_params_html does not exist.
Tests pass locally (helpers + dnsbrute_mutations/waf_bypass/virtualhost + wayback/bevigil).
One nit: the body says bevigil reverts collapse_urls back to run_in_executor_mp, but the net diff vs dev is zero since dev already uses mp. Worth a line so reviewers do not hunt for a bevigil change that is not in the diff.
LGTM.
Summary
The original version of this PR moved
compute_simhashandcollapse_urlsfrom the process executor to the thread executor, on the grounds that xxhash releases the GIL and that pickle/IPC dominates the work. Both of those reasons were wrong, and I've since measured them:compute_simhash. cProfile puts 88% of the time in thehash()body.waf_bypasspath the tradeoff was off by roughly 5000x.The real problem is the function.
compute_simhashruns a doubly-nested pure-Python loop, ~148k GIL-holding iterations, for any input at or above 3KB. Neither executor looks good when the work itself is 10x more expensive than it needs to be.Benchmarking the executor question against bbot's actual pool also changes the picture. The process pool sets
max_tasks_per_child=25, which costs a 458ms worker respawn and caps the pool at roughly 237 tasks/s, and a shared pool has co-tenants (badsecrets, kreuzberg) that a dedicated idle pool cannot show. A single FIFO call queue means a burst from one tenant delays every other one.Changes
Fix the function. Pack the feature hashes into a buffer and tally each bit column with
Counterover a byte-strided slice, keeping the per-bit work in C. Output is bit-identical; the call drops from 10.4ms to 1.1ms. At 1ms the thread executor is clearly the right home, sowaf_bypasskeeps the move this PR originally proposed, andvirtualhost(up to 160 concurrent simhash calls during the scan) benefits without changing.Correct the executor placements that were actually wrong.
bevigil.py:collapse_urlsmoves back torun_in_executor_mp. Reverting the change from the first version of this PR. It is 88ms per 2k URLs, GIL-holding,_module_threads=1so no respawn churn, and pickle is about 1% of the work.wayback.py:_parse_cdx_responseandcollapse_urlsmove torun_in_executor_mp. Both are long GIL-holding passes over 100k+ entry responses, and the latter is the same functionbevigilalready runs on the process pool, with input roughly 50x larger._parse_cdx_response's own docstring already said "Designed to run in a separate process"._filter_urls_syncstays on the thread pool: it is a bound method and it is YARA-based, which does release the GIL.dnsbrute_mutations.py:closest_matchmoves torun_in_executor_mp, passing hostnames rather than the dict, whose values are events and are expensive to pickle. 58.5ms of difflib per call with zero thread parallelism was the worst thread-pool offender.Document what each executor is for.
run_in_executor_cpu's docstring claimed it was for work that releases the GIL, but CPython'sredoes not (0.91x on 8 threads); yara is the only workload in the tree that fits that description. That wording is how simhash ended up on the thread pool invirtualhostwith the same incorrect rationale copied into its docstring. Also dropsRegexHelper.extract_params_html, which called amiscfunction that does not exist and had no callers.Measurements
compute_simhash, bit-identical across 133 inputs x 4 kwarg variants, 8 bit widths, and 400 similarity pairs:waf_bypass.finish()burst of 100, 16 cores, with badsecrets on the process pool and excavate's regex/yara on the thread pool at their real_module_threads:closest_match, thread pool with the dict vs process pool with the hostname list:Left out
max_tasks_per_child=25is miscalibrated forbadsecrets, which does 0.08ms of work per call and pays ~18ms of amortized respawn for it, measured at 143ms p95 and a ~237 tasks/s pool ceiling. That affects every process-pool tenant and has a real tradeoff, so it belongs in its own issue rather than here.