Remove synchronization in SegmentReader.isDeleted

• Key: LUCENE-1329
• Type: Improvement
• Status: Closed
• Resolution: Fixed
• Priority: Trivial
• Assignee: Michael McCandless
• Reporter: Jason Rutherglen
• Component/s: Index
• Affects Version/s: 2.3.1
• Fix Version/s: 2.4

Description

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.

Comments

[Jason Rutherglen]

lucene-1329.patch

[Michael McCandless]

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.

[Sanne Grinovero]

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.

[Yonik Seeley]

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.

[Yonik Seeley]

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.

[Michael McCandless]

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.

[Yonik Seeley]

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).

[Michael McCandless]

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

[Michael McCandless]

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.

[Eks Dev]

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.

[Michael McCandless]

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 LUCENE-753 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.

[Eks Dev]

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...

[Michael McCandless]

I just committed this. Thanks Jason!