This is not unlike what we had to do in creating CloseableThreadLocal... that was another case where the underlying impl failed to free things as immediately as we'd like.

A better approach is to don IR-keyed weakHashMaps completely and bind everything you need onto IR itself. That's how I do it and it works like a charm.

LUCENE-831...

A better approach is to don IR-keyed weakHashMaps completely and bind everything you need onto IR itself. That's how I do it and it works like a charm.

That would be nice... I'd love to see a
Map<Object,Object> IndexReader.getInfo()
That was usable by anyone (not just the field cache) to associate stuff with a reader.

I'd love to see a Map<Object,Object> IndexReader.getInfo()

I'm currently using - <T> T IndexReader.component(Class<T> key)
Plus a bundle of factories passed to IR on construction. Factories are called after IR is initialized, and also for child IRs and reopens. In case of reopens, besides new IR they are handed the component they produced for the current one (probably better just to pass old IR).

I can try to conjure a patch this weekend.

I'm currently using - <T> T IndexReader.component(Class<T> key)

Thats much better than untyped. Same like AttributeSource.

+1 for a patch.

Please see LUCENE-2133 for a refactoring of FieldCache, which also addresses these problems.

Please see LUCENE-2133 for a refactoring of FieldCache, which also addresses these problems.

It doesn't address a problem of adding custom components to IR. It also does complicate IR beyond that unholy mess it already is.

I think it's better to have an ability to add 'any' kind of component to IR, and then implement whateverCaches over it.

I haven't followed the aforementioned discussion on the mailing list, but I think this issue covers a few things that are not mentioned explicitly here. Maybe it is a good idea to summarize the actual problems/challenges/benefits in a few sentences?

What I understand is that you plan to add arbitrary (cacheable) attributes to IndexReader. I suggest to move these features to the IndexCache proposed in LUCENE-2133. Especially when using decorating IndexReaders (things like "ReadOnlyIndexReader") you would not want to store attributes separately from the decorated IndexReader. The same probably applies to SegmentReader with all its clones.

IndexCache would provide a common base for the extensions you mentioned. (i.e. you are welcome to apply your patches on top of LUCENE-2133).

I would love to see a bigger solution here, but in the interim, I
think we should fix the current FieldCache (patch attached).

The patch adds FieldCache.purge to the interface. This is technically
a break in back-compat, to any external impls of FieldCache, but
that's such an insanely expert & difficult thing that I think it's
fine to make an exception.

A few tests (incl back-compat) needed fixing, because they were
closing the reader in-between to calls to getInts and then incorrectly
asserting the int[]'s were the same.

In 2.9. we wrote in the BW section, that FieldCache interface is no BW problem as nobody ever can implement it (because the FileCacheImpl singleton is the only used one). Ok, you can implement it without any use.

In my opinion, all IndexReaders should call purge, mabe put it on toplevel IR.close default impl? Because if you request FieldCache from Top-level (which you should not do, but you can), it should also be purged.

all IndexReaders should call purge, mabe put it on toplevel IR.close default impl?

Hmm... this actually gets tricky to get right, because of the FieldCacheKey.

EG on closing a SegmentReader that's a clone of another, you don't want to evict it from the FieldCache.

I guess I could fix each of the IndexReader subclasses to evict themselves from the cache. Let me look into that...

Hmm... this actually gets tricky to get right, because of the FieldCacheKey.

It's almost like we want two caches... one with entries that are independent of any changes in deleted docs (like the current FieldCache), and one that isn't.

New patch, also evicts the other subclasses of IR from FieldCache.

+1

I just noticed, it is even possible to retrieve a field cache from the FilterIndexReader and that would be a duplicate of the dlegate's cache. Very ugly.

it is even possible to retrieve a field cache from the FilterIndexReader and that would be a duplicate of the dlegate's cache.

Yeah, not good. Should we default getFieldCacheKey to delegate? A subclass of FIR would presumably need to then override if their filtering altered what's in the field cache.

+1 Good idea, just add a note to the method javadocs, that you have to override this, if you change the contents by the filter.

Honestly, please have a look at LUCENE-2133, I really think it is a good starting point to solve all these problems. Could we perhaps merge the two issues (LUCENE-2133 and LUCENE-2135)?

A quick summary of LUCENE-2133:

The patch allows one or more IndexReaders to share common cache information (whatever this is), stored in the same "IndexCache" instance. The IndexCache is designed to contain any cacheable/volatile information that can be regenerated from the IndexReader.

For example: all clones of SegmentReader share the same SegmentReaderIndexCache with the original instance, containing the ThreadLocals of the "core reader".
By default (for all IndexReader classes) the IndexCache provides access to the "IndexFieldCache" (a non-static reimplementation of FieldCache).

To provide arbitrary cacheable objects we could now extend IndexCache by a simple HashMap (it does not need to be a WeakHashMap, since the IndexCache is closed and purged as soon as the original IndexReader is closed).

If you wish so, with the help of IndexCache we might even easily implement two different field caches for the same IndexCache instance, one that changes with deleted docs and another one that does not. Basically we may add any other kind of cache at a later point without touching IndexReader again. To re-use Earwin Burrfoot statement from above, that would then not "complicate IndexReader beyond that unholy mess it already is."

I definitely plan to have a look at LUCENE-2133, but that's a rather large (and, good, on first read!) change to Lucene. I just don't think it should hold this small change up.

New patch, adds override to FIR.getFieldCacheKey

To provide arbitrary cacheable objects we could now extend IndexCache by a simple HashMap (it does not need to be a WeakHashMap, since the IndexCache is closed and purged as soon as the original IndexReader is closed).

If I'm reading your patch right, to add something as a user to DirectoryReader+SegmentReader, I have to extend SegmentReaderIndexCache, IndexCache, then extend SegmentReader and DirectoryReader, and override all methods in DirectoryReader that create SegmentReader.

My aim is to be able to bind stuff to readers without overriding them, delegating, or touching in any manner except providing certain factories on creation.

If I'm reading your patch right, to add something as a user to DirectoryReader+SegmentReader, I have to extend SegmentReaderIndexCache, IndexCache, then extend SegmentReader and DirectoryReader, and override all methods in DirectoryReader that create SegmentReader.

No, the functionality you intend to have is just not there at the moment. But it could be added directly to IndexCache (and thus, to all subclasses of IndexCache automatically).

My aim is to be able to bind stuff to readers without overriding them, delegating, or touching in any manner except providing certain factories on creation.

We could add getProperty/setProperty methods to IndexCache. You could then bind/get arbitrary objects as follows:

IndexReader ir = // somehow create your new reader
IndexCache cache = ir.getIndexCache();
Object someObject = cache.getProperty(someKey);
cache.setProperty("org.example.someCoolProperty", anotherObject);

(Personally, I prefer standardized string keys to avoid collisions, just like in the Servlet API, for example)

Again, this is not yet implemented, but could be done easily, without affecting any existing IndexReader or the other changes on FieldCache etc.

There is some discussion in LUCENE-2133 where we need a decision that also affects this issue. Could you please check and comment?

Thanks!
Christian

backport