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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
