michaeljmarshall commented on PR #16145: URL: https://github.com/apache/lucene/pull/16145#issuecomment-6357014478
Thanks @gsmiller. I decided to double check on my benchmarks to confirm the performance of this solution. The tl;dr is that I think this is a good change, but there may be some room for debating the correct value of `SKIP`. I had an agent rerun the JMH benchmarks on the current head (37c755bf3e, `N - 1` start) against its merge-base with main, and added a sweep of various `SKIP` values to see what the fixed 64 costs on hot files. The performance was the same or better in all cases except for the `StoredFieldsPrefetchBenchmark.prefetchOnly` case, which runs the `prefetch()` call in a tight loop on 1 and 8 threads (without consuming the prefetched bytes). Reducing the `mincore` frequency, aka increasing the value of `SKIP`, increases throughput when measuring `prefetch()` ops/ms on hot files. In itself, this is unsurprising, but when comparing to perf on `main`, this branch has lower throughput on single threaded calls and it may be worth considering `SKIP=128` or `SKIP=256` as the fully hot file is a common case. Notably, we do not see the same reduced throughput for 8 threads, which I hypothesize is a result of removing the write to `AtomicInteger` on every call to `prefetch()`. @goankur - since your benchmarks proposed the drop to `SKIP-64`, do you have any insight? It's possible that this tight loop that simply measures `prefetch()` throughput is not representative of realistic access patterns and the cost of increasing `SKIP` means potentially more major page faults before switching back to the ramp phase. Below is agent synthesis and analysis of the benchmarks: > Same machine and harness as my earlier comment (GCP n2-custom-16-32768, 2 local SSDs RAID-0, JDK 25). Hot is 1 GiB / 1M docs, fully cached. Each cell is the mean of two runs, the second in reverse variant order to cancel ordering effects. Setup and raw results: fd6f4ac79b27109ebc52997ae24e4122b492e326. > > **Hot, `StoredFieldsPrefetchBenchmark.prefetchOnly` (100 `prefetch()` calls per op and nothing else), % vs main:** > > | | main (ops/ms) | SKIP=64 (current) | SKIP=128 | SKIP=256 | SKIP=512 | > |---|---:|---:|---:|---:|---:| > | T01 topK=100 | 42.1 | -17.2% | -8.0% | -4.3% | -2.1% | > | T01 topK=10 | 405.8 | -14.6% | -7.1% | -3.8% | -2.4% | > | T08 topK=100 | 200.4 | +40.8% | +49.0% | +60.9% | +63.9% | > | T08 topK=10 | 2,045.6 | +34.8% | +45.5% | +55.4% | +59.7% | > > The single-thread cost is a fixed per-call overhead (the 1-in-`SKIP` `mincore` plus counter bookkeeping): about 238 ns per `prefetch()` on main, so roughly +49 ns at 64, +21 ns at 128, +11 ns at 256, +5 ns at 512. This benchmark does no reads, so it is the worst case for that cost. With 8 threads the branch is faster than main at every `SKIP` because it no longer writes the shared counter on the hot path. > > Everything else on hot is flat across all variants: > - `retrievePrefetchThenRead` and `retrieveNoPrefetch` (stored fields with actual reads): within about 3%. > - Random-read benchmarks (`mmap*`, `mmap*BatchedPrefetch`, T01 to T16): within about 2.5%. > > **Under memory pressure** (39 GiB file, 31 GiB RAM, two runs) the results do not depend on `SKIP`: `mmapBatchedPrefetch` is about +300% (T01) to +407% (T16) and `mmapRandomBatchedPrefetch` about +281% (T16) to +792% (T01) vs main for every variant. For stored fields with real reads, `retrievePrefetchThenRead` is about +390% at T01 (topK=100) and +13% to +29% at T08, again the same for every variant. > > **Cold** (page cache dropped before every iteration, one run, `-f 1`): also independent of `SKIP`. `retrievePrefetchThenRead` is about +90% to +104% at T08 and more than +2,000% at T01, and `mmapRandomBatchedPrefetch_T01` is about +3,700%, with the T16 rows flat. The no-prefetch rows are noisy at this setting (e.g. `mmapRandom_T08` swings from -31% to +56% between variants on identical code), so I would not read anything into small differences here. > > `prefetchOnly` under pressure and cold shows -94% to -99.6% for every variant. That benchmark only issues prefetches and never reads, so the branch is doing the I/O that main skips; it is not a cost in the read-then-prefetch rows above. > > **On choosing `SKIP`:** a file only starts sampling after `N = 1024` consecutive hits, so half-warm files are not affected by it. `SKIP` only bounds how long a nearly fully-cached file stays blind after pages are evicted, roughly `SKIP` unprefetched cold reads before a sample notices, after which the file goes back to probing every call. Given that, [I'd lean toward 256: it recovers most of the hot single-thread cost and is still a hard bound, unlike main's unbounded backoff / happy to keep 64 or go with 128 if you prefer a tighter bound]. > > Caveats: one machine, a synthetic single-file benchmark, and nothing here exercises HNSW or vector-rescoring paths on hot data, where prefetch calls are denser relative to other work. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
