Repository navigation
detect_binary_per_byte reads every changed file to EOF (no early exit, no size cap on watcher path) #895
Description
Activity
I agree largely, I've been researching the way this actually works in the linux file command for example and there is unfortunately no other way to detect if file is not a binary if you are scanning every single byte till the end.
But I agree this is proably too much I will warp my head around it how to make it faster
Yes I think
MAX_FFFILE_SIZEfix is the correct safeguard @gustav-fff go fix it[triage-bot] LINKED PR: #14543 (open) — pingdotgg/t3code#14543
Only linked PR is downstream in
pingdotgg/t3code, not in this repo. It bumps@ff-labs/fff-node0.9.4 -> 0.11.0 to pick up the watcher rescan throttle from #751, re-targets the vendored patch and fixes the Windows native-load probe path. It does not touchFileItem::detect_binary_per_byteand its own body says so explicitly: the sniff cost reported here stays unfixed.So: no upstream PR exists for this issue.
crates/fff-core/src/types.rsstill reads to EOF with no early exit and no size cap on the watcher path. @dmtrKovalenko greenlit theMAX_FFFILE_SIZEsafeguard above - that work is unclaimed, not covered by #14543.Honk-Honk 🪿
@gustav-fff I mean in the detect binary path!
[triage-bot] DIRECTED: fix landed in #899
Guard is now inside
FileItem::detect_binary_per_byte(crates/fff-core/src/types.rs:563), not at call sites:size > MAX_FFFILE_SIZE-> return beforeopen(). Covers watcher modify (file_picker.rs:1694) and add (file_picker.rs:1741); scan path already skipped these, so its results don't change.breakafter first chunk containing NUL.
Downstream consumers (
prefilter.rs:78,bigram_filter.rs:1123,file_picker.rs:2154,:2228) all check size anyway, so >10 MiB files stayingis_binary=falsechanges nothing. The scan already left them that way.Regression test
detect_binary_tests::skips_files_above_max_sizefails without the guard:assertion failed: !classify(&content)cargo test -p fff-searchgreen.Honk-Honk 🪿
Which fff frontend?
Node SDK (@ff-labs/fff-node). The code is in
fff-core, so it affects every frontend.Has logs
Stack samples are below. I didn't enable the fff log file for this.
Description
FileItem::detect_binary_per_byte(crates/fff-core/src/types.rs, unchanged onmainat 89c1927) reads the whole file to EOF. It has two problems:set_binary(true)and keeps reading until EOF. One NUL byte is enough to decide.bigram_filter.rs) skips files aboveMAX_FFFILE_SIZE. Buthandle_create_or_modifyandadd_new_filecalldetect_binary_per_byteon every created or modified file of any size. So each write to a large text file re-reads the whole file: think of an append-only log, a JSONL trace or a large generated file under the root.In practice this is the main CPU cost when the root is a large folder that changes a lot. T3 Code embeds fff for its workspace index. With a ~1.65M-file non-git parent folder of active repos (macOS, Apple M4),
sampleof the host process showed:I ran a standalone fff-node harness on that folder with T3's options (
disableContentIndexing: true,disableMmapCache: true). Upgrading 0.9.4 → 0.11.0 brought the process from 80% to 47% of a core, thanks to the #751 rescan throttle, which is great. In both versions, most of the remaining CPU is this sniff, not the scans: 7 scans of about 2–4 s each took 191 s of CPU in 0.9.4.Suggested fix (a few lines):
Also cap the bytes read: either skip files above
MAX_FFFILE_SIZEon the watcher path, as the scan already does, or read only the first N KB as git does (it checks the first 8000 bytes). The second option changes classification for text files that have a NUL byte late in the file. A cap on the watcher path alone keeps today's results for anything the scan already classifies.Downstream report: pingdotgg/t3code#14536