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

BattleSearch: Fix pathological regex #10679

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

larry-the-table-guy
Copy link
Contributor

PCRE lookahead was causing the regex to be pathologically slow.

The lookahead was introduced by #7801 , the purpose of which I (eventually) inferred to be to handle out-of-order player IDs.
I.e., you do /battlesearch alice, bob and also want to match battles where p1 was bob and p2 was alice.


Cases I've tested: files with no matches, and search 1 ID w/ matches in either slot, and search 2 ID w/ matches in either order.


Measured perf on directory with 500K files, 8GB, all non-matches.
Low-end cacheless NVMe SSD.

File cache was cleared each run with echo 3 | sudo tee /proc/sys/vm/drop_caches.

Before: 100% CPU usage, 1MB/s disk read.
After: 20% CPU usage, 500MB/s disk read.

$ rg --version
ripgrep 14.1.1 (rev 4649aa9700)

features:+pcre2
simd(compile):+SSE2,-SSSE3,-AVX2
simd(runtime):+SSE2,+SSSE3,+AVX2

PCRE2 10.43 is available (JIT is available)

When testing, I had to disable a couple permission checks, disable ripgrep's gitignore behavior and import Monitor to actually be able to run this. Those changes are visible at larry-the-table-guy@b519ef6 . That is the code I tested and measured with, and then I copied the relevant changes to this PR branch.


Chatlog was also touched by #7801, but it seems that feature has since been moved to a DB, so IDK if it's worth changing that one as well.

PCRE lookahead was causing the regex to be pathologically slow.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant