Better encapsulate locking logic in HnswGraphBuilder

[viliam-durina]

This PR moves the locking logic from HnswConcurrentMergeBuilder to HnswGraphBuilder, which automatically picks the single-threaded vs. concurrent searcher based on whether a lock is used. This makes it possible to use concurrent graph building outside of the context of a merge process.

This PR is a pure refactoring.

[msokolov]

I'm not sure we need this because we guarantee that an index cannot be written while it is being read under normal circumstances, and I think it's actually good to expose less functionality if there is no purpose for it, to avoid traps. Did you have a use case in mind?

[viliam-durina]

Our use case is to speed-up indexing of larger segments. We want to build fewer segments, so it makes sense to build them on multiple cores. We build the segments directly, not building smaller segments first, and then merging.

The HnswGraphSearcher is used internally when building the graph. When the graph is built concurrently, you need to copy the neighborArray while holding the lock, which is implemented in a private inner class within HnswConcurrentMergeBuilder. This PR moves that inner implementation to HnswGraphBuilder - this way the locking logic is better encapsulated there - if a lock is given, it will use the concurrent searcher using the lock. If there's no lock, it will use the unsynchronized version.

I understand this PR is for a non-standard use of the API and you might not care, and that the current structure works for your case. But I think the new structure is more logical anyway.

[msokolov]

I see, thanks for the explanation: it makes more sense to me now. We want to be able to use concurrency while indexing the initial segment, even before flushing, sure.

[msokolov]

Even though this change does not alter the public API, I don't think it is a pure refactoring. Won't it change the execution plan used during indexing? Or -- maybe it would actually be a no-op because the IndexWriter creates a unique segment for each of its threads, so indexing a single segment is always single-threaded. Have you actually tried this out and seen some improvement?

[viliam-durina]

Won't it change the execution plan used during indexing?

As far as I'm aware, it doesn't.

Or -- maybe it would actually be a no-op because the IndexWriter creates a unique segment for each of its threads, so indexing a single segment is always single-threaded.

Creating a new segment is always single-threaded in the current version. It can be parallel when merging segments - to enable it, you need to pass a value >=1 to numMergeWorkers argument of Lucene99HnswVectorsFormat. In my own code I'm implementing concurrent graph building when initially adding the documents. For this, I needed the MergeSearcher outside of the context of merging - the locking should ideally be a business of the builder, not of the merger, so I thought the move is logical in general, so I created this PR.

Checked the existing tests, I didn't find a test that would test the concurrent merging. However I tested it in my code, and it seems to work...