Add a TermContextCache to IndexSearcher [LUCENE-6561]

[asfimport]

TermContexts can be quite expensive to build, and if you have fairly complex queries that re-use the same terms you can end up spending a lot of time re-building the same ones over and over again. It would be nice to be able to cache them on an IndexSearcher, so that they can be re-used.

---

Migrated from LUCENE-6561 by Alan Woodward (@romseygeek), updated Jun 15 2015
Attachments: LUCENE-6561.patch (versions: 2)

[asfimport]

Alan Woodward (@romseygeek) (migrated from JIRA)

Very rough patch that adds a TermContextCache to IndexSearcher, and moves TermContext.build() to IndexSearcher.getTermContext(Term). There are three cache implementations, one no-op (the default), one that caches everything, and a dumb LRU implementation that uses LinkedHashMap. LuceneTestCase.newSearcher() randomly selects the various implementations.

Needs some proper tests, and probably the concurrency on the LRUCache needs more thought. And benchmarking! (do our benchmark tests re-use queries at all?) But I thought I'd put this up to see what people think.

[asfimport]

Robert Muir (@rmuir) (migrated from JIRA)

I have concerns, there used to be a terms dict cache, and we removed it because it caused things like concurrency slowdowns and other issues.

Generally speaking I don't think a cache is beneficial...

[asfimport]

Michael McCandless (@mikemccand) (migrated from JIRA)

I think this kind of caching is better done "higher up" in the stack, e.g. at the server layer, or in the query itself (if it really is a query that "needs" the same term many times)?

[asfimport]

Alan Woodward (@romseygeek) (migrated from JIRA)

Concurrency was one of the things that I was unsure of, and it may be that this is only useful in specific cases (in my case, very large complex queries with lots of redundancy in them being run in series).

Would a good compromise be to keep the move of TermContext.build(), so that if anybody does want to implement a cache, they can subclass IndexSearcher? Difficult to implement your own cache when queries are all calling a static method.

[asfimport]

Alan Woodward (@romseygeek) (migrated from JIRA)

Patch that just moves TermContext.build() to IndexSearcher.

[asfimport]

Robert Muir (@rmuir) (migrated from JIRA)

I don't think we should even encourage caching of this? Its not really designed for that: instead its just a "state" we pass along but it has a very finite life during the processing of the query.

If we do this then it puts additional pressure on the postings format, maybe their impl isnt memory efficient or whatever.

[asfimport]

Robert Muir (@rmuir) (migrated from JIRA)

Also from the API perspective, this TermContext is a lucene implementation detail (to pass along shit from multitermquery rewrite or whatever). It really shouldnt be publicly exposed anywhere: definitely not in an important API like IndexSearcher.

[asfimport]

Alan Woodward (@romseygeek) (migrated from JIRA)

I don't think we should even encourage caching of this?

Well at the moment we don't even make caching possible - TermContext.build() is a static method, so there's no way to do what Mike suggests and build an external cache. I agree that IndexSearcher is the wrong place to expose it though. And a package-private method wouldn't work, because SpanTermQuery is in a different package.

The concurrency problems arise because of sharing the cache between multiple queries, maybe what's really needed is a QueryContext that's passed to createWeight() that holds this kind of info?