apache
Use a confined Arena for IOContext.READONCE
Use a confined Arena for IOContext.READONCE.
This change will require inputs opened with READONCE to be consumed and closed on the creating thread. Further testing and assertions will need to be added.
relates #13325
Loopy testing runs without issue for 100+ times ( I just stopped it at that point ).
$ export x=; while ./gradlew :lucene:core:test ; do echo $x | wc -c; export x=x$x; done
Cool. Seems useful to achieve the goal.
As written in the original issue maybe we should disallow clones, random access and slices of IndexInput on top of that. This may allow us to find more bugs.
In general we should document thois somehow on the methods in the base class Directory that the IOContext has implications on usage from multiple threads.
Cool. Seems useful to achieve the goal.
As written in the original issue maybe we should disallow clones, random access and slices of IndexInput on top of that. This may allow us to find more bugs.
Yeah, we can either pass in a boolean, or something based on MemorySegment::isAccessibleBy. The former seems more straightforward, while the latter seems more flexible to allow slices, etc, but just for the calling thread. But that seems likely incorrect usage and not something we need to add complexity for.
In general we should document thois somehow on the methods in the base class Directory that the IOContext has implications on usage from multiple threads.
++ I'll see if I can find, at least, some minimal wording that we can add here.
I also grepped through source code: https://github.com/search?q=repo%3Aapache%2Flucene%20READONCE&type=code
It looks like IOContext.READONCE is used at all places we are interested in. ChecksumIndexInput is not compatible to multithreading anyways (you can only read sequentially) and the other ones like reading lifedocs also look correct.
There are some occurrences with index sorting. We should make sure that there is no multithreading (optionally) used.
Maybe run all tests with -Ptests.directory=MMapDirectory; this raises chance that we hit a bug.
loopy testing with -Ptests.directory=MMapDirectory all successfully after several dozen runs.
Looks ok to me. Maybe ask @jpountz for his opinion; maybe he has more ideas where we only work single threaded.
Not sure about this: We could possibly also modify the general Exception handler which catches IllegalStateException and rethrow it as IOException. We do this already for closed inputs to throw EOFException. Not sure about pros and cons, but when we add more tests and an "official documentation" for the READONCE case, this might be useful.
https://github.com/apache/lucene/blob/f4cd4b46fc1ccb497624c5c5454785078aca6501/lucene/core/src/java21/org/apache/lucene/store/MemorySegmentIndexInput.java#L110-L132
I believe that this could be a problem for cross-data-structure merging concurrency (which we just disabled, but would like to re-enable soon-ish) since merging uses
READONCE. In this case, I'd expect aSegmentReaderto be opened in one thread, and thenXXXProducer#getMergeInstance()(aclone()optimized for merging) to be called on the thread where the actual merging happens. So while the clone would be consumed in the thread where it's been created, the arena was previously initialized in a different thread?
Merging uses sequential read ahead but from my review it never passes the constant IOContext.READONCE (see the link with references to READONCE above).
Merging uses an IOContext with a MergeInfo attached. This uses same ReadAdvice SEQUENTIAL, but that is different to the Singleton READONCE IOContext. Or do I miss something?
Merging uses an IOContext with a MergeInfo attached. This uses same ReadAdvice SEQUENTIAL, but that is different to the Singleton READONCE IOContext. Or do I miss something?
I see same as you @uschindler, however ...
An alternative to modifying the semantics of the existing IOContext.READONCE constant is to add a new constant that indicates both the read-once, sequential, and same-thread semantics. Then opt-in use sites as appropriate, for example, ChecksumIndexInput, etc.
P.S.: Because of this I proposed that READONCE IndexInputs should throw exceptions on clone/slice.
Thanks for checking, I misremembered.
@uschindler +1 to disallowing clones and slices. I' m contemplating making MockDirectoryWrapper also fail if IndexInputs get used in a different thread from the one where they were created.
I'm contemplating making MockDirectoryWrapper also fail if IndexInputs get used in a different thread from the one where they were created.
You mean this for READONCE ones, correct?
I updated clone and slice. It finds some issues. These look like incorrect usage of READONCE.
... there's still some intermittent failures because of this. I'll track them down so that we can discuss.
To me those problems where a CFS file was opened with READONCE look like a bug. It is also unlikely that the CFS file is just opened once and then closed again.
If we want to keep the CFS file support, we should at least allow slices of IndexInput. Whenever you create clones its a bug., because those are used in different threads, but slices may be acceptable (e.g. for CFS files).
To me those problems where a CFS file was opened with READONCE look like a bug. It is also unlikely that the CFS file is just opened once and then closed again.
If we want to keep the CFS file support, we should at least allow slices of IndexInput. Whenever you create clones its a bug., because those are used in different threads, but slices may be acceptable (e.g. for CFS files).
Maybe the pending deletes stuff really only wants to open the CFS file to read deleted docs, but this still looks strange to me.
Hi, when thinking about this. ;Maybe we should for now just disallow clones and not slices. Slices are needed for CFS files.
But nevertheless the code affected by the change seems to have some shortcomings. Why does it need to open a CFS file only to read deletes from it? Shouldn't the CFS file not be open already as the IndexSearcher is open already? MMapping a huge amount of data just to read a few kilobytes looks wrong to me.
Thanks, looks fine now. I think we can revert the changes to the SegmentReader and look into this in another issue. It still looks strange not me, but I had not time to look closely into the code of the parts which used READONCE on CFS files, that you now changed to DEFAULT.
If the code opens the CFS file and closes it later, using READONCE is fine, but not in general to open such a file.
Thanks, looks fine now. I think we can revert the changes to the SegmentReader and look into this in another issue. It still looks strange not me, but I had not time to look closely into the code of the parts which used READONCE on CFS files, that you now changed to DEFAULT.
If the code opens the CFS file and closes it later, using READONCE is fine, but not in general to open such a file.
I reviewed this. We can undo the IOContext#DEFAULT changes for the CFS files.
I am not really sure how we should proceed. The new SegmentReader() which is closed a few lines below looks like a valid use case for READONCE, but SegmentReader creates clones. - damn!
Not sure how we should handle that. Maybe let's remove disallowing of clones and slices, but check that the threads are correct. @jpountz ?
About the CFS files we should really look if we can't get a "better way" to only request a single file from a huge CFS. But this is just an observation we have seen here and has nothing to do with this isse: It seems very bad to mmap a whole huge CFS file just to extract one child file from it in an readonce context. This is an idea for another issue to refactor the CFS Directory to better work with an underlying index input and maybe allow to mmap only slices from begining. This is more complicated to implement but might be worth to look into.
I'm contemplating making MockDirectoryWrapper also fail if IndexInputs get used in a different thread from the one where they were created.
You mean this for READONCE ones, correct?
I meant all IndexInputs actually, they should all be used in the thread where they were created (either via Directory#openInput or IndexInput#clone)? But I'm happy to look into it in a separate issue.
Not sure how we should handle that. Maybe let's remove disallowing of clones and slices, but check that the threads are correct. @jpountz ?
This sounds like ok progress over perferction to me. We can look into getting stricter in a followup.
Thanks for the comments so far. I updated the PR to only check same-thread semantics for MSII clone and slice. And also added some basic thread checks to MockIndexInputWrapper. I think this is a reasonable improvement and a good place to stop in this PR. As suggested, further improvements can be done as a follow up.
There are some test failures due to strict thread checking. I think the mock input should only do this when its in confined mode.