Can backward reading be less adversarial to BufferedIndexInput?

[jpountz]

Description

Forked from https://lists.apache.org/thread/1fskhmz84pp60o41txsxj2193vt9txod: if BufferedIndexInput is asked to read N bytes backward, one after the other, it will internally trigger N refills of its internal 1kB buffer. While this particular case may be better fixed by changing the layout of the data (#12355), there are a couple other cases when we occasionally need to go backwards and it would be nice if BufferedIndexInput handled it more gracefully (e.g. when navigating to a parent node in the BKD index)?

[romseygeek]

Maybe the simplest change to make here would be in BufferedIndexInput.readXXX(pos), when pos is less than bufferStart, we reset bufferStart to Math.min(pos, bufferStart - bufferSize)? In other words, assume that once we're trying to read backwards we are likely to continue to do so and pull in the full previous page of bytes.

[jpountz]

I reviewed some of the other places that we have that may read backwards:

• The BKD index may jump backwards when moving from a node to the parent node, but then reading information about this parent node involves forward reads, and moving to a child node would translate into a forward read as well.
• The stored fields and term vectors indexes perform binary searches on RandomAccessIndexInput, so there are occasional backward reads as well.

I'm expecting this issue to raise the question of whether it's actually a good idea to read the terms index (and BKD index, and stored fields index, and term vectors index) off-heap, or if we should load it in heap like we used to in the past. As far as I'm concerned, I'd really like to keep loading it off-heap by default, this change has simplified many things, e.g. we no longer have RAM accounting on our readers, we almost don't need to take data size into account for sizing the JVM heap, etc.

@romseygeek I worry that the assumption that observing one backward read means that the next one is likely going to be backward too might be too strong, e.g. we'd have the same issue that every read needs to perform a refill if backward and forward reads are interleaved, which could happen as part of the binary search for instance. Maybe we could meet half way instead and set bufferStart so that pos is in the middle of the buffer when refilling due to a backward read?

[romseygeek]

we'd have the same issue that every read needs to perform a refill if backward and forward reads are interleaved

I think this would only be the case if the target of the binary search was precisely on the boundary of the buffer pages? Which could happen with whatever paging strategy we use.

[jpountz]

Thanks, I think you're right that the binary search scenario I had in mind would not be impacted.

[mikemccand]

Could we improve IOContext to let the Directory impl know that we may read backwards, or more "random accessy"? We seem to know the few places in Lucene's sources where we do this (thanks @jpountz).

[jpountz]

We added IOContext#load for this purpose recently (#11930), it tells the directory that the index input is going to be hot and accessed in a random fashion. We use it for the terms, points, stored fields and term vectors indexes.

[jpountz]

One thing worth noting is that Alan's change only modifies the behavior when reading index files via the RandomAccessInput interface. If you read a file via the IndexInput abstraction, the behavior doesn't change.