Make TaskExecutor cx public and use TaskExecutor for concurrent HNSW graph build #12799

shubhamvishu · 2023-11-11T20:31:30Z

Description

This PR proposes to :

Make the TaskExecutor public which is currently pkg-private - As we are making more use to concurrency and not restrictive at searching time eg: now at indexing time we concurrently create the hnsw graph (Concurrent HNSW Merge #12660). We could use the TaskExecutor implementation to do this for us.
Use TaskExecutor#invokeAll in HnswConcurrentMergeBuilder#build to run the workers concurrently.

javanna

Nice to see more parallelism introduced, and new ways to use the current TaskExecutor!

I initially wondered if it was a good idea to use TaskExecutor outside of the searcher, but it looks like it is already general enough to be able to do so. It has protection against deadlock (for concurrent tasks trying to parallelize further), which applies to any usage.

The fact that a separate instance is created makes sense to me, as the underlying executor won't necessarily be the one for search.

This seems like a good idea all in all.

@jpountz would you like to double check?

javanna

I left a small comment on exception handling. Could you add a changes entry please as well?

javanna · 2023-11-16T10:01:03Z

lucene/core/src/java/org/apache/lucene/util/hnsw/HnswConcurrentMergeBuilder.java

+            try {
+              workers[finalI].run(maxOrd);
+              return null;
+            } catch (IOException e) {


Is it necessary to catch IOException here?

I don't think so. I retained this as it is. I'll remove it in the next revision

shubhamvishu · 2023-11-16T10:58:51Z

@javanna I have added the CHANGES entry and addressed the comment. Seems the precommit check fails on :lucene:documentation:markdownToHtml task, which looks unrelated? Not sure.

benwtrent · 2023-11-16T13:23:30Z

lucene/core/src/java/org/apache/lucene/search/TaskExecutor.java

@@ -53,7 +53,7 @@ public final class TaskExecutor {

  private final Executor executor;

-  TaskExecutor(Executor executor) {
+  public TaskExecutor(Executor executor) {


Now that its public, you might have to add a JavaDoc here?

Ahh I see....just wondering why it wasn't caught locally with gradlew check. I think it should have? Also the initial revision passed successfully which had it public too. Dunno but maybe there is some opportunity for some sort of fix?

javanna

thanks for making the changes I requested, I found another small thing, sorry for not noticing that straight-away.

lucene/core/src/java/org/apache/lucene/util/hnsw/HnswConcurrentMergeBuilder.java

shubhamvishu · 2023-11-20T12:18:40Z

@javanna I have addressed the latest comments now. Thanks!

lucene/core/src/java/org/apache/lucene/util/hnsw/ConcurrentHnswMerger.java

javanna

I left one last small comment, sorry I could have made my earlier comment clearer and avoid more back and forth. Thanks for your patience. Could you also address the merge conflict in the changes file please?

lucene/core/src/java/org/apache/lucene/codecs/lucene99/Lucene99HnswVectorsWriter.java

…ent HNSW graph build

shubhamvishu · 2023-11-20T20:15:22Z

I left one last small comment, sorry I could have made my earlier comment clearer and avoid more back and forth. Thanks for your patience.

No problem. I have addressed your comment to hold the TE in the classes instead of ES in the latest revision. Thanks for reviewing @javanna !

Could you also address the merge conflict in the changes file please?

Done

javanna · 2023-11-21T10:41:58Z

lucene/core/src/java/org/apache/lucene/codecs/lucene99/Lucene99HnswVectorsFormat.java

   *     generated by this format to do the merge
   */
  public Lucene99HnswVectorsFormat(
-      int maxConn, int beamWidth, int numMergeWorkers, ExecutorService mergeExec) {
+      int maxConn, int beamWidth, int numMergeWorkers, TaskExecutor mergeExec) {


I would have rather left ExecutorService as a constructor argument, but create a TaskExecutor out of it and hold a reference to it instead of the executor. Does that make sense to you?

Sure I'll change it, but tbh I feel maybe it was better to keep the ES itself instead of storing the TE reference because ES is capable of doing other things as well that TE does not support yet eg: timeout support etc. I understand there is no use case as such currently in hnsw indexing side here but if maybe there could be some in future that would help in having this a bit extensible(?). Just wanted to make this point here.

Thanks for voicing your opinion. My main goal was to create one instance of TaskExecutor per Executor, hence create it as soon as possible. I also believe that we will want to extend TaskExecutor's capabilities instead of relying on executor service capabilities in specific places. The idea would be that we use TaskExecutor for general task execution, and that is used in different places like query rewrite, search against slices, loading of terms statistics, and now also building the graph.

@javanna Totally makes sense to me! I have addressed your comment in the latest commit. Please have a look. Thanks!

The idea would be that we use TaskExecutor for general task execution, and that is used in different places like query rewrite, search against slices, loading of terms statistics, and now also building the graph.

+1 to extend TE capabilities and use it for all concurrency use cases.

javanna

LGTM

javanna · 2023-11-21T16:24:14Z

Thanks @shubhamvishu !

…graph build (apache#12799) Make the TaskExecutor public which is currently pkg-private. At indexing time we concurrently create the hnsw graph (Concurrent HNSW Merge apache#12660). We could use the TaskExecutor implementation to do this for us. Use TaskExecutor#invokeAll in HnswConcurrentMergeBuilder#build to run the workers concurrently.

javanna approved these changes Nov 13, 2023

View reviewed changes

javanna reviewed Nov 16, 2023

View reviewed changes

shubhamvishu force-pushed the taskexecutor-public branch from 61a5f33 to c374e20 Compare November 16, 2023 10:47

benwtrent reviewed Nov 16, 2023

View reviewed changes

javanna reviewed Nov 16, 2023

View reviewed changes

lucene/core/src/java/org/apache/lucene/util/hnsw/HnswConcurrentMergeBuilder.java Outdated Show resolved Hide resolved

shubhamvishu force-pushed the taskexecutor-public branch from 6a8d8cd to 3456d7d Compare November 20, 2023 12:16

javanna reviewed Nov 20, 2023

View reviewed changes

lucene/core/src/java/org/apache/lucene/util/hnsw/ConcurrentHnswMerger.java Outdated Show resolved Hide resolved

javanna reviewed Nov 20, 2023

View reviewed changes

lucene/core/src/java/org/apache/lucene/codecs/lucene99/Lucene99HnswVectorsWriter.java Outdated Show resolved Hide resolved

shubhamvishu added 4 commits November 21, 2023 01:28

Make TaskExecutor constructor public and use TaskExecutor for concurr…

7ce8a7a

…ent HNSW graph build

Add javadoc

361ddd3

Create TaskExecutor in ConcurrentHnswMerger and pass it down

8a14935

Only use TaskExecutor for concureent hnsw indexing

e42abc7

shubhamvishu force-pushed the taskexecutor-public branch from 8a24931 to e42abc7 Compare November 20, 2023 20:12

javanna reviewed Nov 21, 2023

View reviewed changes

javanna mentioned this pull request Nov 21, 2023

Add static function in TaskExecutor to retrieve the results for a collection of Future #12798

Closed

Some more changes

8410a18

javanna approved these changes Nov 21, 2023

View reviewed changes

javanna added this to the 9.9.0 milestone Nov 21, 2023

javanna merged commit 4628327 into apache:main Nov 21, 2023
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make TaskExecutor cx public and use TaskExecutor for concurrent HNSW graph build #12799

Make TaskExecutor cx public and use TaskExecutor for concurrent HNSW graph build #12799

shubhamvishu commented Nov 11, 2023

javanna left a comment

javanna left a comment

javanna Nov 16, 2023

shubhamvishu Nov 16, 2023

shubhamvishu commented Nov 16, 2023 •

edited

Loading

benwtrent Nov 16, 2023

shubhamvishu Nov 16, 2023 •

edited

Loading

javanna left a comment

shubhamvishu commented Nov 20, 2023

javanna left a comment

shubhamvishu commented Nov 20, 2023 •

edited

Loading

javanna Nov 21, 2023

shubhamvishu Nov 21, 2023 •

edited

Loading

javanna Nov 21, 2023 •

edited

Loading

shubhamvishu Nov 21, 2023

javanna left a comment

javanna commented Nov 21, 2023

Make TaskExecutor cx public and use TaskExecutor for concurrent HNSW graph build #12799

Make TaskExecutor cx public and use TaskExecutor for concurrent HNSW graph build #12799

Conversation

shubhamvishu commented Nov 11, 2023

Description

javanna left a comment

Choose a reason for hiding this comment

javanna left a comment

Choose a reason for hiding this comment

javanna Nov 16, 2023

Choose a reason for hiding this comment

shubhamvishu Nov 16, 2023

Choose a reason for hiding this comment

shubhamvishu commented Nov 16, 2023 • edited Loading

benwtrent Nov 16, 2023

Choose a reason for hiding this comment

shubhamvishu Nov 16, 2023 • edited Loading

Choose a reason for hiding this comment

javanna left a comment

Choose a reason for hiding this comment

shubhamvishu commented Nov 20, 2023

javanna left a comment

Choose a reason for hiding this comment

shubhamvishu commented Nov 20, 2023 • edited Loading

javanna Nov 21, 2023

Choose a reason for hiding this comment

shubhamvishu Nov 21, 2023 • edited Loading

Choose a reason for hiding this comment

javanna Nov 21, 2023 • edited Loading

Choose a reason for hiding this comment

shubhamvishu Nov 21, 2023

Choose a reason for hiding this comment

javanna left a comment

Choose a reason for hiding this comment

javanna commented Nov 21, 2023

shubhamvishu commented Nov 16, 2023 •

edited

Loading

shubhamvishu Nov 16, 2023 •

edited

Loading

shubhamvishu commented Nov 20, 2023 •

edited

Loading

shubhamvishu Nov 21, 2023 •

edited

Loading

javanna Nov 21, 2023 •

edited

Loading