jimczi commented on code in PR #16769:
URL: https://github.com/apache/lucene/pull/16769#discussion_r4205760219
##########
lucene/core/src/java/org/apache/lucene/codecs/lucene99/Lucene99FlatVectorsReader.java:
##########
@@ -218,42 +224,55 @@ public FlatVectorsReader getMergeInstance() throws
IOException {
return mergeInstance;
} finally {
if (success == false) {
- original.releaseMergeVectorData();
+ original.release();
}
}
}
+ /**
+ * Whether a merge has to map the file again: only when searches read it at
random. Otherwise the
+ * mapping searches use carries no advice a merge reading front to back
would suffer from, and a
+ * reader a merge opened already reads the file the way a merge does.
+ */
+ private boolean mergeNeedsItsOwnMapping() {
+ return dataContext.context() != IOContext.Context.MERGE
+ && dataContext.hints().contains(DataAccessHint.RANDOM);
Review Comment:
Agreed, I updated the #16749 entry to say a merge maps the vectors again
only when searches read them at random, and the #16769 entry now only covers
what this PR adds.
##########
lucene/sandbox/src/java/org/apache/lucene/sandbox/codecs/dedup/MergeInput.java:
##########
@@ -0,0 +1,128 @@
+/*
+ * 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.sandbox.codecs.dedup;
+
+import java.io.Closeable;
+import java.io.FileNotFoundException;
+import java.io.IOException;
+import java.nio.file.NoSuchFileException;
+import java.util.stream.Stream;
+import org.apache.lucene.store.DataAccessHint;
+import org.apache.lucene.store.Directory;
+import org.apache.lucene.store.IOContext;
+import org.apache.lucene.store.IndexInput;
+import org.apache.lucene.store.NoReuseHint;
+
+/**
+ * The raw vectors as merges read them: at random, since a duplicate reads its
group's vector back,
+ * and not reused. Read advice applies to a whole mapping, so when searches
read the file
+ * differently a merge maps it again instead of reading through the mapping
searches use.
+ *
+ * <p>The mapping is opened when a merge first asks for it, shared by the
merges that follow, and
+ * closed when the last one is done, or with the reader.
+ */
+final class MergeInput implements Closeable {
+ private final Directory directory;
+ private final String fileName;
+ private final IOContext context;
+ private final IndexInput searchInput;
+ private IndexInput mergeInput;
+ // merges holding the mapping and not yet finished
+ private int merges;
+
+ /**
+ * @param context the context {@code searchInput} was opened with
+ */
+ MergeInput(Directory directory, String fileName, IOContext context,
IndexInput searchInput) {
+ this.directory = directory;
+ this.fileName = fileName;
+ this.context = context;
+ this.searchInput = searchInput;
+ }
+
+ /**
+ * Whether a merge needs a mapping of its own: only when searches read the
file differently. A
+ * file a merge opened is already read the way a merge reads it.
+ */
+ boolean needed() {
+ return context.context() != IOContext.Context.MERGE
+ && context.hints().equals(mergeContext().hints()) == false;
+ }
+
+ /** The mapping for one more merge, to give back with {@link #release()}. */
+ synchronized IndexInput acquire() throws IOException {
+ assert needed();
+ if (mergeInput == null) {
+ try {
+ mergeInput = directory.openInput(fileName, mergeContext());
+ } catch (FileNotFoundException | NoSuchFileException _) {
+ // an open reader outlives its files, so fall back to the mapping it
already holds
+ mergeInput = searchInput;
+ }
+ }
+ merges++;
+ return mergeInput;
+ }
+
+ /** Gives back the mapping of a merge that is done, closing it after the
last one. */
+ synchronized void release() throws IOException {
+ assert merges > 0;
+ if (--merges > 0) {
+ return;
+ }
+ if (mergeInput != null && mergeInput != searchInput) {
+ mergeInput.close();
Review Comment:
Good catch. `MergeInput` is gone with the de-duplicating formats, but
`Lucene99FlatVectorsReader` had the same pattern: the last merge instance now
takes the mapping under the lock and closes it outside.
--
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]
