jimczi commented on code in PR #16883:
URL: https://github.com/apache/lucene/pull/16883#discussion_r4359276739
##########
lucene/core/src/java/org/apache/lucene/codecs/lucene99/Lucene99FlatVectorsReader.java:
##########
@@ -212,59 +211,40 @@ public void checkIntegrity(MergePolicy.OneMerge merge)
throws IOException {
@Override
public FlatVectorsReader getMergeInstance() throws IOException {
- if (mergeNeedsItsOwnMapping() == false) {
- return this;
- }
- IndexInput data = original.mergeVectorData();
- boolean success = false;
- try {
- FlatVectorsReader mergeInstance = new Lucene99FlatVectorsReader(this,
data.clone());
- success = true;
- return mergeInstance;
- } finally {
- if (success == false) {
- IOUtils.closeWhileHandlingException(original.release());
- }
- }
+ IndexInput data = original.sequentialVectorData();
+ return data == vectorData ? this : new Lucene99FlatVectorsReader(this,
data.clone());
Review Comment:
Yeah, the clone now happens under the lock. sequentialVectorData() returns
the clone itself, same in the stored fields reader, so close can't get in
between.
##########
lucene/core/src/java/org/apache/lucene/codecs/lucene99/Lucene99FlatVectorsReader.java:
##########
@@ -389,57 +382,19 @@ public RandomVectorScorer getRandomVectorScorer(String
field, short[] target) th
target);
}
- /**
- * Closes the mapping a merge used, once no merge instance holds it. A later
merge maps the file
- * again. Only a merge instance holds the mapping, and it gives it back
once: finishing the reader
- * it came from, or finishing it again, releases nothing.
- */
- @Override
- public void finishMerge() throws IOException {
- if (original != this) {
- IOUtils.close(original.releaseMergeVectorData(this));
- }
- }
-
- /** Gives back the hold of {@code mergeInstance}, once; returns the mapping
to close, if any. */
- private synchronized IndexInput
releaseMergeVectorData(Lucene99FlatVectorsReader mergeInstance) {
- assert original == this && mergeInstance.original == this;
- if (mergeInstance.finished) {
- return null;
- }
- mergeInstance.finished = true;
- return release();
- }
-
- /**
- * Gives back one hold on the mapping. Once none is left, returns it for the
caller to close
- * outside the lock.
- */
- private synchronized IndexInput release() {
- assert original == this && mergeInstances > 0;
- if (--mergeInstances > 0) {
- return null;
- }
- IndexInput toClose = mergeVectorData == vectorData ? null :
mergeVectorData;
- mergeVectorData = null;
- return toClose;
- }
-
@Override
public void close() throws IOException {
- IOUtils.close(vectorData, takeMergeVectorData());
+ IOUtils.close(vectorData, takeSequentialVectorData());
Review Comment:
Done, I moved it into close like you suggested, in both readers. It returns
early if already closed, so closing twice is fine.
##########
lucene/core/src/java/org/apache/lucene/store/MMapDirectory.java:
##########
Review Comment:
Good catch, thanks. The function is captured at open now, and
testIsEfficientFor checks that a later setReadAdvice doesn't change the answer.
##########
lucene/core/src/java/org/apache/lucene/store/MMapDirectory.java:
##########
@@ -327,21 +327,20 @@ public IndexInput openInput(String name, IOContext
context) throws IOException {
final Arena arena = confined ? Arena.ofConfined() : getSharedArena(name,
arenas);
try (var fc = FileChannel.open(path, StandardOpenOption.READ)) {
final long fileSize = fc.size();
+ final ReadAdvice advice = toReadAdvice.apply(context);
+ final boolean preloaded = preload.test(name, context);
+ // preloaded or within a page, any advice reads the same
+ final boolean withinAPage =
+ NATIVE_ACCESS.filter(na -> fileSize < na.getPageSize()).isPresent();
Review Comment:
We could, by rounding the advised range inwards so only pages fully inside
the inner file get advised. The helper is shared with prefetch though, which
needs the outward rounding, and it changes how slices are advised today. Could
we do it in a follow up?
--
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]
