Hi
I recently read CHANGES to learn more about the readOnly parameter
IndexReader now supports, and came across LUCENE-1329 with a comment that
isDeleted was made not synchronized if readOnly=true (e.g.
ReadOnlyIndexReader), which can affect search code, as it is usually the
bottleneck for search operations.
I searched the code and was surprised to see isDeleted and hasDeletions are
not called from any search code. Instead, the code, such as SegmentTermDocs,
uses the deletedDocs instance directly. So in fact isDeleted wasn't the
bottleneck (unless this was part of the changes in 1329). Anyway, doesn't
matter, that's good !
However, I did find out some indexing code whic calls these two, like
SegmentMerger when it merges fields and term vectors. I think that we can
improve that code by writing some specialized code for merging - if the
reader has no deletions, there's no point checking for each document if
there are deletions and/or if the document was deleted. In fact,
SegmentMerger checks for each document: (1) whether the reader has deletions
and if the document was deleted, (2) if the reader has a matching reader and
(3) if checkAbort is not null.
I have a couple of suggestions to simplify that code:
1. Create two specialized copyFieldsWithDeletions/copyFieldsNoDeletions to
get rid of the unnecessary if (hasDeletions) check for each doc.
2. In these, check if the reader has matching reader or not, and execute the
appropriate code in each case.
3. Also, check up front if checkAbort != null.
3.1 (3) duplicates the code - so perhaps instead I can create a dummy
checkAbort, which does nothing? That way we'll always call
checkAbort.work(). But this adds a method call, so I'm not sure.
(same optimizations for mergeVectors()).
In addition, I think something can be done w/ SegmentTermDocs. Either create
a specialized STD based on whether it has deletions or not, or create a
dummy BitVector which returns false for every check. That way we can
eliminate the checks in each next(), skipTo(). Dummy BitVector will leave
the file as-is, but will add a method call, so I think I lean towards the
specialized STD. This can be as simple as making STD abstract, with a static
create() method which creates the right instance.
I believe there are other places where we can make such optimizations. If
the overall direction seems right to you, I can open an issue and start to
work on a patch. Currently I've patched SegmentMerger, and besides the class
getting bigger, nothing bad happened (meaning all tests pass), though it
might be interesting to check how it affects performance.
What do you think?
Shai