You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by "jimczi (via GitHub)" <gi...@apache.org> on 2026/10/21 13:09:58 UTC
[PR] Open files twice for sequential reads only when it helps [lucene]
jimczi opened a new pull request, #16883:
URL: https://github.com/apache/lucene/pull/16883
Readers open a file a second time for sequential reads, such as merges, only when the two opens get different read advice.
- Add `IndexInput#readsAs(IOContext)`, which returns whether an input reads as efficiently as one opened with the given context. `MemorySegmentIndexInput` returns false only when the advice for that context differs from the advice applied to its pages. Other inputs return true by default.
- `Lucene99FlatVectorsReader` and `Lucene90CompressingStoredFieldsReader` open their data file again for sequential reads only when the current input does not read as the sequential context. The new input is opened lazily and kept until the reader is closed.
- `Lucene90CompoundReader` gives an inner file another mapping only when it does not read as its open on the existing mappings.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Open files twice for sequential reads only when it helps [lucene]
Posted by "thecoop (via GitHub)" <gi...@apache.org>.
thecoop commented on code in PR #16883:
URL: https://github.com/apache/lucene/pull/16883#discussion_r4358701170
##########
lucene/core/src/java/org/apache/lucene/codecs/lucene90/Lucene90CompoundReader.java:
##########
@@ -176,28 +182,50 @@ public IndexInput openInput(String name, IOContext context) throws IOException {
+ entries.keySet()
+ ")");
}
- IndexInput data = context.context() == IOContext.Context.MERGE ? mergeHandle() : handle;
- return data.slice(name, entry.offset, entry.length, context);
+ synchronized (this) {
+ Mapping mapping = mappingFor(id, context);
+ IndexInput slice = mapping.data.slice(name, entry.offset, entry.length, context);
+ mapping.firstOpens.putIfAbsent(id, slice);
Review Comment:
Right, after many minutes staring at this, I think I've got it.
Each mapping stores the 'parent' handle and any open slices _each with a separate IOContext_. The issue is that the `IOContext` each mapping is for is _implicit_, due to the opaqueness of `isEfficientFor`. It's additionally confusing because `Mapping` is a `record`, which implies that the `IndexInput` is somehow part of its identity, but it's actually got nothing to do with the `IOContext` that that specific mapping is for, it's just stored for re-use for creating more slices, because the same pages of the same `IndexInput` can't be re-mapped with a different context.
On top of this, each `Mapping` can have slices for _different_ inner files open with _different_ contexts, because it just depends on in which order the contexts are opened for a specific inner file.
To say this is confusing is an understatement.
Firstly, it shouldn't be a record. It doesn't depend on record semantics, and it implies that the IndexInput matters for the 'identity' of the Mapping.
Secondly, this needs to be keyed, somehow, on what is actually being separated here - the inner files. I suggest the following:
There's a `Map<String, List<Slice>>` mapping each inner file to the collection of *slices* for that file. `Slice` is a tuple of the slice IndexInput and the 'parent' `IndexInput` it was created from. When opening, the slices are searched for one that `isEfficientFor` the context.
If one is not found, then a separate *pool* of top-level `IndexInput`s are searched to find an `IndexInput` that has not already been used to open a slice for that inner file (existing ones can be recorded during first iteration of the `List<Slice>`), opening a new one if necessary, and that is used to create a `Slice` for that file, and recorded in that file's list.
This way, the key is the thing we're actually trying to open, and the relationship between the source IndexInputs is not embedded into the search structure, but is kept separate from the slices.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Open files twice for sequential reads only when it helps [lucene]
Posted by "thecoop (via GitHub)" <gi...@apache.org>.
thecoop commented on code in PR #16883:
URL: https://github.com/apache/lucene/pull/16883#discussion_r4358701170
##########
lucene/core/src/java/org/apache/lucene/codecs/lucene90/Lucene90CompoundReader.java:
##########
@@ -176,28 +182,50 @@ public IndexInput openInput(String name, IOContext context) throws IOException {
+ entries.keySet()
+ ")");
}
- IndexInput data = context.context() == IOContext.Context.MERGE ? mergeHandle() : handle;
- return data.slice(name, entry.offset, entry.length, context);
+ synchronized (this) {
+ Mapping mapping = mappingFor(id, context);
+ IndexInput slice = mapping.data.slice(name, entry.offset, entry.length, context);
+ mapping.firstOpens.putIfAbsent(id, slice);
Review Comment:
Right, after many minutes staring at this, I think I've got it.
Each mapping stores the 'parent' handle and any open slices _each with a separate IOContext_. The issue is that the `IOContext` each mapping is for is _implicit_, due to the opaqueness of `isEfficientFor`. It's additionally confusing because `Mapping` is a `record`, which implies that the `IndexInput` is somehow part of its identity, but it's actually got nothing to do with the `IOContext` that that specific mapping is for, it's just stored for re-use for creating more slices, because the same pages of the same `IndexInput` can't be re-mapped with a different context.
On top of this, each `Mapping` can have slices for _different_ inner files open with _different_ contexts, because it just depends on in which order the contexts are opened for a specific inner file.
This is very confusing.
Firstly, it shouldn't be a record. It doesn't depend on record semantics, and it implies that the IndexInput matters for the 'identity' of the Mapping.
Secondly, this needs to be keyed, somehow, on what is actually being separated here - the inner files. I suggest the following:
There's a `Map<String, List<Slice>>` mapping each inner file to the collection of *slices* for that file. `Slice` is a tuple of the slice IndexInput and the 'parent' `IndexInput` it was created from. When opening, the slices are searched for one that `isEfficientFor` the context.
If one is not found, then a separate *pool* of top-level `IndexInput`s are searched to find an `IndexInput` that has not already been used to open a slice for that inner file (existing ones can be recorded during first iteration of the `List<Slice>`), opening a new one if necessary, and that is used to create a `Slice` for that file, and recorded in that file's list.
This way, the key is the thing we're actually trying to open, and the relationship between the source IndexInputs is not embedded into the search structure, but is kept separate from the slices, because it doesn't actually matter _which_ `IndexInput` a slice is from, as long as it is _different_ to any of the existing open slices.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Open files twice for sequential reads only when it helps [lucene]
Posted by "jimczi (via GitHub)" <gi...@apache.org>.
jimczi commented on code in PR #16883:
URL: https://github.com/apache/lucene/pull/16883#discussion_r4358942974
##########
lucene/core/src/java/org/apache/lucene/codecs/lucene90/Lucene90CompoundReader.java:
##########
@@ -176,28 +182,50 @@ public IndexInput openInput(String name, IOContext context) throws IOException {
+ entries.keySet()
+ ")");
}
- IndexInput data = context.context() == IOContext.Context.MERGE ? mergeHandle() : handle;
- return data.slice(name, entry.offset, entry.length, context);
+ synchronized (this) {
+ Mapping mapping = mappingFor(id, context);
+ IndexInput slice = mapping.data.slice(name, entry.offset, entry.length, context);
+ mapping.firstOpens.putIfAbsent(id, slice);
Review Comment:
Yeah you got it right, and sorry, it was way more confusing than it needed to be. The structure came from trying to keep updateIOContext working, which could change the advice of an input after it was opened. So it was built around the mappings and which inner files had been advised on each one, and that's why the context of a mapping was implicit and you had to work out the order to follow it. updateIOContext should never have been something to handle in the first place, since this replaces it. Once it became a no-op, a slice keeps the advice it was sliced with, so the slices of an inner file are all we need to look at, and we missed that. I changed it that way: the slices are keyed by inner file, the i-th one sits on the i-th copy of the data file, and we open a new copy only when a file already has a slice on every copy. No more Mapping record.
##########
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:
A standalone file is its own mapping starting at offset 0, so it never shares pages with another file. For inner files of a compound file, yes, the first and last pages can be shared with the neighbours and get the advice of whichever was advised last. That was already the case before this PR (see the comment in slice with an IOContext). Only aligning inner files to pages would fix 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.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Open files twice for sequential reads only when it helps [lucene]
Posted by "jimczi (via GitHub)" <gi...@apache.org>.
jimczi commented on PR #16883:
URL: https://github.com/apache/lucene/pull/16883#issuecomment-6361825843
@ChrisHegarty as we discussed offline, updateIOContext is the wrong way to do this. It changes the advice of pages shared by every clone and slice, so I made it a no-op on mmap. Nothing calls it anyway. That simplifies the branch a lot: an input keeps the advice it was opened or sliced with, and the field is final, so there's no shared state and no race. I'll deprecate 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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Open files twice for sequential reads only when it helps [lucene]
Posted by "thecoop (via GitHub)" <gi...@apache.org>.
thecoop commented on code in PR #16883:
URL: https://github.com/apache/lucene/pull/16883#discussion_r4359456113
##########
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:
Yes, let's look at it separately
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Open files twice for sequential reads only when it helps [lucene]
Posted by "jimczi (via GitHub)" <gi...@apache.org>.
jimczi commented on code in PR #16883:
URL: https://github.com/apache/lucene/pull/16883#discussion_r4358569408
##########
lucene/core/src/java/org/apache/lucene/codecs/lucene90/Lucene90CompoundReader.java:
##########
@@ -176,28 +182,50 @@ public IndexInput openInput(String name, IOContext context) throws IOException {
+ entries.keySet()
+ ")");
}
- IndexInput data = context.context() == IOContext.Context.MERGE ? mergeHandle() : handle;
- return data.slice(name, entry.offset, entry.length, context);
+ synchronized (this) {
+ Mapping mapping = mappingFor(id, context);
+ IndexInput slice = mapping.data.slice(name, entry.offset, entry.length, context);
+ mapping.firstOpens.putIfAbsent(id, slice);
+ return slice;
+ }
}
/**
- * A merge reads through a mapping of its own, so the access it advises does not replace the
- * advice of searches reading the same inner files. Merges of this segment share it. Like the
- * search mapping, it is opened without advice: each inner file is advised on its own region.
+ * Returns the first mapping where {@code id} is not open yet or reads as {@code context}, and
+ * opens a new mapping otherwise. This way an inner file is never advised twice on one mapping.
*/
- private synchronized IndexInput mergeHandle() throws IOException {
+ private Mapping mappingFor(String id, IOContext context) throws IOException {
+ assert Thread.holdsLock(this);
if (closed) {
throw new AlreadyClosedException("this Directory is closed");
}
- if (mergeHandle == null) {
- try {
- mergeHandle = directory.openInput(dataFileName, IOContext.DEFAULT);
- } catch (@SuppressWarnings("unused") FileNotFoundException | NoSuchFileException e) {
- // an open reader outlives its files, so fall back to the mapping it already holds
- mergeHandle = handle;
+ if (mappings.isEmpty()) {
+ mappings.add(new Mapping(handle));
+ }
+ for (Mapping mapping : mappings) {
+ IndexInput first = mapping.firstOpens.get(id);
+ if (first == null || first.readsAs(context)) {
+ return mapping;
}
}
- return mergeHandle;
+ IndexInput data;
+ try {
+ // like the reader's mapping, opened without advice
+ data = directory.openInput(dataFileName, IOContext.DEFAULT);
Review Comment:
Then the reader that needs it is stuck anyway? The lock is per compound reader and only openInput and close take it. Searches never call openInput, they read through inputs that are already open. So the only things that would wait are another open on the same segment's compound file, or close, and close should wait anyway? SegmentDocValues does the same thing: it opens a whole doc values producer under its lock.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Open files twice for sequential reads only when it helps [lucene]
Posted by "jimczi (via GitHub)" <gi...@apache.org>.
jimczi commented on code in PR #16883:
URL: https://github.com/apache/lucene/pull/16883#discussion_r4360247068
##########
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:
Actually I take that back, sorry. Closing the input closes its arena, so every clone throws once it's closed anyway. Cloning under the lock only moves the AlreadyClosedException from clone() to the first read, so I put it back the way it was. Using a reader while it's being closed gives AlreadyClosedException either way. The earlier race was different, it leaked an input, and that fix stays.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Open files twice for sequential reads only when it helps [lucene]
Posted by "thecoop (via GitHub)" <gi...@apache.org>.
thecoop commented on code in PR #16883:
URL: https://github.com/apache/lucene/pull/16883#discussion_r4358732495
##########
lucene/core/src/java/org/apache/lucene/codecs/lucene90/Lucene90CompoundReader.java:
##########
@@ -176,28 +182,50 @@ public IndexInput openInput(String name, IOContext context) throws IOException {
+ entries.keySet()
+ ")");
}
- IndexInput data = context.context() == IOContext.Context.MERGE ? mergeHandle() : handle;
- return data.slice(name, entry.offset, entry.length, context);
+ synchronized (this) {
+ Mapping mapping = mappingFor(id, context);
+ IndexInput slice = mapping.data.slice(name, entry.offset, entry.length, context);
+ mapping.firstOpens.putIfAbsent(id, slice);
+ return slice;
+ }
}
/**
- * A merge reads through a mapping of its own, so the access it advises does not replace the
- * advice of searches reading the same inner files. Merges of this segment share it. Like the
- * search mapping, it is opened without advice: each inner file is advised on its own region.
+ * Returns the first mapping where {@code id} is not open yet or reads as {@code context}, and
+ * opens a new mapping otherwise. This way an inner file is never advised twice on one mapping.
*/
- private synchronized IndexInput mergeHandle() throws IOException {
+ private Mapping mappingFor(String id, IOContext context) throws IOException {
+ assert Thread.holdsLock(this);
if (closed) {
throw new AlreadyClosedException("this Directory is closed");
}
- if (mergeHandle == null) {
- try {
- mergeHandle = directory.openInput(dataFileName, IOContext.DEFAULT);
- } catch (@SuppressWarnings("unused") FileNotFoundException | NoSuchFileException e) {
- // an open reader outlives its files, so fall back to the mapping it already holds
- mergeHandle = handle;
+ if (mappings.isEmpty()) {
+ mappings.add(new Mapping(handle));
+ }
+ for (Mapping mapping : mappings) {
+ IndexInput first = mapping.firstOpens.get(id);
+ if (first == null || first.readsAs(context)) {
+ return mapping;
}
}
- return mergeHandle;
+ IndexInput data;
+ try {
+ // like the reader's mapping, opened without advice
+ data = directory.openInput(dataFileName, IOContext.DEFAULT);
Review Comment:
OK, not an issue then
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Open files twice for sequential reads only when it helps [lucene]
Posted by "thecoop (via GitHub)" <gi...@apache.org>.
thecoop commented on code in PR #16883:
URL: https://github.com/apache/lucene/pull/16883#discussion_r4358797898
##########
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:
Don't we also need to take account of alignment? A file greater than a page but is not aligned can still then share pages with the files either side, which might then have their read advice changed from under them.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Open files twice for sequential reads only when it helps [lucene]
Posted by "ChrisHegarty (via GitHub)" <gi...@apache.org>.
ChrisHegarty commented on code in PR #16883:
URL: https://github.com/apache/lucene/pull/16883#discussion_r4349109624
##########
lucene/core/src/java/org/apache/lucene/store/IndexInput.java:
##########
@@ -238,6 +238,16 @@ public boolean prefetch(long offset, long length) throws IOException {
*/
public void updateIOContext(IOContext context) throws IOException {}
+ /**
+ * Returns true if reading this input is as efficient as reading an input opened with the given
+ * {@code IOContext}. If not, callers that read with this context may open the file again.
+ *
+ * <p>The default implementation returns {@code true}.
+ */
+ public boolean readsAs(IOContext context) {
Review Comment:
Subjective: `readsAs` is a hard name to parse. What about something like `isEfficientFor(IOContext)` ?
Maybe document that it is a hint, relative to how the input was last advised. So maybe something along the lines of "This is a hint based on the advice applied when the input was opened, sliced, or updated", or some such.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Open files twice for sequential reads only when it helps [lucene]
Posted by "thecoop (via GitHub)" <gi...@apache.org>.
thecoop commented on code in PR #16883:
URL: https://github.com/apache/lucene/pull/16883#discussion_r4361249363
##########
lucene/core/src/java/org/apache/lucene/codecs/lucene90/Lucene90CompoundReader.java:
##########
@@ -176,28 +185,33 @@ public IndexInput openInput(String name, IOContext context) throws IOException {
+ entries.keySet()
+ ")");
}
- IndexInput data = context.context() == IOContext.Context.MERGE ? mergeHandle() : handle;
- return data.slice(name, entry.offset, entry.length, context);
- }
-
- /**
- * A merge reads through a mapping of its own, so the access it advises does not replace the
- * advice of searches reading the same inner files. Merges of this segment share it. Like the
- * search mapping, it is opened without advice: each inner file is advised on its own region.
- */
- private synchronized IndexInput mergeHandle() throws IOException {
- if (closed) {
- throw new AlreadyClosedException("this Directory is closed");
- }
- if (mergeHandle == null) {
- try {
- mergeHandle = directory.openInput(dataFileName, IOContext.DEFAULT);
- } catch (@SuppressWarnings("unused") FileNotFoundException | NoSuchFileException e) {
- // an open reader outlives its files, so fall back to the mapping it already holds
- mergeHandle = handle;
+ synchronized (this) {
+ if (closed) {
+ throw new AlreadyClosedException("this Directory is closed");
+ }
+ List<Slice> fileSlices = slices.computeIfAbsent(id, _ -> new ArrayList<>());
+ int copy = 0;
+ while (copy < fileSlices.size()
+ && fileSlices.get(copy).input().isEfficientFor(context) == false) {
+ copy++;
+ }
+ // each slice of an inner file is on its own copy of the data file, so it keeps its own advice
+ if (copy == fileSlices.size()) {
+ try {
+ if (copy == data.size()) {
+ // without advice, like handle, so inner files advise their own pages
+ data.add(directory.openInput(dataFileName, IOContext.DEFAULT));
+ }
+ IndexInput input = data.get(copy).slice(name, entry.offset, entry.length, context);
Review Comment:
This slice isn't actually used anywhere?
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Open files twice for sequential reads only when it helps [lucene]
Posted by "thecoop (via GitHub)" <gi...@apache.org>.
thecoop commented on code in PR #16883:
URL: https://github.com/apache/lucene/pull/16883#discussion_r4358484307
##########
lucene/core/src/java/org/apache/lucene/codecs/lucene90/Lucene90CompoundReader.java:
##########
@@ -176,28 +182,50 @@ public IndexInput openInput(String name, IOContext context) throws IOException {
+ entries.keySet()
+ ")");
}
- IndexInput data = context.context() == IOContext.Context.MERGE ? mergeHandle() : handle;
- return data.slice(name, entry.offset, entry.length, context);
+ synchronized (this) {
+ Mapping mapping = mappingFor(id, context);
+ IndexInput slice = mapping.data.slice(name, entry.offset, entry.length, context);
+ mapping.firstOpens.putIfAbsent(id, slice);
+ return slice;
+ }
}
/**
- * A merge reads through a mapping of its own, so the access it advises does not replace the
- * advice of searches reading the same inner files. Merges of this segment share it. Like the
- * search mapping, it is opened without advice: each inner file is advised on its own region.
+ * Returns the first mapping where {@code id} is not open yet or reads as {@code context}, and
+ * opens a new mapping otherwise. This way an inner file is never advised twice on one mapping.
*/
- private synchronized IndexInput mergeHandle() throws IOException {
+ private Mapping mappingFor(String id, IOContext context) throws IOException {
+ assert Thread.holdsLock(this);
if (closed) {
throw new AlreadyClosedException("this Directory is closed");
}
- if (mergeHandle == null) {
- try {
- mergeHandle = directory.openInput(dataFileName, IOContext.DEFAULT);
- } catch (@SuppressWarnings("unused") FileNotFoundException | NoSuchFileException e) {
- // an open reader outlives its files, so fall back to the mapping it already holds
- mergeHandle = handle;
+ if (mappings.isEmpty()) {
+ mappings.add(new Mapping(handle));
+ }
+ for (Mapping mapping : mappings) {
+ IndexInput first = mapping.firstOpens.get(id);
+ if (first == null || first.readsAs(context)) {
+ return mapping;
}
}
- return mergeHandle;
+ IndexInput data;
+ try {
+ // like the reader's mapping, opened without advice
+ data = directory.openInput(dataFileName, IOContext.DEFAULT);
Review Comment:
What if the open blocks for some reason?
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Open files twice for sequential reads only when it helps [lucene]
Posted by "thecoop (via GitHub)" <gi...@apache.org>.
thecoop commented on code in PR #16883:
URL: https://github.com/apache/lucene/pull/16883#discussion_r4358701170
##########
lucene/core/src/java/org/apache/lucene/codecs/lucene90/Lucene90CompoundReader.java:
##########
@@ -176,28 +182,50 @@ public IndexInput openInput(String name, IOContext context) throws IOException {
+ entries.keySet()
+ ")");
}
- IndexInput data = context.context() == IOContext.Context.MERGE ? mergeHandle() : handle;
- return data.slice(name, entry.offset, entry.length, context);
+ synchronized (this) {
+ Mapping mapping = mappingFor(id, context);
+ IndexInput slice = mapping.data.slice(name, entry.offset, entry.length, context);
+ mapping.firstOpens.putIfAbsent(id, slice);
Review Comment:
Right, after many minutes staring at this, I think I've got it.
Each mapping stores the 'parent' handle and any open slices _for a particular IOContext for each slice_. The issue is that the IOContext each mapping is for is _implicit_, due to the opaqueness of `isEfficientFor`. It's additionally confusing because `Mapping` is a `record`, which implies that the `IndexInput` is somehow part of its identity, but it's actually got nothing to do with the `IOContext` that that specific mapping is for, it's just stored for re-use for creating more slices, because the same pages of the same `IndexInput` can't be re-mapped with a different context.
Additionally, each `Mapping` can have slices for _different_ inner files open with `different` contexts, because it just depends on in which order the contexts are opened for a specific inner file.
To say this is confusing is an understatement.
Firstly, it shouldn't be a record. It doesn't depend on record semantics, and it implies that the IndexInput matters for the 'identity' of the Mapping.
Secondly, this needs to be keyed, somehow, on what is actually being separated here - the inner files. I suggest the following:
There's a `Map<String, List<Slice>>` mapping each inner file to the collection of *slices* for that file. `Slice` is a tuple of the slice IndexInput and the 'parent' `IndexInput` it was created from. When opening, the slices are searched for one that `isEfficientFor` the context.
If one is not found, then a separate *pool* of top-level `IndexInput`s are searched to find an `IndexInput` that has not already been used to open a slice for that inner file (existing ones can be recorded during first iteration of the `List<Slice>`), opening a new one if necessary, and that is used to create a `Slice` for that file, and recorded in that file's list.
This way, the key is the thing we're actually trying to open, and the relationship between the source IndexInputs is not embedded into the search structure, but is kept separate from the slices.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Open files twice for sequential reads only when it helps [lucene]
Posted by "thecoop (via GitHub)" <gi...@apache.org>.
thecoop commented on code in PR #16883:
URL: https://github.com/apache/lucene/pull/16883#discussion_r4359110002
##########
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:
Or not changing the read advice of pages that straddle multiple files?
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Open files twice for sequential reads only when it helps [lucene]
Posted by "thecoop (via GitHub)" <gi...@apache.org>.
thecoop commented on code in PR #16883:
URL: https://github.com/apache/lucene/pull/16883#discussion_r4350051639
##########
lucene/core/src/java/org/apache/lucene/store/IndexInput.java:
##########
@@ -238,6 +238,16 @@ public boolean prefetch(long offset, long length) throws IOException {
*/
public void updateIOContext(IOContext context) throws IOException {}
+ /**
+ * Returns true if reading this input is as efficient as reading an input opened with the given
+ * {@code IOContext}. If not, callers that read with this context may open the file again.
Review Comment:
The 'may' is unclear. Are they _allowed_ to do it? Or can they _choose_ to do 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.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Open files twice for sequential reads only when it helps [lucene]
Posted by "jimczi (via GitHub)" <gi...@apache.org>.
jimczi commented on code in PR #16883:
URL: https://github.com/apache/lucene/pull/16883#discussion_r4350706725
##########
lucene/core/src/java/org/apache/lucene/codecs/lucene90/compressing/Lucene90CompressingStoredFieldsReader.java:
##########
@@ -274,14 +271,16 @@ private void ensureOpen() throws AlreadyClosedException {
@Override
public void close() throws IOException {
if (!closed) {
- IOUtils.close(indexReader, fieldsStream, mergeFieldsStreamToClose());
+ IOUtils.close(indexReader, fieldsStream, sequentialFieldsStreamToClose());
closed = true;
}
}
- /** The mapping a merge opened, read under the lock that guards it, closed outside it. */
- private synchronized IndexInput mergeFieldsStreamToClose() {
- return original == this && mergeFieldsStream != fieldsStream ? mergeFieldsStream : null;
+ /** Returns the sequential input to close, if it is not {@code fieldsStream}. */
+ private synchronized IndexInput sequentialFieldsStreamToClose() {
+ return original == this && sequentialFieldsStream != fieldsStream
+ ? sequentialFieldsStream
+ : null;
Review Comment:
Good catch, fixed. closed is now set under the same monitor that guards the sequential input, like in Lucene99FlatVectorsReader.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Open files twice for sequential reads only when it helps [lucene]
Posted by "thecoop (via GitHub)" <gi...@apache.org>.
thecoop commented on code in PR #16883:
URL: https://github.com/apache/lucene/pull/16883#discussion_r4350260798
##########
lucene/core/src/java/org/apache/lucene/codecs/lucene90/Lucene90CompoundReader.java:
##########
@@ -176,28 +182,50 @@ public IndexInput openInput(String name, IOContext context) throws IOException {
+ entries.keySet()
+ ")");
}
- IndexInput data = context.context() == IOContext.Context.MERGE ? mergeHandle() : handle;
- return data.slice(name, entry.offset, entry.length, context);
+ synchronized (this) {
+ Mapping mapping = mappingFor(id, context);
+ IndexInput slice = mapping.data.slice(name, entry.offset, entry.length, context);
+ mapping.firstOpens.putIfAbsent(id, slice);
Review Comment:
It's a bit odd having the id each mapping is for inside the first map, or I'm not properly understanding the data structure - `mappingFor` is very unclear. Can `mappings` be a `Map<String, List<IndexInput>>` instead?
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Open files twice for sequential reads only when it helps [lucene]
Posted by "ChrisHegarty (via GitHub)" <gi...@apache.org>.
ChrisHegarty commented on code in PR #16883:
URL: https://github.com/apache/lucene/pull/16883#discussion_r4348865594
##########
lucene/core/src/test/org/apache/lucene/codecs/lucene99/TestVectorsMergeReadAdvice.java:
##########
@@ -199,10 +204,24 @@ public void testAMergeReadsThroughTheReaderItOpened() throws Exception {
}
}
- /** Searches that do not read the vectors at random leave nothing for a merge to undo. */
- public void testNoMappingWhenSearchesDoNotReadAtRandom() throws Exception {
- Opens opens = new Opens();
- try (Directory dir = new RecordingDirectory(newDirectory(), opens)) {
+ /** No new input when searches already read the vectors sequentially. */
+ public void testNoMappingWhenSearchesReadInOrder() throws Exception {
+ assertNoMapping(
+ dir -> new RecordingDirectory(dir, new Opens()),
+ IOContext.DEFAULT.withHints(DataAccessHint.SEQUENTIAL));
+ }
+
+ /** No new input when every context reads the same. */
+ public void testNoMappingWhenTheDirectoryReadsTheSame() throws Exception {
+ assertNoMapping(
+ dir -> new RecordingDirectory(dir, new Opens(), false),
+ IOContext.DEFAULT.withHints(DataAccessHint.RANDOM, NoReuseHint.INSTANCE));
+ }
+
+ private static void assertNoMapping(
+ java.util.function.Function<Directory, RecordingDirectory> wrap, IOContext searchContext)
Review Comment:
nit: let's import `java.util.function.Function`
##########
lucene/core/src/java/org/apache/lucene/codecs/lucene90/compressing/Lucene90CompressingStoredFieldsReader.java:
##########
@@ -274,14 +271,16 @@ private void ensureOpen() throws AlreadyClosedException {
@Override
public void close() throws IOException {
if (!closed) {
- IOUtils.close(indexReader, fieldsStream, mergeFieldsStreamToClose());
+ IOUtils.close(indexReader, fieldsStream, sequentialFieldsStreamToClose());
closed = true;
}
}
- /** The mapping a merge opened, read under the lock that guards it, closed outside it. */
- private synchronized IndexInput mergeFieldsStreamToClose() {
- return original == this && mergeFieldsStream != fieldsStream ? mergeFieldsStream : null;
+ /** Returns the sequential input to close, if it is not {@code fieldsStream}. */
+ private synchronized IndexInput sequentialFieldsStreamToClose() {
+ return original == this && sequentialFieldsStream != fieldsStream
+ ? sequentialFieldsStream
+ : null;
Review Comment:
`close()` takes `sequentialFieldsStreamToClose()` and only sets `closed = true` after `IOUtils.close(...)` returns. `sequentialFieldsStream()` checks `ensureOpen()` against that same volatile flag. A concurrent `getMergeInstance()` can therefore pass `ensureOpen()` after `close()` has already collected a null stream. It then opens a new input that is never closed.
You avoided this in `Lucene99FlatVectorsReader` with a closed flag set inside the lock (lines 224-226, 395). Suggest the same here, with closed checked and set under the monitor.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Open files twice for sequential reads only when it helps [lucene]
Posted by "jimczi (via GitHub)" <gi...@apache.org>.
jimczi commented on code in PR #16883:
URL: https://github.com/apache/lucene/pull/16883#discussion_r4350733544
##########
lucene/core/src/java/org/apache/lucene/codecs/lucene90/Lucene90CompoundReader.java:
##########
@@ -176,28 +182,50 @@ public IndexInput openInput(String name, IOContext context) throws IOException {
+ entries.keySet()
+ ")");
}
- IndexInput data = context.context() == IOContext.Context.MERGE ? mergeHandle() : handle;
- return data.slice(name, entry.offset, entry.length, context);
+ synchronized (this) {
+ Mapping mapping = mappingFor(id, context);
+ IndexInput slice = mapping.data.slice(name, entry.offset, entry.length, context);
+ mapping.firstOpens.putIfAbsent(id, slice);
+ return slice;
+ }
}
/**
- * A merge reads through a mapping of its own, so the access it advises does not replace the
- * advice of searches reading the same inner files. Merges of this segment share it. Like the
- * search mapping, it is opened without advice: each inner file is advised on its own region.
+ * Returns the first mapping where {@code id} is not open yet or reads as {@code context}, and
+ * opens a new mapping otherwise. This way an inner file is never advised twice on one mapping.
*/
- private synchronized IndexInput mergeHandle() throws IOException {
+ private Mapping mappingFor(String id, IOContext context) throws IOException {
+ assert Thread.holdsLock(this);
if (closed) {
throw new AlreadyClosedException("this Directory is closed");
}
- if (mergeHandle == null) {
- try {
- mergeHandle = directory.openInput(dataFileName, IOContext.DEFAULT);
- } catch (@SuppressWarnings("unused") FileNotFoundException | NoSuchFileException e) {
- // an open reader outlives its files, so fall back to the mapping it already holds
- mergeHandle = handle;
+ if (mappings.isEmpty()) {
+ mappings.add(new Mapping(handle));
+ }
+ for (Mapping mapping : mappings) {
+ IndexInput first = mapping.firstOpens.get(id);
+ if (first == null || first.readsAs(context)) {
+ return mapping;
}
}
- return mergeHandle;
+ IndexInput data;
+ try {
+ // like the reader's mapping, opened without advice
+ data = directory.openInput(dataFileName, IOContext.DEFAULT);
Review Comment:
The open under the lock happens at most once per advice per reader, so twice per reader in practice, and the lock is what keeps it to a single open. Opening outside would need open-then-close-if-we-lost code.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Open files twice for sequential reads only when it helps [lucene]
Posted by "jimczi (via GitHub)" <gi...@apache.org>.
jimczi commented on code in PR #16883:
URL: https://github.com/apache/lucene/pull/16883#discussion_r4350723929
##########
lucene/core/src/java/org/apache/lucene/store/IndexInput.java:
##########
@@ -238,6 +238,16 @@ public boolean prefetch(long offset, long length) throws IOException {
*/
public void updateIOContext(IOContext context) throws IOException {}
+ /**
+ * Returns true if reading this input is as efficient as reading an input opened with the given
+ * {@code IOContext}. If not, callers that read with this context may open the file again.
+ *
+ * <p>The default implementation returns {@code true}.
+ */
+ public boolean readsAs(IOContext context) {
Review Comment:
I reworded it around the decision rather than the claim: false means callers should open the file again, and the default true means they never do. An Optional would leave every caller to map "unknown" to "don't open again", which is what true already says?
##########
lucene/core/src/java/org/apache/lucene/codecs/lucene90/Lucene90CompoundReader.java:
##########
@@ -148,13 +150,17 @@ private Map<String, FileEntry> readMapping(IndexInput entriesStream) throws IOEx
@Override
public void close() throws IOException {
- IndexInput merge;
+ List<IndexInput> others = new ArrayList<>();
synchronized (this) {
closed = true;
- merge = mergeHandle == handle ? null : mergeHandle;
- mergeHandle = null;
+ for (Mapping mapping : mappings) {
+ if (mapping.data != handle) {
+ others.add(mapping.data);
+ }
+ }
+ mappings.clear();
}
- IOUtils.close(handle, merge);
+ IOUtils.close(handle, () -> IOUtils.close(others));
Review Comment:
Done. The reader's mapping is in the list from the start and close just closes the list.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Open files twice for sequential reads only when it helps [lucene]
Posted by "jimczi (via GitHub)" <gi...@apache.org>.
jimczi commented on code in PR #16883:
URL: https://github.com/apache/lucene/pull/16883#discussion_r4350711527
##########
lucene/core/src/java/org/apache/lucene/store/IndexInput.java:
##########
@@ -238,6 +238,16 @@ public boolean prefetch(long offset, long length) throws IOException {
*/
public void updateIOContext(IOContext context) throws IOException {}
+ /**
+ * Returns true if reading this input is as efficient as reading an input opened with the given
+ * {@code IOContext}. If not, callers that read with this context may open the file again.
+ *
+ * <p>The default implementation returns {@code true}.
+ */
+ public boolean readsAs(IOContext context) {
Review Comment:
Renamed to isEfficientFor, and documented as a hint based on the advice applied when the input was opened, sliced or updated.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Open files twice for sequential reads only when it helps [lucene]
Posted by "thecoop (via GitHub)" <gi...@apache.org>.
thecoop commented on code in PR #16883:
URL: https://github.com/apache/lucene/pull/16883#discussion_r4350073304
##########
lucene/core/src/java/org/apache/lucene/store/IndexInput.java:
##########
@@ -238,6 +238,16 @@ public boolean prefetch(long offset, long length) throws IOException {
*/
public void updateIOContext(IOContext context) throws IOException {}
+ /**
+ * Returns true if reading this input is as efficient as reading an input opened with the given
+ * {@code IOContext}. If not, callers that read with this context may open the file again.
+ *
+ * <p>The default implementation returns {@code true}.
+ */
+ public boolean readsAs(IOContext context) {
Review Comment:
Whilst I understand why this returns `true` by default, it's a bit misleading. Basically, with no other information, it says 'yeah, it's ok, no worries!', which is incorrect.
I think it'll be better to return a 3rd option to some extent - maybe an `Optional<Boolean>` so it's not saying `true` when it actually doesn't know.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Open files twice for sequential reads only when it helps [lucene]
Posted by "thecoop (via GitHub)" <gi...@apache.org>.
thecoop commented on code in PR #16883:
URL: https://github.com/apache/lucene/pull/16883#discussion_r4350117696
##########
lucene/core/src/java/org/apache/lucene/codecs/lucene90/Lucene90CompoundReader.java:
##########
@@ -176,28 +182,50 @@ public IndexInput openInput(String name, IOContext context) throws IOException {
+ entries.keySet()
+ ")");
}
- IndexInput data = context.context() == IOContext.Context.MERGE ? mergeHandle() : handle;
- return data.slice(name, entry.offset, entry.length, context);
+ synchronized (this) {
+ Mapping mapping = mappingFor(id, context);
+ IndexInput slice = mapping.data.slice(name, entry.offset, entry.length, context);
+ mapping.firstOpens.putIfAbsent(id, slice);
+ return slice;
+ }
}
/**
- * A merge reads through a mapping of its own, so the access it advises does not replace the
- * advice of searches reading the same inner files. Merges of this segment share it. Like the
- * search mapping, it is opened without advice: each inner file is advised on its own region.
+ * Returns the first mapping where {@code id} is not open yet or reads as {@code context}, and
+ * opens a new mapping otherwise. This way an inner file is never advised twice on one mapping.
*/
- private synchronized IndexInput mergeHandle() throws IOException {
+ private Mapping mappingFor(String id, IOContext context) throws IOException {
+ assert Thread.holdsLock(this);
if (closed) {
throw new AlreadyClosedException("this Directory is closed");
}
- if (mergeHandle == null) {
- try {
- mergeHandle = directory.openInput(dataFileName, IOContext.DEFAULT);
- } catch (@SuppressWarnings("unused") FileNotFoundException | NoSuchFileException e) {
- // an open reader outlives its files, so fall back to the mapping it already holds
- mergeHandle = handle;
+ if (mappings.isEmpty()) {
+ mappings.add(new Mapping(handle));
+ }
+ for (Mapping mapping : mappings) {
+ IndexInput first = mapping.firstOpens.get(id);
+ if (first == null || first.readsAs(context)) {
+ return mapping;
}
}
- return mergeHandle;
+ IndexInput data;
+ try {
+ // like the reader's mapping, opened without advice
+ data = directory.openInput(dataFileName, IOContext.DEFAULT);
Review Comment:
I'm cautious about there being IO operations inside a mutex - this could cause threading issues
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Open files twice for sequential reads only when it helps [lucene]
Posted by "jimczi (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Open files twice for sequential reads only when it helps [lucene]
Posted by "ChrisHegarty (via GitHub)" <gi...@apache.org>.
ChrisHegarty commented on PR #16883:
URL: https://github.com/apache/lucene/pull/16883#issuecomment-6361381668
I have a design question on `readAdvice` in `MemorySegmentIndexInput` and `isEfficientFor`.
`updateIOContext` calls madvise on the shared pages, so it affects the parent and its clones and slices. But `readAdvice` is a per-instance field. Clones and slices copy it when they're created, and `updateIOContext` only updates the instance it's called on.
So, is the intent that the advice belongs to the parent, and clones and slices defer to it? Or is it per instance, with each copy reporting what it was created with? Right now it's the second, but the pages behave like the first.
This matters for `updateIOContext`. After updateIOContext on the parent, existing clones and slices keep reporting the old advice. Similarly, after `updateIOContext` on a clone or slice, the parent keeps reporting the old advice, even though the pages changed.
Related to that, `readAdvice` is a plain non-volatile field. `updateIOContext `can write it while another thread reads it in `isEfficientFor`.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Open files twice for sequential reads only when it helps [lucene]
Posted by "thecoop (via GitHub)" <gi...@apache.org>.
thecoop commented on code in PR #16883:
URL: https://github.com/apache/lucene/pull/16883#discussion_r4358701170
##########
lucene/core/src/java/org/apache/lucene/codecs/lucene90/Lucene90CompoundReader.java:
##########
@@ -176,28 +182,50 @@ public IndexInput openInput(String name, IOContext context) throws IOException {
+ entries.keySet()
+ ")");
}
- IndexInput data = context.context() == IOContext.Context.MERGE ? mergeHandle() : handle;
- return data.slice(name, entry.offset, entry.length, context);
+ synchronized (this) {
+ Mapping mapping = mappingFor(id, context);
+ IndexInput slice = mapping.data.slice(name, entry.offset, entry.length, context);
+ mapping.firstOpens.putIfAbsent(id, slice);
Review Comment:
Right, after many minutes staring at this, I think I've got it.
Each mapping stores the 'parent' handle and any open slices _each with a separate IOContext_. The issue is that the `IOContext` each mapping is for is _implicit_, due to the opaqueness of `isEfficientFor`. It's additionally confusing because `Mapping` is a `record`, which implies that the `IndexInput` is somehow part of its identity, but it's actually got nothing to do with the `IOContext` that that specific mapping is for, it's just stored for re-use for creating more slices, because the same pages of the same `IndexInput` can't be re-mapped with a different context.
On top of this, each `Mapping` can have slices for _different_ inner files open with _different_ contexts, because it just depends on in which order the contexts are opened for a specific inner file.
To say this is confusing is an understatement.
Firstly, it shouldn't be a record. It doesn't depend on record semantics, and it implies that the IndexInput matters for the 'identity' of the Mapping.
Secondly, this needs to be keyed, somehow, on what is actually being separated here - the inner files. I suggest the following:
There's a `Map<String, List<Slice>>` mapping each inner file to the collection of *slices* for that file. `Slice` is a tuple of the slice IndexInput and the 'parent' `IndexInput` it was created from. When opening, the slices are searched for one that `isEfficientFor` the context.
If one is not found, then a separate *pool* of top-level `IndexInput`s are searched to find an `IndexInput` that has not already been used to open a slice for that inner file (existing ones can be recorded during first iteration of the `List<Slice>`), opening a new one if necessary, and that is used to create a `Slice` for that file, and recorded in that file's list.
This way, the key is the thing we're actually trying to open, and the relationship between the source IndexInputs is not embedded into the search structure, but is kept separate from the slices, because it doesn't actually matter _which_ `IndexInput` a slice is from, as long as it is _different_ to any of the existing open slices.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Open files twice for sequential reads only when it helps [lucene]
Posted by "ChrisHegarty (via GitHub)" <gi...@apache.org>.
ChrisHegarty commented on code in PR #16883:
URL: https://github.com/apache/lucene/pull/16883#discussion_r4358950903
##########
lucene/core/src/java/org/apache/lucene/store/MMapDirectory.java:
##########
Review Comment:
Nit: isEfficientFor mixes two read advice functions if setReadAdvice is called after an input is opened.
`readAdvice` is captured when the input is created, but the `toReadAdvice` lambda reads `MMapDirectory.this.readAdvice` each time it runs. After `setReadAdvice(...`), `isEfficientFor` compares advice stored under the old function with advice computed under the new one. The field is also non-volatile, so another thread may not see the new function.
Capturing the function at open time keeps each input (and its clones and slices) consistent with the function it was opened under:
```suggest
// the function in effect now, so that this input is not affected by a later setReadAdvice
final var readAdviceFunction = readAdvice;
Function<IOContext, ReadAdvice> toReadAdvice =
c -> readAdviceFunction.apply(name, c).orElse(Constants.DEFAULT_READADVICE);
```
<details>
<summary>Test that fails without this and passes with it (in <code>TestMMapDirectory</code>)</summary>
```
/** An input keeps answering from the read advice function it was opened with. */
public void testIsEfficientForIgnoresLaterReadAdviceChanges() throws IOException {
assumeTrue("needs madvise to tell random from sequential", MMapDirectory.supportsMadvise());
var random = new DefaultIOContext(DataAccessHint.RANDOM);
var sequential = new DefaultIOContext(DataAccessHint.SEQUENTIAL);
final int size = 1 << 17;
try (MMapDirectory dir = new MMapDirectory(createTempDir("testIsEfficientForLater"))) {
try (IndexOutput out = dir.createOutput("test", IOContext.DEFAULT)) {
out.writeBytes(new byte[size], 0, size);
}
dir.setReadAdvice(MMapDirectory.ADVISE_BY_CONTEXT);
try (IndexInput in = dir.openInput("test", random)) {
assertFalse(
"random pages are not efficient for sequential reads", in.isEfficientFor(sequential));
// changing the function after the open must not change what this input answers
dir.setReadAdvice((_, _) -> Optional.of(ReadAdvice.NORMAL));
assertFalse("answer changed after setReadAdvice", in.isEfficientFor(sequential));
assertFalse("a clone answers like its input", in.clone().isEfficientFor(sequential));
assertFalse(
"a slice answers like its input", in.slice("slice", 0, size).isEfficientFor(sequential));
}
}
}
```
</details>
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Open files twice for sequential reads only when it helps [lucene]
Posted by "ChrisHegarty (via GitHub)" <gi...@apache.org>.
ChrisHegarty commented on code in PR #16883:
URL: https://github.com/apache/lucene/pull/16883#discussion_r4358372290
##########
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:
`getMergeInstance()` and `getFloatVectorValues(field, SEQUENTIAL)` get the cached sequential input from a synchronized method, then call `.clone()` or `.slice()` on it after the lock is released. A concurrent `close()` can close that input in between and make the clone or slice fail with AlreadyClosedException, so we probable need to clone while holding the lock?
##########
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:
similar in `Lucene90CompressingStoredFieldsReader`
##########
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:
I find the close a little hard to follow, and am worried about the multiple close scenarios, etc. Maybe we can simplify this if we move the implementation into close itself? e.g.
```
public void close() throws IOException {
final IndexInput sequential;
synchronized (this) {
if (closed) {
return;
}
closed = true;
sequential =
original == this && sequentialVectorData != vectorData ? sequentialVectorData : null;
sequentialVectorData = null;
}
IOUtils.close(vectorData, sequential);
}
```
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Open files twice for sequential reads only when it helps [lucene]
Posted by "jimczi (via GitHub)" <gi...@apache.org>.
jimczi commented on code in PR #16883:
URL: https://github.com/apache/lucene/pull/16883#discussion_r4350737732
##########
lucene/core/src/java/org/apache/lucene/codecs/lucene90/Lucene90CompoundReader.java:
##########
@@ -176,28 +182,50 @@ public IndexInput openInput(String name, IOContext context) throws IOException {
+ entries.keySet()
+ ")");
}
- IndexInput data = context.context() == IOContext.Context.MERGE ? mergeHandle() : handle;
- return data.slice(name, entry.offset, entry.length, context);
+ synchronized (this) {
+ Mapping mapping = mappingFor(id, context);
+ IndexInput slice = mapping.data.slice(name, entry.offset, entry.length, context);
+ mapping.firstOpens.putIfAbsent(id, slice);
+ return slice;
+ }
}
/**
- * A merge reads through a mapping of its own, so the access it advises does not replace the
- * advice of searches reading the same inner files. Merges of this segment share it. Like the
- * search mapping, it is opened without advice: each inner file is advised on its own region.
+ * Returns the first mapping where {@code id} is not open yet or reads as {@code context}, and
+ * opens a new mapping otherwise. This way an inner file is never advised twice on one mapping.
*/
- private synchronized IndexInput mergeHandle() throws IOException {
+ private Mapping mappingFor(String id, IOContext context) throws IOException {
+ assert Thread.holdsLock(this);
if (closed) {
throw new AlreadyClosedException("this Directory is closed");
}
- if (mergeHandle == null) {
- try {
- mergeHandle = directory.openInput(dataFileName, IOContext.DEFAULT);
- } catch (@SuppressWarnings("unused") FileNotFoundException | NoSuchFileException e) {
- // an open reader outlives its files, so fall back to the mapping it already holds
- mergeHandle = handle;
+ if (mappings.isEmpty()) {
+ mappings.add(new Mapping(handle));
+ }
+ for (Mapping mapping : mappings) {
+ IndexInput first = mapping.firstOpens.get(id);
+ if (first == null || first.readsAs(context)) {
+ return mapping;
}
}
- return mergeHandle;
+ IndexInput data;
+ try {
+ // like the reader's mapping, opened without advice
+ data = directory.openInput(dataFileName, IOContext.DEFAULT);
Review Comment:
The extra mapping is shared by all inner files, so it's opened without advice, and each inner file advises its own pages through its slice. With context, the whole compound file would get the advice, including inner files that are later read at random on that mapping.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Open files twice for sequential reads only when it helps [lucene]
Posted by "jimczi (via GitHub)" <gi...@apache.org>.
jimczi commented on code in PR #16883:
URL: https://github.com/apache/lucene/pull/16883#discussion_r4350743536
##########
lucene/core/src/java/org/apache/lucene/codecs/lucene90/Lucene90CompoundReader.java:
##########
@@ -176,28 +182,50 @@ public IndexInput openInput(String name, IOContext context) throws IOException {
+ entries.keySet()
+ ")");
}
- IndexInput data = context.context() == IOContext.Context.MERGE ? mergeHandle() : handle;
- return data.slice(name, entry.offset, entry.length, context);
+ synchronized (this) {
+ Mapping mapping = mappingFor(id, context);
+ IndexInput slice = mapping.data.slice(name, entry.offset, entry.length, context);
+ mapping.firstOpens.putIfAbsent(id, slice);
Review Comment:
A mapping serves every inner file opened on it, so it can't be keyed by file. I inlined mappingFor into one loop in openInput: reuse the first mapping where the file is not open yet or its slice serves the context, otherwise map the data file again.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Open files twice for sequential reads only when it helps [lucene]
Posted by "thecoop (via GitHub)" <gi...@apache.org>.
thecoop commented on code in PR #16883:
URL: https://github.com/apache/lucene/pull/16883#discussion_r4350105858
##########
lucene/core/src/java/org/apache/lucene/codecs/lucene90/Lucene90CompoundReader.java:
##########
@@ -148,13 +150,17 @@ private Map<String, FileEntry> readMapping(IndexInput entriesStream) throws IOEx
@Override
public void close() throws IOException {
- IndexInput merge;
+ List<IndexInput> others = new ArrayList<>();
synchronized (this) {
closed = true;
- merge = mergeHandle == handle ? null : mergeHandle;
- mergeHandle = null;
+ for (Mapping mapping : mappings) {
+ if (mapping.data != handle) {
+ others.add(mapping.data);
+ }
+ }
+ mappings.clear();
}
- IOUtils.close(handle, merge);
+ IOUtils.close(handle, () -> IOUtils.close(others));
Review Comment:
Rather than nested `close`, why not just start off with `handle` in `others`, and just close the list items?
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Open files twice for sequential reads only when it helps [lucene]
Posted by "jimczi (via GitHub)" <gi...@apache.org>.
jimczi commented on code in PR #16883:
URL: https://github.com/apache/lucene/pull/16883#discussion_r4361384288
##########
lucene/core/src/java/org/apache/lucene/codecs/lucene90/Lucene90CompoundReader.java:
##########
@@ -176,28 +185,33 @@ public IndexInput openInput(String name, IOContext context) throws IOException {
+ entries.keySet()
+ ")");
}
- IndexInput data = context.context() == IOContext.Context.MERGE ? mergeHandle() : handle;
- return data.slice(name, entry.offset, entry.length, context);
- }
-
- /**
- * A merge reads through a mapping of its own, so the access it advises does not replace the
- * advice of searches reading the same inner files. Merges of this segment share it. Like the
- * search mapping, it is opened without advice: each inner file is advised on its own region.
- */
- private synchronized IndexInput mergeHandle() throws IOException {
- if (closed) {
- throw new AlreadyClosedException("this Directory is closed");
- }
- if (mergeHandle == null) {
- try {
- mergeHandle = directory.openInput(dataFileName, IOContext.DEFAULT);
- } catch (@SuppressWarnings("unused") FileNotFoundException | NoSuchFileException e) {
- // an open reader outlives its files, so fall back to the mapping it already holds
- mergeHandle = handle;
+ synchronized (this) {
+ if (closed) {
+ throw new AlreadyClosedException("this Directory is closed");
+ }
+ List<Slice> fileSlices = slices.computeIfAbsent(id, _ -> new ArrayList<>());
+ int copy = 0;
+ while (copy < fileSlices.size()
+ && fileSlices.get(copy).input().isEfficientFor(context) == false) {
+ copy++;
+ }
+ // each slice of an inner file is on its own copy of the data file, so it keeps its own advice
+ if (copy == fileSlices.size()) {
+ try {
+ if (copy == data.size()) {
+ // without advice, like handle, so inner files advise their own pages
+ data.add(directory.openInput(dataFileName, IOContext.DEFAULT));
+ }
+ IndexInput input = data.get(copy).slice(name, entry.offset, entry.length, context);
Review Comment:
Yeah, it was only kept to call isEfficientFor on, and the link was just the index. I removed it: each copy of the data file now keeps the context each inner file was first opened with on it, and openInput slices with that context and checks the slice it returns. So nothing extra is kept, and there's a second copy only when random and sequential reads of the same file conflict. Easier to follow?
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Open files twice for sequential reads only when it helps [lucene]
Posted by "thecoop (via GitHub)" <gi...@apache.org>.
thecoop commented on code in PR #16883:
URL: https://github.com/apache/lucene/pull/16883#discussion_r4361260480
##########
lucene/core/src/java/org/apache/lucene/codecs/lucene90/Lucene90CompoundReader.java:
##########
@@ -176,28 +185,33 @@ public IndexInput openInput(String name, IOContext context) throws IOException {
+ entries.keySet()
+ ")");
}
- IndexInput data = context.context() == IOContext.Context.MERGE ? mergeHandle() : handle;
- return data.slice(name, entry.offset, entry.length, context);
- }
-
- /**
- * A merge reads through a mapping of its own, so the access it advises does not replace the
- * advice of searches reading the same inner files. Merges of this segment share it. Like the
- * search mapping, it is opened without advice: each inner file is advised on its own region.
- */
- private synchronized IndexInput mergeHandle() throws IOException {
- if (closed) {
- throw new AlreadyClosedException("this Directory is closed");
- }
- if (mergeHandle == null) {
- try {
- mergeHandle = directory.openInput(dataFileName, IOContext.DEFAULT);
- } catch (@SuppressWarnings("unused") FileNotFoundException | NoSuchFileException e) {
- // an open reader outlives its files, so fall back to the mapping it already holds
- mergeHandle = handle;
+ synchronized (this) {
+ if (closed) {
+ throw new AlreadyClosedException("this Directory is closed");
+ }
+ List<Slice> fileSlices = slices.computeIfAbsent(id, _ -> new ArrayList<>());
+ int copy = 0;
+ while (copy < fileSlices.size()
+ && fileSlices.get(copy).input().isEfficientFor(context) == false) {
+ copy++;
+ }
+ // each slice of an inner file is on its own copy of the data file, so it keeps its own advice
+ if (copy == fileSlices.size()) {
+ try {
+ if (copy == data.size()) {
+ // without advice, like handle, so inner files advise their own pages
+ data.add(directory.openInput(dataFileName, IOContext.DEFAULT));
+ }
+ IndexInput input = data.get(copy).slice(name, entry.offset, entry.length, context);
Review Comment:
Oh, it's checked for efficientFor above. But then it's re-opened below. Is there a hidden link between the order of things in `data` and in `fileSlices`?
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Open files twice for sequential reads only when it helps [lucene]
Posted by "thecoop (via GitHub)" <gi...@apache.org>.
thecoop commented on code in PR #16883:
URL: https://github.com/apache/lucene/pull/16883#discussion_r4361477846
##########
lucene/core/src/java/org/apache/lucene/codecs/lucene90/Lucene90CompoundReader.java:
##########
@@ -176,28 +184,31 @@ public IndexInput openInput(String name, IOContext context) throws IOException {
+ entries.keySet()
+ ")");
}
- IndexInput data = context.context() == IOContext.Context.MERGE ? mergeHandle() : handle;
- return data.slice(name, entry.offset, entry.length, context);
- }
-
- /**
- * A merge reads through a mapping of its own, so the access it advises does not replace the
- * advice of searches reading the same inner files. Merges of this segment share it. Like the
- * search mapping, it is opened without advice: each inner file is advised on its own region.
- */
- private synchronized IndexInput mergeHandle() throws IOException {
- if (closed) {
- throw new AlreadyClosedException("this Directory is closed");
- }
- if (mergeHandle == null) {
+ synchronized (this) {
+ if (closed) {
+ throw new AlreadyClosedException("this Directory is closed");
+ }
+ for (Copy copy : copies) {
+ // an inner file keeps the advice it was first opened with on a copy
+ IOContext first = copy.contexts().putIfAbsent(id, context);
+ IndexInput slice =
+ copy.data().slice(name, entry.offset, entry.length, first == null ? context : first);
Review Comment:
It's worth noting slices don't need to be closed - doesn't matter if this is actually not used
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Open files twice for sequential reads only when it helps [lucene]
Posted by "ChrisHegarty (via GitHub)" <gi...@apache.org>.
ChrisHegarty commented on PR #16883:
URL: https://github.com/apache/lucene/pull/16883#issuecomment-6342233005
Should `FilterIndexInput` and `MockIndexInputWrapper` delegate `readsAs`? I think that they may be missing some other delegation also, but I just wanted to ask about `readsAs` here.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Open files twice for sequential reads only when it helps [lucene]
Posted by "thecoop (via GitHub)" <gi...@apache.org>.
thecoop commented on code in PR #16883:
URL: https://github.com/apache/lucene/pull/16883#discussion_r4350191765
##########
lucene/core/src/java/org/apache/lucene/codecs/lucene90/Lucene90CompoundReader.java:
##########
@@ -176,28 +182,50 @@ public IndexInput openInput(String name, IOContext context) throws IOException {
+ entries.keySet()
+ ")");
}
- IndexInput data = context.context() == IOContext.Context.MERGE ? mergeHandle() : handle;
- return data.slice(name, entry.offset, entry.length, context);
+ synchronized (this) {
+ Mapping mapping = mappingFor(id, context);
+ IndexInput slice = mapping.data.slice(name, entry.offset, entry.length, context);
+ mapping.firstOpens.putIfAbsent(id, slice);
+ return slice;
+ }
}
/**
- * A merge reads through a mapping of its own, so the access it advises does not replace the
- * advice of searches reading the same inner files. Merges of this segment share it. Like the
- * search mapping, it is opened without advice: each inner file is advised on its own region.
+ * Returns the first mapping where {@code id} is not open yet or reads as {@code context}, and
+ * opens a new mapping otherwise. This way an inner file is never advised twice on one mapping.
*/
- private synchronized IndexInput mergeHandle() throws IOException {
+ private Mapping mappingFor(String id, IOContext context) throws IOException {
+ assert Thread.holdsLock(this);
if (closed) {
throw new AlreadyClosedException("this Directory is closed");
}
- if (mergeHandle == null) {
- try {
- mergeHandle = directory.openInput(dataFileName, IOContext.DEFAULT);
- } catch (@SuppressWarnings("unused") FileNotFoundException | NoSuchFileException e) {
- // an open reader outlives its files, so fall back to the mapping it already holds
- mergeHandle = handle;
+ if (mappings.isEmpty()) {
+ mappings.add(new Mapping(handle));
+ }
+ for (Mapping mapping : mappings) {
+ IndexInput first = mapping.firstOpens.get(id);
+ if (first == null || first.readsAs(context)) {
+ return mapping;
}
}
- return mergeHandle;
+ IndexInput data;
+ try {
+ // like the reader's mapping, opened without advice
+ data = directory.openInput(dataFileName, IOContext.DEFAULT);
Review Comment:
Doesn't this need to open it with `context`, not `DEFAULT`?
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Open files twice for sequential reads only when it helps [lucene]
Posted by "jimczi (via GitHub)" <gi...@apache.org>.
jimczi commented on PR #16883:
URL: https://github.com/apache/lucene/pull/16883#issuecomment-6317875152
cc @mikemccand @uschindler that s the cleanup you asked for so that directories that cannot apply the hint don t open their files twice. That would also allow to remove all the finishMerge in a follow up since this is not needed anymore.
I opened this PR mainly for the case that random and sequential works in pair and they don t only depend on merge vs default. So a mapping with random needs to have his counter part sequential to handle sequential reads. So a brute force over raw vectors indexed with HNSW and opened with random should be able to expose a sequential reader even for search. That s what this PR provides explicitly.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Open files twice for sequential reads only when it helps [lucene]
Posted by "jimczi (via GitHub)" <gi...@apache.org>.
jimczi commented on code in PR #16883:
URL: https://github.com/apache/lucene/pull/16883#discussion_r4350721082
##########
lucene/core/src/java/org/apache/lucene/store/IndexInput.java:
##########
@@ -238,6 +238,16 @@ public boolean prefetch(long offset, long length) throws IOException {
*/
public void updateIOContext(IOContext context) throws IOException {}
+ /**
+ * Returns true if reading this input is as efficient as reading an input opened with the given
+ * {@code IOContext}. If not, callers that read with this context may open the file again.
Review Comment:
Reworded: it now says when callers should open the file again.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Open files twice for sequential reads only when it helps [lucene]
Posted by "thecoop (via GitHub)" <gi...@apache.org>.
thecoop commented on code in PR #16883:
URL: https://github.com/apache/lucene/pull/16883#discussion_r4361487693
##########
lucene/core/src/java/org/apache/lucene/codecs/lucene90/Lucene90CompoundReader.java:
##########
@@ -176,28 +185,33 @@ public IndexInput openInput(String name, IOContext context) throws IOException {
+ entries.keySet()
+ ")");
}
- IndexInput data = context.context() == IOContext.Context.MERGE ? mergeHandle() : handle;
- return data.slice(name, entry.offset, entry.length, context);
- }
-
- /**
- * A merge reads through a mapping of its own, so the access it advises does not replace the
- * advice of searches reading the same inner files. Merges of this segment share it. Like the
- * search mapping, it is opened without advice: each inner file is advised on its own region.
- */
- private synchronized IndexInput mergeHandle() throws IOException {
- if (closed) {
- throw new AlreadyClosedException("this Directory is closed");
- }
- if (mergeHandle == null) {
- try {
- mergeHandle = directory.openInput(dataFileName, IOContext.DEFAULT);
- } catch (@SuppressWarnings("unused") FileNotFoundException | NoSuchFileException e) {
- // an open reader outlives its files, so fall back to the mapping it already holds
- mergeHandle = handle;
+ synchronized (this) {
+ if (closed) {
+ throw new AlreadyClosedException("this Directory is closed");
+ }
+ List<Slice> fileSlices = slices.computeIfAbsent(id, _ -> new ArrayList<>());
+ int copy = 0;
+ while (copy < fileSlices.size()
+ && fileSlices.get(copy).input().isEfficientFor(context) == false) {
+ copy++;
+ }
+ // each slice of an inner file is on its own copy of the data file, so it keeps its own advice
+ if (copy == fileSlices.size()) {
+ try {
+ if (copy == data.size()) {
+ // without advice, like handle, so inner files advise their own pages
+ data.add(directory.openInput(dataFileName, IOContext.DEFAULT));
+ }
+ IndexInput input = data.get(copy).slice(name, entry.offset, entry.length, context);
Review Comment:
Yes, thats much clearer!
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Open files twice for sequential reads only when it helps [lucene]
Posted by "thecoop (via GitHub)" <gi...@apache.org>.
thecoop commented on code in PR #16883:
URL: https://github.com/apache/lucene/pull/16883#discussion_r4350260798
##########
lucene/core/src/java/org/apache/lucene/codecs/lucene90/Lucene90CompoundReader.java:
##########
@@ -176,28 +182,50 @@ public IndexInput openInput(String name, IOContext context) throws IOException {
+ entries.keySet()
+ ")");
}
- IndexInput data = context.context() == IOContext.Context.MERGE ? mergeHandle() : handle;
- return data.slice(name, entry.offset, entry.length, context);
+ synchronized (this) {
+ Mapping mapping = mappingFor(id, context);
+ IndexInput slice = mapping.data.slice(name, entry.offset, entry.length, context);
+ mapping.firstOpens.putIfAbsent(id, slice);
Review Comment:
It's a bit odd having the id each mapping is for inside the first map. Can `mappings` be a `Map<String, List<IndexInput>>` instead?
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Open files twice for sequential reads only when it helps [lucene]
Posted by "jimczi (via GitHub)" <gi...@apache.org>.
jimczi merged PR #16883:
URL: https://github.com/apache/lucene/pull/16883
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org