filestream_gets() reads one byte per VFS call — ~21 ns/byte for every core that scans a text file
Nobody has claimed this yet.
- Dominant language
- C
- Stars
- 14.1k
- Forks
- 2.2k
- Avg merge
- 7h 35m
- Merged PRs (30d)
- 51
Description
filestream_gets() is implemented as one filestream_getc() per character, and filestream_getc()
is filestream_read(stream, &c, 1) — a VFS-interface indirection plus a one-byte read per byte of
the file.
Measured against plain buffered fgets() over the same bytes, same stop positions:
| bytes scanned | plain fgets |
through filestream_gets() |
ns/byte |
|---|---|---|---|
| 3,674,296 | 5.96 ms | 76.80 ms | 20.90 |
| 6,498,700 | 5.17 ms | 136.06 ms | 20.94 |
| 11,678,730 | 6.33 ms | 246.39 ms | 21.10 |
| 25,279,764 | 9.46 ms | 530.91 ms | 21.00 |
A dead-constant ~21 ns/byte across file sizes is a fixed per-byte overhead, not parsing.
Why this has probably gone unnoticed: RetroArch itself barely pays it. There are four
filestream_gets() call sites in the tree — configuration.c (timezone file),
input/common/linux_common.c (illuminance sensor), interface_stream.c (wrapper), and
gfx/drivers_context/mali_fbdev_ctx.c — and they all read tiny files. The cost lands on cores,
which reach the same function through the #define fgets rfgets compatibility shim in
file_stream_transforms.h whenever they scan a text database line by line.
A concrete example I measured: the FBNeo core scanning a 13 MB cheat.dat for one romset's section
spends ~2.8 s in this function on a Hisense Android TV (MediaTek mt5896, armeabi-v7a, Android 12),
and roughly half that again when the entry is a clone and the file must be rescanned for the parent.
On that device it is the difference between content loading and the app being killed by an ANR. That
ANR is my problem and I am not raising it here — the measurements and the corrected diagnosis are at
finalburnneo/FBNeo#2686 if the context is useful.
To get the obvious deflection out of the way: yes, FBNeo could mitigate this on its side, by not
rescanning, or by not parsing cheats during load at all. It probably should. That would not change
the fact that the shim charges ~21 ns/byte to every core that reads a text file line by line, and
that none of them can opt out of it.
There is already a fast path in-tree, and nothing that scans lines can reach it.
RETRO_VFS_FILE_ACCESS_HINT_FREQUENT_ACCESS puts a memory mapping behind the handle, and the recent
filestream_get_mapped_ptr() work exposes it. With a mapping available, filestream_gets() can find
the line with memchr() and copy exactly the bytes it returns — no read, no read-ahead, no rewind.
I have that implemented against master (282ca43c) and measured it at 12.04× on a 25 MB /
265k-line scan (92.269 → 7.664 ms, n=50 per arm alternated, Welch t = 109.1, p = 3.9e-65), with the
fgets contract verified by 382,002 conformance checks, 0 failures, against unmodified upstream
and against the change, with MMAP=1 and MMAP=0, under ASan + UBSan.
The catch is the reason I am opening an issue rather than a PR: rfopen() hard-codes
RETRO_VFS_FILE_ACCESS_HINT_NONE (file_stream_transforms.c:59), so no core using the fgets
shim can request a mapping, and the three in-tree callers that do pass the hint
(task_cloudsync.c, lrc_hash.c) never call filestream_gets(). The intersection is empty. So the
optimization on its own would improve a path nothing currently takes, and the useful change —
letting line-scanning callers opt into a mapping — touches API surface and default behaviour, which
is your call and not mine to make in a drive-by patch.
Two things worth weighing that I do not have the context to decide:
- Whether
rfopen()'sHINT_NONEis deliberate. Mapping everyfopen-compat handle would cost
address space, and on 32-bit builds that is not free — I measured 1.85 GB of peak virtual address
space in a 32-bit Android process during ordinary use, so a blanket change there could hurt. - Whether the better shape is a hinted
rfopenvariant, converting specific callers, or something
else entirely.
I also tried the obvious fix first — block reads with a seek-back — and it is a 1.40× regression
on this base (742.50 vs 529.48 ms). vfs_implementation.c installs a 64 KiB setvbuf buffer and
the per-line fseeko discards it on every line; the block reads themselves cost 17 ms, the rewinds
cost 823 ms. Recording that so nobody else spends a day on it.
Happy to open a PR with the mapped-path implementation and the conformance test if it is useful, or
to leave this as a report if you would rather shape it differently. Either is fine.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with filestream_gets(), file_stream_transforms.c:59, and the mapped-handle path exposed by filestream_get_mapped_ptr(); review how task_cloudsync.c and lrc_hash.c pass access hints. Run the existing fgets conformance checks and the reported scan benchmark. Done means an agreed API/default behavior lets relevant line scanners use mapping without regressions on MMAP=0 or constrained 32-bit builds.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- c
- Domain
- performance
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 45/100