[GitHub] [lucene-solr] atris commented on a change in pull request #823: LUCENE-8939: Introduce Shared Count Early Termination In Parallel Search
atris commented on a change in pull request #823: LUCENE-8939: Introduce Shared
Count Early Termination In Parallel Search
URL: https://github.com/apache/lucene-solr/pull/823#discussion_r320344708
##
File path: lucene/core/src/java/org/apache/lucene/search/IndexSearcher.java
##
@@ -467,9 +467,19 @@ public TopDocs searchAfter(ScoreDoc after, Query query,
int numHits) throws IOEx
final CollectorManager manager = new
CollectorManager() {
+ private HitsThresholdChecker hitsThresholdChecker;
@Override
public TopScoreDocCollector newCollector() throws IOException {
-return TopScoreDocCollector.create(cappedNumHits, after,
TOTAL_HITS_THRESHOLD);
+
+if (hitsThresholdChecker == null) {
+ if (executor == null || leafSlices.length <= 1) {
+hitsThresholdChecker =
HitsThresholdChecker.create(TOTAL_HITS_THRESHOLD);
+ } else {
+hitsThresholdChecker =
HitsThresholdChecker.createShared(TOTAL_HITS_THRESHOLD);
+ }
+}
Review comment:
Fixed this by using `final` modifier
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] [lucene-solr] atris commented on a change in pull request #823: LUCENE-8939: Introduce Shared Count Early Termination In Parallel Search
atris commented on a change in pull request #823: LUCENE-8939: Introduce Shared
Count Early Termination In Parallel Search
URL: https://github.com/apache/lucene-solr/pull/823#discussion_r320343276
##
File path:
lucene/core/src/java/org/apache/lucene/search/TopScoreDocCollector.java
##
@@ -191,32 +195,63 @@ public static TopScoreDocCollector create(int numHits,
int totalHitsThreshold) {
* objects.
*/
public static TopScoreDocCollector create(int numHits, ScoreDoc after, int
totalHitsThreshold) {
+return create(numHits, after,
HitsThresholdChecker.create(totalHitsThreshold));
+ }
+
+ static TopScoreDocCollector create(int numHits, ScoreDoc after,
HitsThresholdChecker hitsThresholdChecker) {
if (numHits <= 0) {
throw new IllegalArgumentException("numHits must be > 0; please use
TotalHitCountCollector if you just need the total hit count");
}
-if (totalHitsThreshold < 0) {
- throw new IllegalArgumentException("totalHitsThreshold must be >= 0, got
" + totalHitsThreshold);
+if (hitsThresholdChecker == null) {
+ throw new IllegalArgumentException("hitsThresholdChecker must be non
null");
}
if (after == null) {
- return new SimpleTopScoreDocCollector(numHits, totalHitsThreshold);
+ return new SimpleTopScoreDocCollector(numHits, hitsThresholdChecker);
} else {
- return new PagingTopScoreDocCollector(numHits, after,
totalHitsThreshold);
+ return new PagingTopScoreDocCollector(numHits, after,
hitsThresholdChecker);
}
}
- final int totalHitsThreshold;
+ /**
+ * Create a CollectorManager which uses a shared hit counter to maintain
number of hits
+ */
+ public static CollectorManager
createSharedManager(int numHits, FieldDoc after,
+
int totalHitsThreshold) {
+return new CollectorManager<>() {
+
+ @Override
+ public TopScoreDocCollector newCollector() throws IOException {
+return TopScoreDocCollector.create(numHits, after,
HitsThresholdChecker.createShared(totalHitsThreshold));
Review comment:
Fixed
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] [lucene-solr] atris commented on a change in pull request #823: LUCENE-8939: Introduce Shared Count Early Termination In Parallel Search
atris commented on a change in pull request #823: LUCENE-8939: Introduce Shared
Count Early Termination In Parallel Search
URL: https://github.com/apache/lucene-solr/pull/823#discussion_r320343227
##
File path: lucene/core/src/java/org/apache/lucene/search/TopFieldCollector.java
##
@@ -410,10 +423,34 @@ public static TopFieldCollector create(Sort sort, int
numHits, FieldDoc after,
throw new IllegalArgumentException("after.fields has " +
after.fields.length + " values but sort has " + sort.getSort().length);
}
- return new PagingFieldCollector(sort, queue, after, numHits,
totalHitsThreshold);
+ return new PagingFieldCollector(sort, queue, after, numHits,
hitsThresholdChecker);
}
}
+ /**
+ * Create a CollectorManager which uses a shared hit counter to maintain
number of hits
+ */
+ public static CollectorManager
createSharedManager(Sort sort, int numHits, FieldDoc after,
+
int totalHitsThreshold) {
+return new CollectorManager<>() {
+
+ @Override
+ public TopFieldCollector newCollector() throws IOException {
+return create(sort, numHits, after,
HitsThresholdChecker.createShared(totalHitsThreshold));
Review comment:
Yeah, rebasing error, thanks for highlighting -- fixed
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] [lucene-solr] atris commented on a change in pull request #823: LUCENE-8939: Introduce Shared Count Early Termination In Parallel Search
atris commented on a change in pull request #823: LUCENE-8939: Introduce Shared
Count Early Termination In Parallel Search
URL: https://github.com/apache/lucene-solr/pull/823#discussion_r320184927
##
File path: lucene/core/src/java/org/apache/lucene/search/IndexSearcher.java
##
@@ -467,9 +467,19 @@ public TopDocs searchAfter(ScoreDoc after, Query query,
int numHits) throws IOEx
final CollectorManager manager = new
CollectorManager() {
+ private HitsThresholdChecker hitsThresholdChecker;
@Override
public TopScoreDocCollector newCollector() throws IOException {
-return TopScoreDocCollector.create(cappedNumHits, after,
TOTAL_HITS_THRESHOLD);
+
+if (hitsThresholdChecker == null) {
+ if (executor == null || leafSlices.length <= 1) {
+hitsThresholdChecker =
HitsThresholdChecker.create(TOTAL_HITS_THRESHOLD);
+ } else {
+hitsThresholdChecker =
HitsThresholdChecker.createShared(TOTAL_HITS_THRESHOLD);
+ }
+}
Review comment:
Meaning, in the constructor? We will have to create a named implementation
of the `CollectorManager` for that
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] [lucene-solr] atris commented on a change in pull request #823: LUCENE-8939: Introduce Shared Count Early Termination In Parallel Search
atris commented on a change in pull request #823: LUCENE-8939: Introduce Shared
Count Early Termination In Parallel Search
URL: https://github.com/apache/lucene-solr/pull/823#discussion_r320019631
##
File path:
lucene/core/src/java/org/apache/lucene/search/HitsThresholdChecker.java
##
@@ -0,0 +1,119 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.lucene.search;
+
+import java.util.concurrent.atomic.LongAdder;
+import java.util.function.BooleanSupplier;
+
+/**
+ * Used for defining custom algorithms to allow searches to early terminate
+ */
+abstract class HitsThresholdChecker implements BooleanSupplier {
+ /**
+ * Implementation of HitsThresholdChecker which allows global hit counting
+ */
+ private static class GlobalHitsThresholdChecker extends HitsThresholdChecker
{
+private final int totalHitsThreshold;
+private final LongAdder globalHitCount;
Review comment:
Reverted to AtomicLong per discussion
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] [lucene-solr] atris commented on a change in pull request #823: LUCENE-8939: Introduce Shared Count Early Termination In Parallel Search
atris commented on a change in pull request #823: LUCENE-8939: Introduce Shared Count Early Termination In Parallel Search URL: https://github.com/apache/lucene-solr/pull/823#discussion_r319729736 ## File path: lucene/CHANGES.txt ## @@ -45,6 +45,11 @@ Improvements docs on equal scores. Also, remove the ability of TopDocs.merge to set shard indices (Atri Sharma, Adrien Grand, Simon Willnauer) +* LUCENE-8939: Introduce Shared Count Based Early Termination Across Multiple Slices Review comment: Fixed, thanks 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] [lucene-solr] atris commented on a change in pull request #823: LUCENE-8939: Introduce Shared Count Early Termination In Parallel Search
atris commented on a change in pull request #823: LUCENE-8939: Introduce Shared
Count Early Termination In Parallel Search
URL: https://github.com/apache/lucene-solr/pull/823#discussion_r319729707
##
File path:
lucene/core/src/java/org/apache/lucene/search/HitsThresholdChecker.java
##
@@ -0,0 +1,119 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.lucene.search;
+
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.function.BooleanSupplier;
+
+/**
+ * Used for defining custom algorithms to allow searches to early terminate
+ */
+abstract class HitsThresholdChecker implements BooleanSupplier {
+ /**
+ * Implementation of HitsThresholdChecker which allows global hit counting
+ */
+ private static class GlobalHitsThresholdChecker extends HitsThresholdChecker
{
+private final int totalHitsThreshold;
+private final AtomicInteger globalHitCount;
Review comment:
Interestingly, I did not see a performance improvement with `LongAdder`, but
I do agree that it is algorithmically faster than `AtomicInteger`. Update the
same, thanks for highlighting it.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] [lucene-solr] atris commented on a change in pull request #823: LUCENE-8939: Introduce Shared Count Early Termination In Parallel Search
atris commented on a change in pull request #823: LUCENE-8939: Introduce Shared
Count Early Termination In Parallel Search
URL: https://github.com/apache/lucene-solr/pull/823#discussion_r319070775
##
File path:
lucene/core/src/java/org/apache/lucene/search/HitCountThresholdCheckerFactory.java
##
@@ -0,0 +1,116 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.lucene.search;
+
+import java.util.concurrent.atomic.AtomicInteger;
+
+/**
+ * Factory which generates instances of GlobalHitsThresholdChecker and
LocalHitsThresholdChecker
+ * with the threshold as the passed in limit
+ */
+public final class HitCountThresholdCheckerFactory {
Review comment:
> Since we return a `HitsThresholdChecker` we could also make it public and
add the private impl and the static functions creation there. We don't really
need this factory class ?
@jimczi I was hesitant to do that since `HitsThresholdChecker` should
really be agnostic of the underlying implementation. However, given the fact
that the hit count checker(s) is the default, it makes sense to move it to the
parent class. Updated, please take a look.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] [lucene-solr] atris commented on a change in pull request #823: LUCENE-8939: Introduce Shared Count Early Termination In Parallel Search
atris commented on a change in pull request #823: LUCENE-8939: Introduce Shared
Count Early Termination In Parallel Search
URL: https://github.com/apache/lucene-solr/pull/823#discussion_r319065020
##
File path:
lucene/core/src/java/org/apache/lucene/search/HitCountThresholdCheckerFactory.java
##
@@ -0,0 +1,116 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.lucene.search;
+
+import java.util.concurrent.atomic.AtomicInteger;
+
+/**
+ * Factory which generates instances of GlobalHitsThresholdChecker and
LocalHitsThresholdChecker
+ * with the threshold as the passed in limit
+ */
+public final class HitCountThresholdCheckerFactory {
Review comment:
`HitsThresholdChecker` is currently an interface -- I am inclined to let it
remain the same to allow users to do fancy early termination algorithms, hence
introduced the factory. Thoughts?
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
