Use read advice consistently in the knn vector formats

[jimczi]

This change reverts #13985 and makes sure each knn format sticks to a single read advice consistently.
Switching read advice during merges might help some use cases, but it can also hurt others—e.g. when search and merges are running at the same time. To balance this, the approach here picks one read advice per format, focusing on what’s most resource-intensive for that format.

For formats using HNSW, the read advice is set to RANDOM and doesn’t change during merges. Copying bytes from old segments to new ones is much faster than re-building the graph, so keeping RANDOM read advice makes the most sense.

For flat formats, the read advice is set to SEQUENTIAL, as brute-force is the only way to retrieve nearest neighbors.

This is a deliberate decision to keep things simple and predictable. While it might seem like a step back compared to #13985, using multiple read advices on the same file can lead to unpredictable behavior—it might seem fine until you test it in a constrained setup.

That said, we could still improve merge performance with a RANDOM read advice in the future, for instance, by adding eager prefetching.

[ChrisHegarty]

I agree with the direction here. The anti-delta looks correct to me. I left a few comments.

[ChrisHegarty]

++ allowing to pass the read advice here is good, since the higher-level usage of this format really should dictate the intended usage.

[navneet1v]

I like the idea of passing the read advice. If the top level format can dictate the read advice then that makes code more better. +1 on this idea.

[shatejas]

I understand this and I think overall keeping a constant read advise works, just want to see if there is a path forward for optimizing merge.

e.g. when search and merges are running at the same time

@jimczi, #13985 was put out after a discussion since there was the this tradeoff being made. You can find the discussion here #13920 (comment).

using multiple read advices on the same file can lead to unpredictable behavior—it might seem fine until you test it in a constrained setup.

The experiments seemed to suggest it picked the last advice. I understand the setup wasn't constrained, would you be able to share the details of constrained setup where this caused unpredictable behavior. Just curious

That said, we could still improve merge performance with a RANDOM read advice in the future, for instance, by adding eager prefetching.

Are we sure that introducing prefetch during merge won't impact search?

A thought here would be to see the behavior if the read advice is updated (and revert after merge is over) to normal since sequential read advice can be aggressive as per documentation. Reading ahead of pages will then be decided by kernel heuristics for normal read advice as per my understanding.

@uschindler @jpountz any thoughts or suggestions?

[jimczi]

Thanks for looking @shatejas

The opensearch-project/k-NN#2134 (comment) seemed to suggest it picked the last advice. I understand the setup wasn't constrained, would you be able to share the details of constrained setup where this caused unpredictable behavior. Just curious

I may have overstated my point, I didn’t test it directly. My main concern is that the advice applies for the entire duration of the merge. Since the vector copy occurs at the start and represents only a small portion of the total merge time, I believe using prefetch instead of modifying the advice would be more suitable. Prefetch would allow us to fully control the readahead behavior.

Are we sure that introducing prefetch during merge won't impact search?

It would, especially when merging big segments, but only for the time of the copy, which should be relatively fast, rather than for the entire duration of the merge.

[shatejas]

I may have overstated my point, I didn’t test it directly. My main concern is that the advice applies for the entire duration of the merge. Since the vector copy occurs at the start and represents only a small portion of the total merge time, I believe using prefetch instead of modifying the advice would be more suitable. Prefetch would allow us to fully control the readahead behavior.

On a high level this makes a lot of sense to me. Since we can control the offset and the length in prefetch, it wouldn't be as aggressive as sequential advice and the impact of other parts like search would not be as much in theory. Thanks @jimczi for the explanation. I may take a stab at prefetch solution

[github-actions]

This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the [email protected] list. Thank you for your contribution!