jpountz commented on code in PR #12374:
URL: https://github.com/apache/lucene/pull/12374#discussion_r1236671035
##########
lucene/core/src/java/org/apache/lucene/search/IndexSearcher.java:
##########
@@ -116,8 +117,11 @@ public class IndexSearcher {
protected final IndexReaderContext readerContext;
protected final List<LeafReaderContext> leafContexts;
- /** used with executor - each slice holds a set of leafs executed within one
thread */
- private final LeafSlice[] leafSlices;
+ /**
+ * used with executor - LeafSlice supplier where each slice holds a set of
leafs executed within
+ * one thread
+ */
+ private final CachingLeafSlicesSupplier leafSlicesSupplier;
Review Comment:
Can you add a comment that we're caching it instead of creating it eagerly
to avoid calling a protected method from a constructor, which is a bad practice?
##########
lucene/core/src/java/org/apache/lucene/search/IndexSearcher.java:
##########
@@ -1014,4 +1021,48 @@ private static SliceExecutor
getSliceExecutionControlPlane(Executor executor) {
return new SliceExecutor(executor);
}
+
+ /**
+ * Supplier for {@link LeafSlice} slices when passed in executor is
non-null. This supplier
+ * computes and caches the value on first invocation and returns cached
value on subsequent
+ * invocation. If the passed in provider for slice computation throws
exception then same will be
+ * passed to the caller of this supplier on each invocation. If the provider
returns null then
+ * {@link NullPointerException} will be thrown to the caller.
+ */
+ private static class CachingLeafSlicesSupplier implements
Supplier<LeafSlice[]> {
Review Comment:
Maybe also add a comment that this class is implementing the (subtle)
double-checked locking idiom and link to something like
https://shipilev.net/blog/2014/safe-public-construction/ that describes the
traps of it?
##########
lucene/core/src/java/org/apache/lucene/search/IndexSearcher.java:
##########
@@ -1014,4 +1021,48 @@ private static SliceExecutor
getSliceExecutionControlPlane(Executor executor) {
return new SliceExecutor(executor);
}
+
+ /**
+ * Supplier for {@link LeafSlice} slices when passed in executor is
non-null. This supplier
+ * computes and caches the value on first invocation and returns cached
value on subsequent
+ * invocation. If the passed in provider for slice computation throws
exception then same will be
+ * passed to the caller of this supplier on each invocation. If the provider
returns null then
+ * {@link NullPointerException} will be thrown to the caller.
+ */
+ private static class CachingLeafSlicesSupplier implements
Supplier<LeafSlice[]> {
+ private volatile LeafSlice[] leafSlices;
+
+ private final Executor executor;
+
+ private final Function<List<LeafReaderContext>, LeafSlice[]> sliceProvider;
+
+ private final List<LeafReaderContext> leaves;
+
+ private CachingLeafSlicesSupplier(
+ Executor executor,
+ Function<List<LeafReaderContext>, LeafSlice[]> provider,
+ List<LeafReaderContext> leaves) {
+ this.executor = executor;
+ this.sliceProvider = Objects.requireNonNull(provider, "leaf slice
provider cannot be null");
+ this.leaves = Objects.requireNonNull(leaves, "list of LeafReaderContext
cannot be null");
+ }
+
+ @Override
+ public LeafSlice[] get() {
+ if (executor == null) {
+ return null;
+ }
Review Comment:
It would make more sense to have to have the `executor == null` check in
`IndexSearcher#getSlices` than here so that this class focuses solely on
caching logic?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
