Make TokenStream Reuse Mandatory for Analyzers [LUCENE-3396]

[asfimport]

In #3385 it became clear that we'd benefit a lot from Analyzer having to return reusable TokenStreams. This is a big chunk of work, but its time to bite the bullet.

I plan to attack this in the following way:

• Collapse the logic of ReusableAnalyzerBase into Analyzer
• Add a ReuseStrategy abstraction to Analyzer which controls whether the TokenStreamComponents are reused globally (as they are today) or per-field.
• Convert all Analyzers over to using TokenStreamComponents. I've already seen that some of the TokenStreams created in tests need some work to be reusable (even if they aren't reused).
• Remove Analyzer.reusableTokenStream and convert everything over to using .tokenStream (which will now be returning reusable TokenStreams).

---

Migrated from LUCENE-3396 by Chris Male, resolved Nov 14 2011
Attachments: LUCENE-3396-forgotten.patch, LUCENE-3396-rab.patch (versions: 7), LUCENE-3396-remaining-analyzers.patch, LUCENE-3396-remaining-merging.patch
Linked issues:

• CharFilters not being invoked in Solr [LUCENE-3721] #4795

Sub-tasks:

• Cleanup Test TokenStreams so they are reusable [LUCENE-3397] #4470
• Deprecate / Remove DutchAnalyzer.setStemDictionary [LUCENE-3400] #4473
• All Analysis Consumers should use reusableTokenStream [LUCENE-3455] #4529
• Analysis Consumers should end and close any TokenStreams [LUCENE-3456] #4530
• Rename Analyzer.reusableTokenStream() to tokenStream() [LUCENE-3466] #4540

[asfimport]

Robert Muir (@rmuir) (migrated from JIRA)

can we create a separate sub-tasks to fix the tests (so we can backport at least a majority of the changes)?

[asfimport]

Chris Male (migrated from JIRA)

Do you mean correcting the TokenStreams in tests so they are reusable?

[asfimport]

Robert Muir (@rmuir) (migrated from JIRA)

yeah: just thinking it could be a nice cleanup? Some of these are messy :)

[asfimport]

Chris Male (migrated from JIRA)

Done. #4470

[asfimport]

Chris Male (migrated from JIRA)

This is the uber patch which converts all but 8 Analyzers over to using ReusableAnalyzerBase.

This also introduces the ReuseStrategy concept to ReusableAnalyzerBase (will need some documentation before committing). For simplicity, the ReuseStrategy maintains its own CloseableThreadLocal.

The remaining Analyzers can be converted when I push the ReuseAnalyzerBase logic into Analyzer.

After some small tidy (and a review) I think we should commit this even though we're going to collapse ReusabeAnalyzerBase.

[asfimport]

Chris Male (migrated from JIRA)

Improved patch which does some cleanup and documentation ReuseAnalyzerBase.ReuseStrategy.

I'm seeing one test fail in TestPayloads, this class does some insanely non-reusable stuff with payloads. I'll keep digging.

[asfimport]

Chris Male (migrated from JIRA)

Grrr TestPayloads does some really non-reusable stuff. The only two options I can see are either using a NoReuseStrategy implementation in the test, or instantiate the Analyzer again every time it does something crazy.

I've chosen the latter since I don't want to start the NoReuseStrategy slippery slope.

We're in good shape now (minus this stupidity).

[asfimport]

Robert Muir (@rmuir) (migrated from JIRA)

just a quick glance: the MockAnalyzer needs to reuse-per-field?

This way we test the case where payloads are enabled for one field but not for another.

[asfimport]

Chris Male (migrated from JIRA)

Absolutely, I missed that fieldname usage.

Updated patch. Still all green.

[asfimport]

Simon Willnauer (@s1monw) (migrated from JIRA)

this patch looks good to me! I like the reuse strategy and how you factored out the thread local stuff.
I think we should commit and let bake this in?

[asfimport]

Uwe Schindler (@uschindler) (migrated from JIRA)

Hi, the ReuseStrategies look fine. I am just confused about the Generics. Why not make the whole abstract ReuseStrategy T-typed? Then also ThreadLocal<T> is used and no casting anywhere. The subclasses for PerField and global then are typed to the correct class (Map<> or TSComponents).

[asfimport]

Chris Male (migrated from JIRA)

Hi Uwe,

I originally had ReuseStrategy with a generic type but then decided it was overkill since it only benefits implementations, not users of ReuseStrategy. If we want the extra type safety, I'll happily make the change.

[asfimport]

Uwe Schindler (@uschindler) (migrated from JIRA)

I agree its somehow overkill. But if not on class level I would even remove the T parameter from ther getter method, because it does not really fit, it is only used there, not even on the setter. There is no type enforcement anywhere, so the extra T is just to remove the casting on the caller of the protected method, but adding a SuppressWarnings on the implementor's side. So either make all T or use Object everywhere.

[asfimport]

Chris Male (migrated from JIRA)

Patch updated to trunk.

Generic <T> is removed from ReuseStrategy.

[asfimport]

Chris Male (migrated from JIRA)

I'm going to commit this soon and then work on the remaining Analyzers.