Refactor org.eclipse.help.internal.search.SearchIndex#openSearcher()

[howlger]

In org.eclipse.help.internal.search.SearchIndex, refactor the openSearcher() method to return an AutoCloseable object so that it can be used in a try-with-resources statement that implicitly registers and unregisters the search.

Minor performance improvement: Use double-checked locking idiom for lazy initialization of the IndexSearcher.

Grant access to the searches and analyzerDescriptor fields via SearcherInfo#getIndexSearcher() and SearcherInfo#getAnalyzerDescriptor(). These two getter methods are not (yet) used internally. Their only purpose is to be able to implement a hybrid search that combines a semantic search with a sparse keyword search. And the sparse keyword search uses the existing SearchIndex, which requires access to these fields.

[github-actions]

Test Results

 1 758 files  ±0   1 758 suites  ±0   1h 27m 43s ⏱️ -10s
 4 174 tests ±0   4 151 ✅ ±0   23 💤 ±0  0 ❌ ±0 
13 119 runs  ±0  12 952 ✅ ±0  167 💤 ±0  0 ❌ ±0 

Results for commit e26c56d. ± Comparison against base commit 4a4172c.

♻️ This comment has been updated with latest results.

[howlger]

@iloveeclipse This is a work for Advantest.

The only purpose of this change is to add a method to an internal class. We want to use this and #1725 to enhance the local help search with the help of semantic search. More precisely, a hybrid search consisting of a semantic search and a sparse keyword search based on the existing internal local search to enhance the top search results by adding hits and/or changing the ranking. The limitation of hybrid search is that help content needs to be shipped with a pre-built index of the embeddings, also called a vector store, because indexing takes significantly longer for semantic search than for keyword search, where a pre-built index is optional.

Advantest intends to open source and contribute the hybrid search to Eclipse, for which some work still needs to be done.

[iloveeclipse]

The build failed for no obvious reason in unrelated code. I've retriggered build now.

[iloveeclipse]

The build failed for no obvious reason in unrelated code.

It is because your PR is FAR behind master! I will rebase now.

[iloveeclipse]

Same as on other PR: could you provide a simple test that uses this new method (even if it is experimental)?
Could you please check if there are any related tests that could be updated in org.eclipse.ua.tests.help.search.AllSearchTests ?

[iloveeclipse]

The last build failed due heap space problem, not related to this PR change. I've re-triggered that again.

[howlger]

Same as on other PR: could you provide a simple test that uses this new method (even if it is experimental)? Could you please check if there are any related tests that could be updated in org.eclipse.ua.tests.help.search.AllSearchTests ?

I'm not sure if this is really required as the new experimental method customSearch(...) is basically a copy of the search(...) where for the inner part a custom implementation can be used. But if it is wanted, I will try to add a test for it.

[iloveeclipse]

But if it is wanted, I will try to add a test for it.

The point is: there is an internal public method without uses. As a responsible maintainer I would tend to remove it, simply because any extra unused code is wasting our time/resources. If there is an example test that shows that the code does something useful, I would not touch that and even fix that if it should later fail.

[jukzi]

I would prefer not to add this unreferenced code but to make the needed methods more public. is "protected" enough?

[iloveeclipse]

@howlger : please if you push your updated changes, before doing that rebase your branch on latest state. It just wastes time to run tests on ancient platform state that doesn't compile anymore. I will rebase now.

[howlger]

I would prefer not to add this unreferenced code but to make the needed methods more public. is "protected" enough?

Your feedback gave me an idea and I have removed the customSearch(...) method and instead extended the public openSearcher() method to return an AutoCloseable. This way it can be used in a try-with-resources statement that registers and unregisters the search. openSearcher() has been optimized by the double-checked locking idiom and the existing search(...) has been refactored to use openSearcher() in a try-with-resources statement.

[howlger]

@howlger : please if you push your updated changes, before doing that rebase your branch on latest state. It just wastes time to run tests on ancient platform state that doesn't compile anymore. I will rebase now.

@iloveeclipse Thanks, I missed that. Please note that I have now chosen a different approach which IMHO speeds up the existing search a bit and improves the readability of the code a bit.

[iloveeclipse]

Build failed again due heap space issues (unrelated to the changes here)... I've restarted it.

[iloveeclipse]

Your feedback gave me an idea and I have removed the customSearch(...) method and instead extended the public openSearcher() method to return an AutoCloseable. This way it can be used in a try-with-resources statement that registers and unregisters the search. openSearcher() has been optimized by the double-checked locking idiom and the existing search(...) has been refactored to use openSearcher() in a try-with-resources statement.

Indeed, the code is cleaner now. Thanks.

[howlger]

Thanks everyone for your feedback and for applying.