Faster scanning: batched reads, readahead, threaded first scan - #461
Draft
1rhino2 wants to merge 16 commits into
Draft
Faster scanning: batched reads, readahead, threaded first scan#4611rhino2 wants to merge 16 commits into
1rhino2 wants to merge 16 commits into
Conversation
the old suite ran a scan of each data type and checked scanmem exited zero. that passes against almost any regression, a scan routine could return half the matches or none and it would still be green. memfake can now plant a known value a known number of times, and rewrite only those slots on SIGUSR1. so a test scans for the value, mutates, then narrows, and the narrowed count is exactly what was planted. the process's own copies of the value (locals, spilled registers) keep the old value and drop out, which is what makes it exact rather than approximate. covers int8/16/32/64, float32/64, bytearray including wildcards and the >8 byte general loop, and a dense case where every slot in the buffer is a match so buffer edge handling is actually exercised. checked it fails when it should: flipping one < to <= in the scanroutines length guard takes 7 of the 19 checks red.
no optimisation here, just something to measure against. times the initial scan and the scan+narrow separately and takes the difference as the narrowing cost, since scanmem has no per command timing. each measurement restarts the target. first version of this reused one memfake across runs and the second run scanned for a value the mutation had already overwritten, which reported a fake 0.02s narrow. worth knowing if anyone touches this. baseline on 128MB / 16.7M matches: 1.33s initial, 1.95s narrow, 65695 pread64 calls. that call count is one per 2KB of match data, which is the thing worth attacking.
the narrowing scan reads every match back through sm_peekdata, which refilled its window one PEEKDATA_CHUNK (2048 bytes) at a time. that is a syscall per 2KB of match data regardless of how much of the region the scan is about to walk through. keep a readahead window that doubles on each sequential refill up to 64KB and drops back to a single chunk on a real jump, so dense scans get large reads and sparse ones do not pay for bytes they will not use. two things had to change for it to actually take effect. a forward walk lands exactly on base+size when it runs off the end of the window, which hits the cache miss branch rather than the partial hit branch, so the miss path had to stop treating that as a jump. and the head shift only triggered when there was no room for missing_bytes, which in steady state left just the couple of KB freed by the last shift, so every refill collapsed back to one chunk. 128MB / 16.7M matches: 65695 pread64 calls down to 2212, narrow 1.95s to 1.61s. the call count is 30x better but the wall time is only ~17%, because at this match density the narrow is spending most of its time in the scan routine and match bookkeeping rather than in syscalls. the syscall win matters more where syscalls are dearer, seccomp filtered containers and the like. 19/19 of the scan tests pass, and clean under asan+ubsan.
gprof on a 64MB all-matching target showed sm_peekdata at 67M calls and add_element at 134M, once per byte scanned. Both are too big for gcc to inline, so the loop was paying a real call for cases that do almost no work: peekdata is a cache hit nearly every time (1120 preads for 67M calls), and add_element is usually just a contiguous append. Split the trivial case out of each into a small inline wrapper and leave the general function alone as the fallback. No behaviour change, the match sets are identical. 64MB, every 8-byte slot planted, median of 5: initial scan 0.67s -> 0.58s narrow 0.79s -> 0.48s Checked the tests actually catch bugs here rather than just passing: letting add_element_fast skip the contiguity check fails 6 of 19, and over-permitting the peekdata hit test by 64 bytes fails the dense case (254208 of 262144 matches kept).
parse_uservalue guards each cast with `num <= UINT64_MAX` / `<= INT64_MAX`, but neither constant has an exact double. Both round up, to 2^64 and 2^63, so the guard admits the boundary itself and the cast right after it is out of range and undefined. Reachable from normal input. Typing 18446744073709551616 as a number, with a UBSan build: value.c:194:9: runtime error: 1.84467e+19 is outside the range of representable values of type 'long unsigned int' Bound by the power of two and keep the comparison strict. The 32 bit and smaller limits are all under 2^53 so they convert exactly and are left alone. Also silences two clang -Wimplicit-const-int-float-conversion warnings, which were pointing straight at this.
The build workflow had never actually run. It was pinned to ubuntu-22.04 with actions/checkout@v3, built gcc only, and passed the sanitizer flags through `make CFLAGS=...`, which replaces what configure worked out and never reaches LDFLAGS, so nothing got linked against the runtime. Set the flags at configure time instead and split the sanitizers into their own job, so a plain build failure and a sanitizer failure are told apart. float-cast-overflow is asked for explicitly since gcc leaves it out of -fsanitize=undefined, and it is the check that caught the 64 bit parse bound bug in the previous commit. The setup action takes cc/cflags/ldflags/configure-args now so the matrix can drive it. Defaults match the old behaviour so the coverity workflow is unaffected. Dropped `apt upgrade` from it, it pulled in a lot for no benefit and was the slowest step in the run. Checked locally first: gcc and clang both build warning free, and the asan+ubsan build passes make check with no sanitizer output.
Adds process_vm_readv in front of the existing backends. It copies straight between address spaces with no /proc file and no VFS layer, and on its own it is a lot faster: reading 200MB in 64KB chunks out of another process goes 5.3 GB/s on pread to 14.7 GB/s here, about 2.8x. End to end it is worth much less than that, and it is worth being honest about why. After the readahead change a 64MB scan only issues about 1120 reads, so syscalls are already under 1% of the run. On a read heavy scan (256MB, value absent, so almost nothing gets stored) it comes out around 3%: 702ms vs 722ms best of 3. On the dense benchmark it is a wash. The scan is CPU bound at this point, not read bound. Keeping it anyway. It is never slower, it is what #400 asked for, and it gives a working path on setups where /proc/pid/mem reads are restricted but process_vm_readv is allowed. Not always permitted though, so a refusal (ENOSYS/EPERM) disables it for the session and the read is retried on the old path rather than paying a failing syscall every time. A short read is not treated as a refusal, that just means the end of a mapping, same as pread. peekbuf.pid was only kept in the non-procmem build, it is needed either way now. SCANMEM_NO_PROCESS_VM_READV=1 forces the old path, which is how the two were compared. Both backends return byte identical match lists when the same target is scanned twice (10000 listed addresses, no diff), and the suite passes on each.
The first scan runs the scan routine at every byte offset of every region
and nothing about one offset depends on another, so it parallelises. The
narrowing scan is left alone, it walks an existing match list and is
already cheap by comparison.
Splitting by region would not have helped much. The usual case is one big
region and a handful of small ones, so a per region split leaves eleven
threads idle. Regions get cut into chunks instead.
The part that does not split is emitting the records: a match of length L
is followed by L-1 filler entries, so a chunk boundary can land in the
middle of one. A chunk works out its own carry by rescanning the L-1
bytes behind it, which is enough because a match resets that counter
rather than adding to it, so only the last match before the boundary can
still be carrying. For number scans that window is 7 bytes.
Chunks record into their own buffers and a serial pass stitches them in
address order through the same add_element calls the serial scan made, so
the swath layout is byte for byte what it was. That pass is O(matches),
not O(bytes), which matters: doing it per byte cost more than the
threading saved and showed up as a fixed ~350ms that would not scale away.
Slices are handed out as threads free up rather than all being built up
front, so a process with a large mapped address space does not need a
descriptor per slice before any scanning starts.
`option threads N` picks the count, 0 (the default) means one per online
CPU, 1 scans serially. Best of 3, 1000 matches, this box has 12 cores:
serial threads=12
64MB 204ms 84ms
256MB 734ms 208ms
1GB 2936ms 702ms
threads=1 comes out level with the old serial path.
Verified against a build of the previous serial code: identical match
lists for int8/16/32/64, float, and bytearray including wildcards and the
12 byte general loop, and identical across 20 runs at thread counts from
2 to 64. The suite now checks this itself by comparing a run split into
4096 byte chunks against one big enough to have no boundaries at all,
which is the comparison that actually detects a broken carry. Comparing
two thread counts does not: both go through the same chunking, so both
come out wrong the same way and still agree. Confirmed it fails when the
carry is deliberately removed.
asan and ubsan are clean on the threaded scan. tsan could not be used,
it hangs on this workload with threads=1 as well, so it is not the
threading, and the same hang happens on the serial build.
Kept on a branch while it was being built because a bad parallel merge returns a wrong match set rather than crashing, so it fails quietly. Merging now that the split vs unsplit comparison is in the suite and passing, and the numbers hold up.
configure probes each flag with -Werror and keeps the ones the compiler takes, so this does not break a build with a different or older compiler. Ends up as -Wextra -Wno-unused-parameter -fstack-protector-strong -Wformat=2 -Wno-format-nonliteral -Wvla -D_FORTIFY_SOURCE=2 on gcc 15, and the standard moves gnu99 to gnu11. Two suppressions, both deliberate: -Wno-unused-parameter, because the scan routines are macro generated onto one shared signature so most of them really do not use every argument. That is about 130 warnings all saying the same thing and none of them a bug. -Wno-format-nonliteral, because the single place it fires picks its format from a fixed internal table by scan type, not from input. What the new flags turned up, all fixed here: handlers.c, -Wclobbered: `seconds` and `cont` in handler__set are written before the setjmp and read after it. A non volatile local is indeterminate once longjmp has been through, so they are volatile now. show_message.c, -Wunused-result: the pager child ran read() and write() on the status pipe and then called exit(errno), except errno by then belonged to whichever of those two ran last, not to the execvp that actually failed. The parent was being told the wrong error. Save execvp's errno first. The parent side priming write is checked now too rather than being assumed to work. maps.c, -Wvla: `char filename[len]` where len is getline's buffer capacity, so the size came from the longest path the target process has mapped and it landed on the stack, once per line, memset in full every time. One heap buffer that grows with the line buffer instead. Same regions found and same match list as before on the same target, gcc and clang both build with no warnings, 23/23 pass.
The build badge pointed at travis-ci.org and there has been no .travis.yml for a long time, so it rendered as nothing. Points at the Actions workflow now. Adds a scan speed section covering `option threads` and what it is worth, with the measured table rather than a claim. Also says plainly that process_vm_readv is only a few percent end to end even though the call itself is much faster, because the scan is limited by the comparison work. BASELINE.md gets the after numbers next to the before ones, including the bit where syscalls dropped 30x and time only dropped 4x, and a note that the apparent threads=1 regression was two orphaned scanmem processes eating the machine rather than anything in the code.
Adding clang to the matrix turned up that scanmem built with clang at -O1 or higher scans wrong. It finds 3 matches where gcc finds 102, on the same target, having read the right memory, parsed the right value and picked the right routine. Not caused by anything in this branch. Upstream 0375cc0 fails the same way once an asserting suite is pointed at it. Nothing ever was: the old suite checked that scanmem exited zero and CI only built with gcc, so a compiler that silently breaks scanning had nothing to trip over. Ruled out: strict aliasing, vectorisation, signed overflow, and the extern inline definitions, none of which change it. -fno-inline recovers part of it, 4 of 23 checks to 16, so inlining is exposing undefined behaviour rather than being the bug. Left in the matrix as continue-on-error rather than dropped, so it stays visible. gcc and the sanitizer job still gate the run. Written up in the man page and in bench/BASELINE.md with a reproducer.
…ours configure~ is an autoconf backup file. It got committed by a careless `git add -A` in the build flags commit and should never have been in the tree. Removed and .gitignore now covers backup files, which it did not, it only listed `configure`. Two runs on main sat in apt for six hours each before being cancelled. Those were the old workflow, which is already gone, but nothing stopped a hung step from eating the whole default budget, so the jobs get timeout-minutes now: 30 for the builds, 45 for sanitizers and coverity. apt-get instead of apt, since apt prints its own warning about not having a stable interface for scripts, plus DEBIAN_FRONTEND=noninteractive and a retry count so a slow mirror fails instead of hanging. Left the package list alone. Trimming recommends would be a behaviour change I cannot verify without pushing, and the point here was the hang.
The maps line was read with %x into an int for the file offset and %u into an int for the inode. Both of those are unsigned long in what the kernel writes, and an inode number goes past INT_MAX routinely, so the conversion was wrong even though nothing downstream reads either field. This is the second half of CyberShadow's #455. The first half was the VLA on the stack, which is already fixed in this branch, differently: that one keeps a heap buffer that grows with the line rather than a fixed PATH_MAX array, so there is no path length ceiling and no memset of a 4KB buffer for every line of every scan. #455 was closed unmerged because the commit was authored by a tool rather than a person, and the author said to take it from here. Co-authored-by: CyberShadow <160894+CyberShadow@users.noreply.github.com>
Closes #456. ToyKeeper asked for locks that only allow a value to move one way, health that can drop but never be refilled by the game, ammo that can go up but not down. Their patch was against Debian's 0.17-5 which still had a lockflag column. Current main lost it at some point, all that survived is the LOCK_FLAG_TYPES constant and a cheatlist_toggle_lock_flag_cb that threw the edit away, so this is a reimplementation rather than their diff applied. Flag lives at model index 6 instead of index 0 like the old one did, which keeps every existing column index and every already saved cheat list file working. Loader defaults rows with only 6 fields to '='. '=' is the old behavior, write the value every tick. '+' and '-' read first and only push back when the game moved the value the wrong way, so the row display has to follow the value it adopted or the next tick compares against a stale number. bytearray and string have no ordering, they take the plain lock whatever the flag is set to. Tested with a harness that binds the real data_worker, add_to_cheat_list and read/write_value to a fake memory backend and runs the real misc.treeview_append_column combo wiring under Xvfb. 24 checks: both directions on int32/float32/uint64/negative int64, blocked vs kept moves, no redundant write when already satisfied, no display stomp mid-edit, bytearray/string fallback, 6-field file load. Not driven through a real GTK window by hand, and not run against a live target process.
Refs #450, #435, #427. Not a fix for any of them, read on. CLIPBOARD was built at module scope but used in exactly one place, the copy address menu item. That made line 55 the first thing in the program to touch the display, which is why both #450 and #435 have their traceback landing there rather than anywhere meaningful. Reproduced with DISPLAY and WAYLAND_DISPLAY unset: Gtk-CRITICAL: gtk_clipboard_get_for_display: assertion 'display != NULL' failed emitted on plain import, exit status still 0, CLIPBOARD left holding something unusable. Now it is fetched when copy is actually clicked and returns None if there is no display, so copy goes quiet and startup does not. Second half: check for a display in main and say why it is missing. pkexec does not carry x11/wayland authorization over to root, so this fails for root while working fine for your own user, and nothing in the old output said so. Prints the cause and the xhost workaround, exits 1. This does not make it run under pkexec without display authorization. Nothing here forwards auth, it only turns a gtk assertion into a sentence. #427 still tracks the real display fix and #416 the pkexec dependency. Checked: import with no display is now silent, running with no display prints the message and exits 1, get_clipboard() returns None, and under Xvfb the clipboard still round trips a copied address. Not tested under an actual pkexec elevation.
1rhino2
force-pushed
the
perf-threaded-scan
branch
from
August 22, 2026 17:10
e7079b0 to
97b0266
Compare
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
1 participant
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.
scan performance, plus the build and test changes needed to trust it.
first scan, same box, heap size given:
narrowing a dense match set went 1.95s to 0.48s, and the read syscalls for that narrow went 65695 to about 1100.
three changes do the work. process_vm_readv batched through iovecs where the kernel allows it, readahead so the scan is not stalled on the next chunk, and splitting the first scan across threads with an ordered merge.
option threadsdrives the last one, 1 is serial.the gate was byte identical output, not speed. same target, thread counts 2 through 64, twenty runs, diffed against a pre threading reference across every data type. the suite had to go first for that: it used to assert only that scanmem exited zero, it now asserts on match sets, and i checked it fails when a scan routine is deliberately broken instead of passing anyway.
correction to what this branch says about clang. commits here claim clang at -O1 and up builds a scan that returns wrong results, and mark the clang CI job non blocking on that basis. wrong, there is no miscompile. test/memfake plants values into a buffer nothing in that process reads back, so clang deletes the stores as dead and the target never holds them. configure's CFLAGS reach memfake as well as scanmem, which is why it tracked optimisation level. #462 fixes memfake and reverts the claim in BASELINE.md and the man page. scanmem at -O2 with memfake at -O0 is 23/23, which pins it.
a7229a0 overlaps #457, same sscanf fix plus a stricter bounded conversion there. drop mine at merge.
draft on purpose, nothing needs looking at until #457 lands.