Add multi-thread searchability to OnHeapHnswGraph

[zhaih]

Description

Implemented a searcher specific to address the thread-safety problem on searching on OnHeapHnswGraph

Idea: #12246 (comment)

Test

A test case comparing the search using 4 threads and new class with single thread and old one.

[jimczi]

Thanks @zhaih , did you see the other PR that would also allow multi-threaded insertion? Adding multi-threading just for search would be misleading imo since this class exposes insertion too.

[zhaih]

Hi @jimczi, thanks for the comment. Yeah I'm aware of that PR and also looking forward to that. However I decided to make this PR because:

1. This can resolve the thread-safety issue faster, as I would imagine we will iterate on that PR a bit longer.
2. I'm not sure whether the concurrent writable & readable HNSW graph will completely replace the current one, citing the author's own words:

With the optimizations it's still about 38% slower than the non-concurrent code (including the HashMap optimization for the non-concurrent that went in).

I think the original impl still has it's own value in case where people don't necessarily need concurrent write, so have a thread-safe search solution may still be desirable? (Especially when it is quite easy to make one)

As for

Adding multi-threading just for search would be misleading imo since this class exposes insertion too.

Yes I agree, but I think it's more of a javadoc issue? If we documented it clearly that HNSW graph is not supposed to be written and read at the same time unless explicitly stated, maybe it is more acceptable?
Just like HashMap, ArrayList and tons of other data structures that are not supposed to be safe when written & read together but expose an insertion API, if we don't advertise it as "totally safe" then I don't think people should have that impression?

[alessandrobenedetti]

Hi @zhaih , I think your idea can be a quick workaround, I like it, in its simplicity, I take the freedom to add a commit on top of it to make it more usable and compilation friendly, it's just an idea so feel free to revert the commit/ have discussion on top of it.

[alessandrobenedetti]

@zhaih , @jimczi take a look to my last commit, it's intended as a draft, just to spark additional discussion, it's not meant in no way to override/diminish your work.
The rationale is to force users to choose the searcher depending on the graph they use, in this way, a non-thread safe execution will fail at compile time.

Tests should be green and if you like the idea, we shouldn't need the comment "*

Note: if you want to search {@link OnHeapHnswGraph} in a thread-safety manner, please
* consider using {@link OnHeapHnswGraphSearcher}"

[zhaih]

Thanks @alessandrobenedetti for your idea, but sorry I am a bit against the OffHeapHnswSearcher as it isn't thread-safe either: in normal search we achieve it by creating a new copy of graph for each search (source).

I think what HnswGraphSearcher did is ok: it uses the API set that HnswGraph provided to do the search. And we definitely should warn users that it does not guarantee thread safety and it varies on different graph implementation: like if user put the future ConcurrentHnswGraph there it should be able to just work with multi-threading out of box.

[alessandrobenedetti]

Hi @zhaih, the main intent of my commits was not to make searching an OffHeap graph thread safe.
The main problem I see at the moment is that we delegate to a comment the responsibility of indicating to users 'the better searcher for OnHeap graphs'.
I looked around and when we use a graph searcher, as far as I saw, it's always known of we are dealing with an OffHeap or OnHeap version.
Hence the reason I made it explicit in the classes, so it won't be possible to have a choice: if you deal with the OnHeap you must use the OnHeapSearcher and if you deal with the OffHeap, you use the OffHeapSearcher.
For example with your original commits the bug we have on the SynonymProvider will still be there and no compilation error will tell us there's actually a better graphSearcher for the use case.
I hope this makes my contribution here clearer, and what I proposed is not meant to be the perfect solution but I believe we shouldn't ignore the problem and build a similar solution (happy to evaluate better approaches if any!)

[zhaih]

@alessandrobenedetti thanks for explaining, altho I still don't like the idea of creating an OffHeapHnswGraphSearcher but use only APIs provided by HnswGraph, but to address your concern that user may not aware of there's a better searcher, I made a new change:
instead of making a new OnHeapHnswGraphSearch.search API I overloaded the old method so that when users are using OnHeapHnswGraph they will automatically use the thread-safe version, how do you think about that?

[alessandrobenedetti]

I like this pull request, It's not invasive and I think it solves the problem of concurrently searching OnHeap HNSW graphs!
+1 from my side

[zhaih]

Thanks @alessandrobenedetti , I'll wait a day to give @msokolov and @jimczi a chance to review before merging it!

[jimczi]

Sorry but that doesn't make sense to me to work/review this PR while #12254 is under review. I also disagree with the goal here, the mutable hnsw graph is used internally by the indexer, making it multi-threaded just for search is an external requirement. The other PR started with the goal of speeding up the building by using multiple threads. That seems reasonable to me. Using multiple threads just for the sake of this token filter is a non-goal imo.
Can you follow the progress of #12254 and plug your changes there?

[zhaih]

Sorry @jimczi I am not sure I get your idea.
Did you mean:

1. We shouldn't make improvement to this OnHeapHnswGraph right now because the other PR is ongoing and it will likely replace this one so what we're doing here will be deprecated eventually.
   Or:
2. We shouldn't make improvement to this OnHeapHnswGraph for reason that is not for our main use case: which is indexing.

I actually don't think this PR is that related to the other one, actually this one can be benefit to the other one as that one is currently using ThreadLocal internally to solve the problem this PR is solving. If this PR is checked in, I believe the other one might be able to abandon the ThreadLocal.

[jimczi]

I think both 1 and 2 yes. If you think this PR will benefit the other PR then it should be discussed in #12254. We already have issues with indexing so adding new requirements outside of this use case should be avoided imo.
The thing I don't like with this change alone is that it makes it like if using the HNSW builder for a search use case is ok.
That's not right, nobody should use the HNSW builder as a static searcher. If we make it multi-threaded it should be to make the build faster. Search is exposed in the builder simply because it is used by the algorithm to create the graph.

[zhaih]

I'm ok to just pause it here or eventually drop this PR as I'm definitely not the one who need this feature (at least for now). But I guess if @jimczi you want to stop people from using the OnHeapHnswGraph besides indexing, the better place is to stop (since it's merged then probably revert) this one #12169

That's not right, nobody should use the HNSW builder as a static searcher.

BTW we're using HnswGraph but not HnswGraphBuilder here, but if by builder you mean that graph has a public insertion API then yes it does.

[alessandrobenedetti]

I still think this contribution to be valuable, as I don't like much the fact that OnHeapGraph is stateful.
But I agree that the other contribution is solving a very similar problem.
@zhaih would be ok for you to try to get involved in the other contribution, trying to align your intent with the guy in there?
If you don't have the time, I'll try to find some, by the end of the week, as it's a very interesting topic.

In regard to avoiding people using the OnHeapHnswHraph outside the builder, I suspect we'll need to restructure the code in some way, as simple comments won't prevent future people to not using public classes.
And by the way, I also agree that we can improve the Word2Vec synonym filter in the future, changing the OnHeap approach with an OffHeap one, but rather than reverting it (and very likely lose the contribution) I do believe that incremental updates are the key here.

[alessandrobenedetti]

Another option, to be honest, is also to merge this first, and then the other committer will have to resolve the conflicts.
This one is a very minimal change and it seems an improvement to me, so it doesn't seem to me like a massive problem for the other pull request to deal with.

Rephrasing, @jimczi , is there any reason you believe the contribution from @zhaih to not be an improvement?
And no builder is involved in here as far as I reviewed.

[zhaih]

would be ok for you to try to get involved in the other contribution, trying to align your intent with the guy in there?

I'll for sure trying to review that PR, but that one is trying to introduce a new data structure while this one is actually trying to add a quick workaround for HnswGraph's API (which discourage thread-safe), there's no conflict from the beginning IMO.

is there any reason you believe the contribution from @zhaih to not be an improvement?

In my understanding @jimczi is trying to say this change some what encourages the use of OnHeapHnswGraph, which originally is only used to build the graph in memory and shouldn't be used outside.

I'm ok with that, but to be fair: this PR is not trying to promote the use of that, it is trying to solve a potential issue in the current trunk. And the real factor that encourages the use is:

1. It is a public class, even without lucene.internal tag
2. All related tools (Builder, Searcher) are public available
3. There's already an example of using it being recently added and no people are against it.

I'm not sure what all the solutions are, but stopping this PR won't solve any of the problems above, and will only make the thread-safety problem stays in the trunk longer.

[alessandrobenedetti]

I agree to your points @zhaih , thinking more about it, to me this is just a minor contribution and an improvement.

Although I value also Jim's insights, I don't think they are blockers and I would suggest to merge on monday (or when it pleases you Patrick) unless @jimczi has some pragmatic evidence this causes any regression.

The other PR can manage to solve the conflicts later if any, that alone shouldn't block improvements like this.