From 1bdc6e6c74595afaeab3a5ef4dc04436cd1dcfe4 Mon Sep 17 00:00:00 2001 From: Tim Underwood Date: Wed, 17 Oct 2018 08:33:16 -0700 Subject: [PATCH 1/3] SOLR-12878: Use the cached FieldInfos instance in SolrIndexSearcher instead of re-constructing it every time FacetFieldProcessorByHashDV is created --- .../apache/solr/search/facet/FacetFieldProcessorByHashDV.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/solr/core/src/java/org/apache/solr/search/facet/FacetFieldProcessorByHashDV.java b/solr/core/src/java/org/apache/solr/search/facet/FacetFieldProcessorByHashDV.java index c246c210217c..5070a9765ba1 100644 --- a/solr/core/src/java/org/apache/solr/search/facet/FacetFieldProcessorByHashDV.java +++ b/solr/core/src/java/org/apache/solr/search/facet/FacetFieldProcessorByHashDV.java @@ -199,7 +199,7 @@ protected Comparable parseAndAddGap(Comparable value, String gap) throws ParseEx throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, getClass()+" doesn't support prefix"); // yet, but it could } - FieldInfo fieldInfo = fcontext.searcher.getSlowAtomicReader().getFieldInfos().fieldInfo(sf.getName()); + FieldInfo fieldInfo = fcontext.searcher.getFieldInfos().fieldInfo(sf.getName()); if (fieldInfo != null && fieldInfo.getDocValuesType() != DocValuesType.NUMERIC && fieldInfo.getDocValuesType() != DocValuesType.SORTED && From cb34d9116af2b5aa473b1e195f256f4931fff58b Mon Sep 17 00:00:00 2001 From: Tim Underwood Date: Fri, 2 Nov 2018 08:23:07 -0700 Subject: [PATCH 2/3] SOLR-12878: Move caching of FieldInfos instance from SolrIndexSearcher to SlowCompositeReaderWrapper --- .../apache/solr/index/SlowCompositeReaderWrapper.java | 9 +++++++-- .../java/org/apache/solr/search/SolrIndexSearcher.java | 7 ++----- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/solr/core/src/java/org/apache/solr/index/SlowCompositeReaderWrapper.java b/solr/core/src/java/org/apache/solr/index/SlowCompositeReaderWrapper.java index 441abae2c457..94e6d3732d9f 100644 --- a/solr/core/src/java/org/apache/solr/index/SlowCompositeReaderWrapper.java +++ b/solr/core/src/java/org/apache/solr/index/SlowCompositeReaderWrapper.java @@ -55,6 +55,11 @@ public final class SlowCompositeReaderWrapper extends LeafReader { // but do we really need to optimize slow-wrapper any more? final Map cachedOrdMaps = new HashMap<>(); + // Cached copy of FieldInfos to prevent it from being re-created on each + // getFieldInfos call. Most (if not all) other LeafReader implementations + // also have a cached FieldInfos instance so this is consistent. SOLR-12878 + final FieldInfos fieldInfos; + /** This method is sugar for getting an {@link LeafReader} from * an {@link IndexReader} of any kind. If the reader is already atomic, * it is returned unchanged, otherwise wrapped by this class. @@ -71,6 +76,7 @@ public static LeafReader wrap(IndexReader reader) throws IOException { SlowCompositeReaderWrapper(CompositeReader reader) throws IOException { in = reader; in.registerParentReader(this); + fieldInfos = FieldInfos.getMergedFieldInfos(in); if (reader.leaves().isEmpty()) { metaData = new LeafMetaData(Version.LATEST.major, Version.LATEST, null); } else { @@ -273,8 +279,7 @@ public PointValues getPointValues(String field) { @Override public FieldInfos getFieldInfos() { - ensureOpen(); - return FieldInfos.getMergedFieldInfos(in); // TODO cache? + return fieldInfos; } @Override diff --git a/solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java b/solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java index 9b36c975d09a..a659d6631bd9 100644 --- a/solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java +++ b/solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java @@ -126,8 +126,6 @@ public class SolrIndexSearcher extends IndexSearcher implements Closeable, SolrI // list of all caches associated with this searcher. private final SolrCache[] cacheList; - private final FieldInfos fieldInfos; - private DirectoryFactory directoryFactory; private final LeafReader leafReader; @@ -263,7 +261,6 @@ public SolrIndexSearcher(SolrCore core, String path, IndexSchema schema, String this.queryResultMaxDocsCached = solrConfig.queryResultMaxDocsCached; this.useFilterForSortedQuery = solrConfig.useFilterForSortedQuery; - this.fieldInfos = leafReader.getFieldInfos(); this.docFetcher = new SolrDocumentFetcher(this, solrConfig, enableCache); this.cachingEnabled = enableCache; @@ -319,7 +316,7 @@ List getLeafContexts() { } public FieldInfos getFieldInfos() { - return fieldInfos; + return leafReader.getFieldInfos(); } /* @@ -494,7 +491,7 @@ public IndexSchema getSchema() { * Returns a collection of all field names the index reader knows about. */ public Iterable getFieldNames() { - return Iterables.transform(fieldInfos, fieldInfo -> fieldInfo.name); + return Iterables.transform(getFieldInfos(), fieldInfo -> fieldInfo.name); } public SolrCache getFilterCache() { From 20025c5ae8cedbe3e05a469dc5f9b3c299dbb717 Mon Sep 17 00:00:00 2001 From: Tim Underwood Date: Fri, 2 Nov 2018 08:23:18 -0700 Subject: [PATCH 3/3] SOLR-12878: Added test to verify that LeafReader.getFieldInfos() returns the same instance each time it is called along with changes to various LeafReader implementations to reflect this --- .../java/org/apache/lucene/index/FieldInfos.java | 2 ++ .../java/org/apache/lucene/index/LeafReader.java | 4 ++++ .../apache/lucene/index/TestPendingDeletes.java | 2 +- .../lucene/index/TestTermVectorsReader.java | 2 +- .../apache/lucene/index/memory/MemoryIndex.java | 15 +++++++++------ .../java/org/apache/lucene/search/QueryUtils.java | 2 +- .../src/java/org/apache/lucene/util/TestUtil.java | 5 +++++ .../solr/handler/component/ExpandComponent.java | 8 +++++++- .../solr/search/CollapsingQParserPlugin.java | 8 +++++++- .../test/org/apache/solr/search/TestDocSet.java | 2 +- 10 files changed, 38 insertions(+), 12 deletions(-) diff --git a/lucene/core/src/java/org/apache/lucene/index/FieldInfos.java b/lucene/core/src/java/org/apache/lucene/index/FieldInfos.java index 580c50cdcf4a..c58b6da38685 100644 --- a/lucene/core/src/java/org/apache/lucene/index/FieldInfos.java +++ b/lucene/core/src/java/org/apache/lucene/index/FieldInfos.java @@ -126,6 +126,8 @@ public FieldInfos(FieldInfo[] infos) { values = Collections.unmodifiableCollection(Arrays.asList(valuesTemp.toArray(new FieldInfo[0]))); } + public final static FieldInfos EMPTY = new FieldInfos(new FieldInfo[0]); + /** Call this to get the (merged) FieldInfos for a * composite reader. *

diff --git a/lucene/core/src/java/org/apache/lucene/index/LeafReader.java b/lucene/core/src/java/org/apache/lucene/index/LeafReader.java index faea32de2883..1d09742fbda3 100644 --- a/lucene/core/src/java/org/apache/lucene/index/LeafReader.java +++ b/lucene/core/src/java/org/apache/lucene/index/LeafReader.java @@ -206,6 +206,10 @@ public final PostingsEnum postings(Term term) throws IOException { /** * Get the {@link FieldInfos} describing all fields in * this reader. + * + * Note: Implementations should cache the FieldInfos + * instance returned by this method such that subsequent + * calls to this method return the same instance. * @lucene.experimental */ public abstract FieldInfos getFieldInfos(); diff --git a/lucene/core/src/test/org/apache/lucene/index/TestPendingDeletes.java b/lucene/core/src/test/org/apache/lucene/index/TestPendingDeletes.java index d4530344adf9..e82ec10d0e6f 100644 --- a/lucene/core/src/test/org/apache/lucene/index/TestPendingDeletes.java +++ b/lucene/core/src/test/org/apache/lucene/index/TestPendingDeletes.java @@ -132,7 +132,7 @@ public void testIsFullyDeleted() throws IOException { SegmentInfo si = new SegmentInfo(dir, Version.LATEST, Version.LATEST, "test", 3, false, Codec.getDefault(), Collections.emptyMap(), StringHelper.randomId(), new HashMap<>(), null); SegmentCommitInfo commitInfo = new SegmentCommitInfo(si, 0, 0, -1, -1, -1); - FieldInfos fieldInfos = new FieldInfos(new FieldInfo[0]); + FieldInfos fieldInfos = FieldInfos.EMPTY; si.getCodec().fieldInfosFormat().write(dir, si, "", fieldInfos, IOContext.DEFAULT); PendingDeletes deletes = newPendingDeletes(commitInfo); for (int i = 0; i < 3; i++) { diff --git a/lucene/core/src/test/org/apache/lucene/index/TestTermVectorsReader.java b/lucene/core/src/test/org/apache/lucene/index/TestTermVectorsReader.java index 76947dd6852f..e2a3f206c284 100644 --- a/lucene/core/src/test/org/apache/lucene/index/TestTermVectorsReader.java +++ b/lucene/core/src/test/org/apache/lucene/index/TestTermVectorsReader.java @@ -47,7 +47,7 @@ public class TestTermVectorsReader extends LuceneTestCase { private int[][] positions = new int[testTerms.length][]; private Directory dir; private SegmentCommitInfo seg; - private FieldInfos fieldInfos = new FieldInfos(new FieldInfo[0]); + private FieldInfos fieldInfos = FieldInfos.EMPTY; private static int TERM_FREQ = 3; private static class TestToken implements Comparable { diff --git a/lucene/memory/src/java/org/apache/lucene/index/memory/MemoryIndex.java b/lucene/memory/src/java/org/apache/lucene/index/memory/MemoryIndex.java index 0078c3f53550..fde9438e9c90 100644 --- a/lucene/memory/src/java/org/apache/lucene/index/memory/MemoryIndex.java +++ b/lucene/memory/src/java/org/apache/lucene/index/memory/MemoryIndex.java @@ -1142,12 +1142,20 @@ private void prepareForUsage() { private final class MemoryIndexReader extends LeafReader { private final MemoryFields memoryFields = new MemoryFields(fields); + private final FieldInfos fieldInfos; private MemoryIndexReader() { super(); // avoid as much superclass baggage as possible + + FieldInfo[] fieldInfosArr = new FieldInfo[fields.size()]; + + int i = 0; for (Info info : fields.values()) { info.prepareDocValuesAndPointValues(); + fieldInfosArr[i++] = info.fieldInfo; } + + fieldInfos = new FieldInfos(fieldInfosArr); } private Info getInfoForExpectedDocValuesType(String fieldName, DocValuesType expectedType) { @@ -1171,12 +1179,7 @@ public Bits getLiveDocs() { @Override public FieldInfos getFieldInfos() { - FieldInfo[] fieldInfos = new FieldInfo[fields.size()]; - int i = 0; - for (Info info : fields.values()) { - fieldInfos[i++] = info.fieldInfo; - } - return new FieldInfos(fieldInfos); + return fieldInfos; } @Override diff --git a/lucene/test-framework/src/java/org/apache/lucene/search/QueryUtils.java b/lucene/test-framework/src/java/org/apache/lucene/search/QueryUtils.java index d7f63cbc9c01..b7984aa37f4f 100644 --- a/lucene/test-framework/src/java/org/apache/lucene/search/QueryUtils.java +++ b/lucene/test-framework/src/java/org/apache/lucene/search/QueryUtils.java @@ -207,7 +207,7 @@ public NumericDocValues getNormValues(String field) throws IOException { @Override public FieldInfos getFieldInfos() { - return new FieldInfos(new FieldInfo[0]); + return FieldInfos.EMPTY; } final Bits liveDocs = new Bits.MatchNoBits(maxDoc); diff --git a/lucene/test-framework/src/java/org/apache/lucene/util/TestUtil.java b/lucene/test-framework/src/java/org/apache/lucene/util/TestUtil.java index fee918fe122d..64287a0111b5 100644 --- a/lucene/test-framework/src/java/org/apache/lucene/util/TestUtil.java +++ b/lucene/test-framework/src/java/org/apache/lucene/util/TestUtil.java @@ -335,6 +335,11 @@ public static void checkReader(LeafReader reader, boolean doSlowChecks) throws I } assert Accountables.toString(sr) != null; } + + // FieldInfos should be cached at the reader and always return the same instance + if (reader.getFieldInfos() != reader.getFieldInfos()) { + throw new RuntimeException("getFieldInfos() returned different instances for class: "+reader.getClass()); + } } // used by TestUtil.checkReader to check some things really unrelated to the index, diff --git a/solr/core/src/java/org/apache/solr/handler/component/ExpandComponent.java b/solr/core/src/java/org/apache/solr/handler/component/ExpandComponent.java index 2cbe703d6b2e..8e61bb04a0c3 100644 --- a/solr/core/src/java/org/apache/solr/handler/component/ExpandComponent.java +++ b/solr/core/src/java/org/apache/solr/handler/component/ExpandComponent.java @@ -769,17 +769,19 @@ public Category getCategory() { private static class ReaderWrapper extends FilterLeafReader { private String field; + private FieldInfos fieldInfos; public ReaderWrapper(LeafReader leafReader, String field) { super(leafReader); this.field = field; + this.fieldInfos = initFieldInfos(); } public SortedDocValues getSortedDocValues(String field) { return null; } - public FieldInfos getFieldInfos() { + private FieldInfos initFieldInfos() { Iterator it = in.getFieldInfos().iterator(); List newInfos = new ArrayList<>(); while(it.hasNext()) { @@ -809,6 +811,10 @@ public FieldInfos getFieldInfos() { return infos; } + public FieldInfos getFieldInfos() { + return fieldInfos; + } + // NOTE: delegating the caches is wrong here as we are altering the content // of the reader, this should ONLY be used under an uninvertingreader which // will restore doc values back using uninversion, otherwise all sorts of diff --git a/solr/core/src/java/org/apache/solr/search/CollapsingQParserPlugin.java b/solr/core/src/java/org/apache/solr/search/CollapsingQParserPlugin.java index 57d3d43b07d4..a074c75e9f30 100644 --- a/solr/core/src/java/org/apache/solr/search/CollapsingQParserPlugin.java +++ b/solr/core/src/java/org/apache/solr/search/CollapsingQParserPlugin.java @@ -390,10 +390,12 @@ public DelegatingCollector getFilterCollector(IndexSearcher indexSearcher) { private static class ReaderWrapper extends FilterLeafReader { private String field; + private FieldInfos fieldInfos; public ReaderWrapper(LeafReader leafReader, String field) { super(leafReader); this.field = field; + this.fieldInfos = initFieldInfos(); } // NOTE: delegating the caches is wrong here as we are altering the content @@ -415,7 +417,7 @@ public SortedDocValues getSortedDocValues(String field) { return null; } - public FieldInfos getFieldInfos() { + private FieldInfos initFieldInfos() { Iterator it = in.getFieldInfos().iterator(); List newInfos = new ArrayList(); while(it.hasNext()) { @@ -441,6 +443,10 @@ public FieldInfos getFieldInfos() { FieldInfos infos = new FieldInfos(newInfos.toArray(new FieldInfo[newInfos.size()])); return infos; } + + public FieldInfos getFieldInfos() { + return fieldInfos; + } } diff --git a/solr/core/src/test/org/apache/solr/search/TestDocSet.java b/solr/core/src/test/org/apache/solr/search/TestDocSet.java index b59231700956..7672692d25b6 100644 --- a/solr/core/src/test/org/apache/solr/search/TestDocSet.java +++ b/solr/core/src/test/org/apache/solr/search/TestDocSet.java @@ -391,7 +391,7 @@ public int numDocs() { @Override public FieldInfos getFieldInfos() { - return new FieldInfos(new FieldInfo[0]); + return FieldInfos.EMPTY; } @Override