Remove synchronization in SegmentReader.isDeleted [LUCENE-1329]

[asfimport]

Removes SegmentReader.isDeleted synchronization by using a volatile deletedDocs variable on Java 1.5 platforms. On Java 1.4 platforms synchronization is limited to obtaining the deletedDocs reference.

---

Migrated from LUCENE-1329 by Jason Rutherglen, resolved Aug 23 2008
Attachments: lucene-1329.patch, LUCENE-1329.patch (versions: 2)

[asfimport]

Jason Rutherglen (migrated from JIRA)

lucene-1329.patch

[asfimport]

Michael McCandless (@mikemccand) (migrated from JIRA)

Jason, have you run any scale tests (w/ many threads) to confirm whether volatile is faster than using synchronized, for this case? I'm especially curious on what happens with JRE 1.4, since with this patch it is now synchronized and volatile.

I think we should, at least in addition but perhaps instead, create a way to open a read-only IndexReader. This way no synchronization nor volatile would be necessary when accessing deletedDocs.

[asfimport]

Sanne Grinovero (migrated from JIRA)

Adding a readonly IndexReader would be really great, I'm contributing some code to Hibernate Search (integration of Lucene and Hibernate) and that
project could really benefit from that.

[asfimport]

Yonik Seeley (@yonik) (migrated from JIRA)

I think we should, at least in addition but perhaps instead, create a way to open a read-only IndexReader.

Right... a volatile is still "half" a synchronized in many ways, and gets more expensive as you add more cores. IAFAIK It's also something you won't see with a profiler because it involves cache flushes, not explicit high level blocking.

[asfimport]

Yonik Seeley (@yonik) (migrated from JIRA)

Once we have a read-only IndexReader, if we still want the deleting-reader then we could also weaken the semantics of deleteDocument such that applications would need to synchronize themselves to guarantee visibility to another thread.

We could safely do this for a deleting-reader by pre-allocating the BitVector objects, thus eliminating the possibility of a thread seeing a partially constructed object.

[asfimport]

Michael McCandless (@mikemccand) (migrated from JIRA)

I took a first cut at creating an explicit read only IndexReader
(attached), which is an alternative to the first patch here.

I added "boolean readOnly" to 3 of the IndexReader open methods, and
created ReadOnlySegmentReader and ReadOnlyMultiSegmentReader. The
classes are trivial – they subclass the parent and just override
acquireWriteLock (to throw an exception) and isDeleted.

reopen() also preserves readOnly-ness, and I fixed merging to open readOnly
IndexReaders.

We could safely do this for a deleting-reader by pre-allocating the BitVector objects, thus eliminating the possibility of a thread seeing a partially constructed object.

I didn't do this one yet ... it makes me a bit nervous because it
means that people who just do IndexReader.open on an index with no
deletions pay the RAM cost upfront of allocating the BitVector.

Really, I think we want to default IndexReader.open to be
readOnly=true (which we can't do until 3.0) at which point doing the
above seems safer since you'd have to go out of your way to open a
non-readOnly IndexReader.

[asfimport]

Yonik Seeley (@yonik) (migrated from JIRA)

I didn't do this one yet ... it makes me a bit nervous because it means that people who just do IndexReader.open on an index with no deletions pay the RAM cost upfront of allocating the BitVector.

Right, which is why I said it was for a "deleting-reader" (which presumes the existence of read-only-readers).

[asfimport]

Michael McCandless (@mikemccand) (migrated from JIRA)

I'd like to get this (read-only IndexReader) into 2.4.

[asfimport]

Michael McCandless (@mikemccand) (migrated from JIRA)

Attached new rev of this patch. I think it's ready to commit. I'll wait a few days.

Changes:

• Updated javadocs.

• Stated clearly that in 3.0 the default for readOnly will switch from false to true.

• Factored out IndexReader.READ_ONLY_DEFAULT so we have one clear place to change from false to true, in 3.0.

[asfimport]

Eks Dev (migrated from JIRA)

Mike, did someone measure what this brings?

This practically reduces need to have many IndexReader-s in MT setup when Index is used in read only case.

[asfimport]

Michael McCandless (@mikemccand) (migrated from JIRA)

Mike, did someone measure what this brings?

I don't think so – I haven't yet tested how much of a bottleneck this was / how much it helps that isDeleted is no longer synchronized.

This practically reduces need to have many IndexReader-s in MT setup when Index is used in read only case.

I really want to get Lucene to this point, but I fear #1828 may still stand in the way since many threads can pile up when accessing the same file. Sadly, an optimized index exacerbates the situation (the polar opposite of what you'd expect when optimizing an index). On every platform except Windows, this patch combined with NIOFSDirectory ought to solve all known search-time bottlenecks.

[asfimport]

Eks Dev (migrated from JIRA)

ok, I see, thanks.
At least, It resolves an issue completely for RAM based indexes.

We have seen performance drop for RAM based index when we switched to MT setup with shared IndexReader, I am not yet sure what is the reason for it, problems in our code or this is indeed related to lucene. I am talking about 25-30% drop on 3 Threads on 4-Core CPU. Must measure it properly...

[asfimport]

Michael McCandless (@mikemccand) (migrated from JIRA)

I just committed this. Thanks Jason!