MemorySegmentIndexInput: always prefetch on RANDOM mode - #16145
MemorySegmentIndexInput: always prefetch on RANDOM mode#16145michaeljmarshall wants to merge 8 commits into
Conversation
The power-of-two back off in MemorySegmentIndexInput.prefetch() assumes consecutive page-cache hits suggest the file is warming up, so madvise overhead can be skipped until the next power-of-two call. That assumption breaks for ReadAdvice.RANDOM, where each request accesses unpredictable pages — a warm page in the past says nothing about whether the next page is warm (e.g. HNSW graph traversal touches a different path per query). Add a `volatile boolean isRandom` field, set by updateReadAdvice() and also by slice(String, long, long, IOContext), the primary entry point for HNSW vector files, and skip the backoff in prefetch() when it is true. The one drawback: for RANDOM-advised files that are fully warm in the page cache, isLoaded() is now called on every prefetch rather than being throttled. In practice this seems acceptable because isLoaded() on warm pages should be cheap, and a fully warm RANDOM file means prefetch will now return a more accurate answer than before. On x86 (TSO) a volatile load before LOCK XADD requires no hardware fence and costs nothing measurable. On ARM the ldar before ldadd adds a small ordering cost, still dwarfed by the atomic itself. The miss- reset lambda returns to a plain set(0) with no special-casing. I used claude to generate the tests and then I manually reviewed them. I also considered encoding RANDOM mode as a negative sentinel in the counter itself (Integer.MIN_VALUE) to avoid any new field. Rejected: the counter walks toward zero on every getAndIncrement(), losing RANDOM mode after ~2B calls without a miss; preserving it through misses required a CAS loop (getAndUpdate) in the miss handler, adding complexity that doesn't seem necessary.
The miss handler in prefetch() was calling sharedPrefetchCounter.set(0) unconditionally, even in RANDOM mode where the counter is never read. This was inconsistent with the isRandom short-circuit that already prevents getAndIncrement() from running in RANDOM mode. Guard the reset with `if (isRandom == false)` so the counter is completely untouched when RANDOM advice is active.
When NATIVE_ACCESS.isEmpty() returns true, we don't ever update isRandom. This isn't a true problem with the current usage, but in order to ensure the boolean is set correctly, we can move the assignment to before the NATIVE_ACCESS.isEmpty() check.
| MemorySegmentIndexInput slice = slice(sliceDescription, offset, length); | ||
| ReadAdvice advice = toReadAdvice.apply(context); | ||
| MemorySegmentIndexInput slice = | ||
| buildSlice(sliceDescription, offset, length, advice == ReadAdvice.RANDOM); | ||
| if (NATIVE_ACCESS.isPresent() && advice != ReadAdvice.NORMAL) { | ||
| // No need to madvise with a normal advice, since it's the OS' default. | ||
| final NativeAccess nativeAccess = NATIVE_ACCESS.get(); | ||
| if (length >= nativeAccess.getPageSize()) { | ||
| // Only set the read advice if the inner file is large enough. Otherwise the cons are likely | ||
| // outweighing the pros as we're: | ||
| // - potentially overriding the advice of other files that share the same pages, | ||
| // - paying the cost of a madvise system call for little value. | ||
| // We could align inner files with the page size to avoid the first issue, but again the | ||
| // pros don't clearly overweigh the cons. | ||
| slice.advise( | ||
| 0, | ||
| slice.length, | ||
| segment -> { | ||
| nativeAccess.madvise(segment, advice); | ||
| return true; | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
This is actually wrong in a subtle way. A slice can pull in a subset of segments from the parent, then result in different advice, which ultimately modifies the internal advice, and can therefore lead to out of sync values for isRandom. As such, this extra metadata needs rethinking.
|
@michaeljmarshall did we see any improvements after adding this change? |
| @@ -349,8 +359,12 @@ | |||
| length, | |||
| segment -> { | |||
| if (segment.isLoaded() == false) { | |||
There was a problem hiding this comment.
This could be avoided too in the random case since we don't use this information?
There was a problem hiding this comment.
I think it makes sense to remove this check when we have advised RANDOM, but note that we do use the isLoaded() information to determine whether to call madvise and whether to return true or false where true indicates the caller should defer access to the prefetched page. From the method's javadoc:
* @return true if prefetch actually prefetched something, hence user can benefit from deferring
* reading the prefetched memory block.
|
Why does the field need to be volatile at all? It is per-instance and each instance should only be used from one thread. This is also true for random access slices because some implementations of RandomAccess are NOT thread-safe (e.g., the default impl used by NIOFSDirectory). Documentation: |
|
I like the overall idea here, to turn off the exponential backoff behavior for the
Strangely, it's the opposite: warm / hot pages are more costly than the cold case, because it cannot early-break that for loop when all bytes are I think we can turn off all the |
|
I spent some time spinning up a JMH benchmark to simulate and vet. The setup is an 8G file on my EC2 m5.2xlarge (8 vCPU, 16G mem, io2 EBS with ~338us 4K random read latency direct io bypass page cache verified with The idea is: each read (4k/8k/16k) randomly picks between a small hot region (16MB, stays cached) and a random offset across the full 8G file. I assume this could roughly mimic HNSW scenario, some nodes are warm while others are deep in the graph and cold. Kernal page cache is cleared before each iteration. Test run for 6s for each iter, not enough time to fully warm page cache. Benchmark results (focus on 4k read)With this PR, compare all hint modes:
With PR vs. without PR comparison:
Fully warm page cache (no page cache clear before each iter), with PR applied:
Key findings
Note that this is a microbenchmark, I think it would be more sound to vet with a real-world workload like in HNSW scenario, but the direction is positive. JMH result details candidate benchmark (clear page cache - mimic cold page access)candidate benchmark (all warm pages in page cache)
baseline benchmarkWithout this PR changes, only test fio testAverage: 338 µs per 4K random read |
|
I don't have benchmark numbers yet, but I can work on getting those. Is this the recommended benchmark to run to get HNSW numbers? https://github.com/mikemccand/luceneutil/blob/main/README.md#running-the-knn-benchmark As for the current state of this PR, I am concerned that the implementation has a latent bug in the state tracking logic. Because we allow cloning of the
This problem also exists for the One solution could be to store the advice per segment in a synchronized bitmap (in a way similar to the My primary concern with this (latent) bug is that it is very nuanced and would like lead to unpredictable behavior. |
|
This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the dev@lucene.apache.org list. Thank you for your contribution! |
|
I spent some time benchmarking and pressure-testing the prefetch path. Sharing the results here, hopefully these add some useful data and insights to the discussion. The key takeaways:
Here are my rationales. The numbers below are based on
Platforms tested.
The latency and throughput numbers are calculated via 1. When data is hot in the page cache, the backoff avoids prefetch overheadThis is the case #13381 was built for and introduced the backoff counter. When everything is in RAM, every prefetch does an Removing the backoff counter and
(unit: ops/ms, higher is better) With the backoff on, the prefetch path almost matches the plain-read, because prefetch is almost free due to counter climbs up in power-of-two increments when hot, backoff skips the The real stored-fields workload tells the same story. I tested only on NVMe, top-k (k=100) store fields retrieval (batch prefetch on store fields to load 4K sized doc then for-loop read), index fully hot:
2. When most reads are cold, the backoff doesn't matterFor a large portion of cold reads (say like >50%), removing backoff counter or not doesn't matter a lot, because page cache misses triggering page fault keep resetting the counter ( Here's the all-cold read scenario (64G file, page cache dropped each iteration) for
And the stored-fields workload at ~50% cold (64G index, ~half in RAM on Linux NVMe) says the same, backoff and always-prefetch are almost a tie, both ~6.5x better than no-prefetch read at T1 thanks for the batch async I/O firing upfront. Note that at T8, a bit surprising, I suspect the I/O queue is full from the 80K stored-fields blocks (default BEST_SPEED), so there's no spare parallelism left for prefetch to exploit. The prefetch effect saturates.
So, fully hot warm and heavily cold are both fine, the backoff neither helps nor hurts in the two scenarios. 3. At the mixed-warm-cold scenario, the backoff suppresses the prefetch it needs mostThis is where the PR attempts to address, in the mixed warm/cold, aggressive prefetch is much-needed, it attempts to load pages in ahead of the real read. Say in memory pressure scenario when ~90% of reads hit page cache and only ~10% are cold, misses are too rare to reset the backoff counter, it climbs the power-of-two ladder and suppresses prefetch on the cold pages that truly needed. Under memory pressure (index ≈ RAM),
Note that with prefetch, a single thread can already saturate the device (2.16 -> 7.13 ops/ms at T1 for NVMe), we don't need extra threads to pile on I/O to keep the queue deep. That's the real power of async prefetch to make I/O parallel even with a single thread. The stored-fields workload shows the same at T1: prefetch-then-read is 0.55 ops/ms with always-prefetch vs just 0.19 with the backoff on (~2.9×). But the benefit diminishes as thread count increases. In the 16K × 16 4.
|
| T1 | T8 | T1 -> T8 scaling | |
|---|---|---|---|
isLoaded() / mincore, warm |
0.57us | 1.28us | ~2× |
isLoaded() / mincore, cold |
0.43us | 1.25us | ~2× |
madvise(WILLNEED), warm |
0.33us | 2.5us | ~7× |
madvise(WILLNEED), cold |
5.3us | 42us | ~8× |
isLoaded() is cheap and stable, and it scales only ~2× from T1->T8. madvise(WILLNEED) latency is lower than isLoaded() single-threaded when pages are warm (0.33us madvise vs. 0.57us isLoaded() check). But it's no free lunch, counter-intuitively, it's contention-prone, madvise on warm is 2x the latency of isLoaded() at T8 (2.5us vs. 1.28us), and its latency climbs ~7–8× with threads and blows up on cold pages (the async WILLNEED hint isn't free, still has to submit and queue the I/O).
So, in terms of isLoaded()'s value: it saves the madvise on warm pages which is a bit cheap single-threaded, but it contended and expensive under concurrency. To be fair, I admit that prefetch in real use case usually interleaves with other computations, so the contention may not be as severe as in this tight micro-benchmark loop.
Looking benchmarks at Linux NVMe for hot and mixed-hot-cold scenario, keeping isLoaded() check is better than always-prefetch except the acceptable loss in single threaded:
RandomReadIOBenchmark, hot: 33.3 / 224.1 / 256.6 (keepisLoaded) vs 34.0 / 169.4 / 216.7 (always-prefetch), T1/T8/T16- stored fields, hot: 2.14 / 13.86 (keep
isLoaded) vs 1.95 / 12.23 (always-prefetch), T1/T8 RandomReadIOBenchmark, mixed hot-warm: 6.55 / 44.9 / 61.2 (keepisLoaded) vs 7.13 / 43.8 / 60.4 (always-prefetch), T1/T8/T16- stored fields, mixed hot-warm: 0.56 / 1.0 (keep
isLoaded) vs. 0.55 / 0.99 (always-prefetch), T1/T8
Looking benchmarks at Linux NVMe for cold scenario. The extra isLoaded() is definitely overhead, but compare with EBS random-read latency ~340us, normal SSD ~100us, a performant NVMe 10–20us, the guard latency looks negligible relative to end-to-end I/O overall.
5. My 2 cents: a better backoff instead of removing it, and keep isLoaded()
So the problem really isn't "backoff yes/no", removing it is great for mixed warm/cold but bad for hot, there is no one-size-fits-all. I think the power-of-two backoff strategy is just a bit too aggressive, what if we soften it to balance hot vs. mixed hot-warm reads? I tried a slow-climbing backoff, fire madvise for the first 8 prefetch, then every Nth with N growing slowly, cap at 64 to force a prefetch. Tested on StoredFieldsPrefetchBenchmark (Linux NVMe, prefetch-then-read ops/ms, higher = better):
| thread | no prefetch | backoff (current impl.) | always-prefetch (this PR) | slow-climb (customized backoff) | |
|---|---|---|---|---|---|
| Hot 16G | T1 | 2.86 | 2.66 | 1.95 | 2.37 |
| Hot 16G | T8 | 17.65 | 16.83 | 12.23 | 15.15 |
| Boundary 32G (≈RAM) | T1 | 0.15 | 0.19 | 0.55 | 0.47 |
| Boundary 32G (≈RAM) | T8 | 1.30 | 1.32 | 0.99 | 1.53 |
In the hot rows, slow-climb sits just under the no-prefetch ceiling (only -9%, vs always-prefetch's -26%), nearly as cheap as the current backoff. At the boundary, T1 it does ~3.0x over its no-prefetch baseline (vs 1.4x for the current power-of-two backoff and ~3.7x for always-prefetch which is the best), a somewhat good middle ground.
We could explore a softer better backoff strategy alongside the isLoaded() guard, like I described, this is really a balance.
Appendix — raw results
Columns across all tables:
- backoff (current impl.) = current (Reduce the overhead of
IndexInput#prefetchwhen data is cached in RAM. #13381) - rm-backoff = remove backoff counter but keep
isLoaded() - always-prefetch = remove both (prefetch unconditionally)
- slow-climb = my customized backoff.
1. RandomReadIOBenchmark (reused from #16279) — RANDOM + batched prefetch, ops/ms (higher = better)
Numbers under test of mmapRandomBatchedPrefetch (RANDOM advice, prefetch the 16 random offsets then read). readSize=16K, readsPerOp=16.
Linux EBS (c5.4xlarge)
| Case | Threads | backoff (current impl.) | rm-backoff | always-prefetch | slow-climb |
|---|---|---|---|---|---|
| Warm 16G (hot) | T1 | 39.98 | 25.36 | 26.95 | 35.35 |
| Warm 16G | T4 | 151.18 | 96.67 | 85.65 | 130.40 |
| Warm 16G | T8 | 272.70 | 180.33 | 129.56 | 243.83 |
| Warm 16G | T16 | 298.66 | 218.23 | 179.06 | 279.37 |
| Mem pressure 32G (≈RAM) | T1 | 0.49 | 2.29 | 2.27 | 0.76 |
| Mem pressure 32G | T4 | 1.98 | 7.21 | 7.19 | 3.21 |
| Mem pressure 32G | T8 | 3.30 | 7.18 | 7.24 | 4.33 |
| Mem pressure 32G | T16 | 3.33 | 7.16 | 7.11 | 4.37 |
| Many cold 64G (no drop) | T1 | 0.64 | 1.58 | 1.52 | 1.63 |
| Many cold 64G | T4 | 1.50 | 1.65 | 1.66 | 1.74 |
| Many cold 64G | T8 | 1.50 | 1.64 | 1.65 | 1.71 |
| Many cold 64G | T16 | 1.47 | 1.64 | 1.65 | 1.70 |
| All cold 64G (drop cache) | T1 | 1.35 | 1.35 | 1.34 | 1.35 |
| All cold 64G | T4 | 1.35 | 1.35 | 1.35 | 1.35 |
| All cold 64G | T8 | 1.35 | 1.35 | 1.35 | 1.35 |
| All cold 64G | T16 | 1.36 | 1.36 | 1.37 | 1.37 |
Linux NVMe (c6id.4xlarge)
| Case | Threads | backoff (current impl.) | rm-backoff | always-prefetch | slow-climb |
|---|---|---|---|---|---|
| Warm 16G (hot) | T1 | 45.75 | 33.27 | 34.02 | 41.44 |
| Warm 16G | T4 | 170.53 | 122.45 | 111.72 | 154.45 |
| Warm 16G | T8 | 310.24 | 224.10 | 169.38 | 283.71 |
| Warm 16G | T16 | 313.69 | 256.63 | 216.72 | 315.65 |
| Mem pressure 32G (≈RAM) | T1 | 2.16 | 6.55 | 7.13 | 3.36 |
| Mem pressure 32G | T4 | 8.14 | 25.01 | 23.81 | 12.21 |
| Mem pressure 32G | T8 | 14.88 | 44.91 | 43.84 | 22.35 |
| Mem pressure 32G | T16 | 25.78 | 61.21 | 60.38 | 38.13 |
| Many cold 64G (no drop) | T1 | 2.61 | 3.11 | 3.16 | 3.09 |
| Many cold 64G | T4 | 7.21 | 7.21 | 7.18 | 7.21 |
| Many cold 64G | T8 | 7.21 | 7.21 | 7.18 | 7.21 |
| Many cold 64G | T16 | 7.21 | 7.21 | 7.18 | 7.19 |
| All cold 64G (drop cache) | T1 | 2.69 | 2.70 | 2.91 | 2.68 |
| All cold 64G | T4 | 6.24 | 6.21 | 6.24 | 6.24 |
| All cold 64G | T8 | 6.28 | 6.27 | 6.27 | 6.29 |
| All cold 64G | T16 | 6.68 | 6.67 | 6.66 | 6.65 |
Baseline mmapRandom (no prefetch) is constant baseline across branches, EBS T16 ~300 ops/ms warm / ~3.3 mem-pressure, NVMe T16 ~310 ops/ms / ~26 ops/ms.
2. Micro: isLoaded() vs madvise(WILLNEED), page-offset aligned, us/op (lower = better)
| Platform | Case | Threads | readSize | isLoaded() | madvise(WILLNEED) |
|---|---|---|---|---|---|
| EBS | warm | T1 | 4K | 1.114 | 0.676 |
| EBS | warm | T1 | 16K | 1.448 | 0.743 |
| EBS | warm | T8 | 4K | 1.712 | 2.940 |
| EBS | warm | T8 | 16K | 1.810 | 3.097 |
| EBS | cold | T1 | 4K | 0.805 | 47.467 |
| EBS | cold | T1 | 16K | 0.863 | 46.521 |
| EBS | cold | T8 | 4K | 1.306 | 390.060 |
| EBS | cold | T8 | 16K | 1.334 | 379.543 |
| NVMe | warm | T1 | 4K | 0.572 | 0.326 |
| NVMe | warm | T1 | 16K | 0.642 | 0.378 |
| NVMe | warm | T8 | 4K | 1.282 | 2.515 |
| NVMe | warm | T8 | 16K | 1.252 | 2.500 |
| NVMe | cold | T1 | 4K | 0.429 | 5.351 |
| NVMe | cold | T1 | 16K | 0.454 | 13.849 |
| NVMe | cold | T8 | 4K | 1.254 | 42.509 |
| NVMe | cold | T8 | 16K | 1.215 | 109.740 |
3. StoredFieldsPrefetchBenchmark (NVMe) — top-100 retrieval, ops/ms (higher = better)
Real workload: collect top-100 hits, prefetch a window of 16, then retrieve. docSizeBytes=4096, RANDOM advice. Note stored fields use an 80KB BEST_SPEED block by default, so each doc read faults/decompresses a large block, not 4KB.
Hot (16G index, fully cached)
| Method | Threads | backoff (current impl.) | rm-backoff | always-prefetch | slow-climb |
|---|---|---|---|---|---|
| retrieveNoPrefetch | T1 | 2.86 | 2.66 | 2.64 | 2.61 |
| retrieveNoPrefetch | T8 | 17.65 | 16.13 | 16.33 | 16.27 |
| prefetchThenRead | T1 | 2.66 | 2.14 | 1.95 | 2.37 |
| prefetchThenRead | T8 | 16.83 | 13.86 | 12.23 | 15.15 |
Memory pressure (32G ≈ RAM)
| Method | Threads | backoff (current impl.) | rm-backoff | always-prefetch | slow-climb |
|---|---|---|---|---|---|
| retrieveNoPrefetch | T1 | 0.134 | 0.150 | 0.150 | 0.156 |
| retrieveNoPrefetch | T8 | 1.165 | 1.255 | 1.296 | 1.307 |
| prefetchThenRead | T1 | 0.191 | 0.558 | 0.554 | 0.474 |
| prefetchThenRead | T8 | 1.322 | 1.007 | 0.986 | 1.534 |
50% cold (64G, ~half resident)
| Method | Threads | backoff (current impl.) | rm-backoff | always-prefetch | slow-climb |
|---|---|---|---|---|---|
| retrieveNoPrefetch | T1 | 0.042 | 0.040 | 0.042 | 0.042 |
| retrieveNoPrefetch | T8 | 0.321 | 0.310 | 0.321 | 0.320 |
| prefetchThenRead | T1 | 0.275 | 0.259 | 0.268 | 0.270 |
| prefetchThenRead | T8 | 0.286 | 0.259 | 0.266 | 0.267 |
8 participants
Description
The power-of-two back off in MemorySegmentIndexInput.prefetch() assumes consecutive page-cache hits suggest the file is warming up, so madvise overhead can be skipped until the next power-of-two call. That assumption breaks for ReadAdvice.RANDOM, where each request accesses unpredictable pages — a warm page in the past says nothing about whether the next page is warm (e.g. HNSW graph traversal touches a different path per query).
Add a
volatile boolean isRandomfield, set by updateReadAdvice() and also by slice(String, long, long, IOContext), the primary entry point for HNSW vector files, and skip the backoff in prefetch() when it is true and also skip resetting thesharedPrefetchCounter.The one drawback: for RANDOM-advised files that are fully warm in the page cache, isLoaded() is now called on every prefetch rather than being throttled. In practice this seems acceptable because isLoaded() on warm pages should be cheap, and a fully warm RANDOM file means prefetch will now return a more accurate answer than before.
On x86 (TSO) a volatile load before LOCK XADD requires no hardware fence and costs nothing measurable. On ARM the ldar before ldadd adds a small ordering cost, still dwarfed by the atomic itself. The miss-reset lambda returns to a plain set(0) with no special-casing.
I used claude to generate the tests and then I manually reviewed them.
I also considered encoding RANDOM mode as a negative sentinel in the counter itself (Integer.MIN_VALUE) to avoid any new field. Rejected: the counter walks toward zero on every getAndIncrement(), losing RANDOM mode after ~2B calls without a miss; preserving it through misses required a CAS loop (getAndUpdate) in the miss handler, adding complexity that doesn't seem necessary.