Uploaded image for project: 'Lucene - Core'
  1. Lucene - Core
  2. LUCENE-1329

Remove synchronization in SegmentReader.isDeleted

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Trivial
    • Resolution: Fixed
    • Affects Version/s: 2.3.1
    • Fix Version/s: 2.4
    • Component/s: core/index
    • Labels:
      None
    • Lucene Fields:
      New, Patch Available

      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.

      1. lucene-1329.patch
        3 kB
        Jason Rutherglen
      2. LUCENE-1329.patch
        33 kB
        Michael McCandless
      3. LUCENE-1329.patch
        26 kB
        Michael McCandless

        Activity

        Hide
        jasonrutherglen Jason Rutherglen added a comment -

        lucene-1329.patch

        Show
        jasonrutherglen Jason Rutherglen added a comment - lucene-1329.patch
        Hide
        mikemccand Michael McCandless added a comment -

        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.

        Show
        mikemccand Michael McCandless added a comment - 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.
        Hide
        sanne.grinovero Sanne Grinovero added a comment -

        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.

        Show
        sanne.grinovero Sanne Grinovero added a comment - 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.
        Hide
        yseeley@gmail.com Yonik Seeley added a comment -

        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.

        Show
        yseeley@gmail.com Yonik Seeley added a comment - 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.
        Hide
        yseeley@gmail.com Yonik Seeley added a comment -

        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.

        Show
        yseeley@gmail.com Yonik Seeley added a comment - 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.
        Hide
        mikemccand Michael McCandless added a comment -

        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.

        Show
        mikemccand Michael McCandless added a comment - 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.
        Hide
        yseeley@gmail.com Yonik Seeley added a comment -

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

        Show
        yseeley@gmail.com Yonik Seeley added a comment - 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).
        Hide
        mikemccand Michael McCandless added a comment -

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

        Show
        mikemccand Michael McCandless added a comment - I'd like to get this (read-only IndexReader) into 2.4.
        Hide
        mikemccand Michael McCandless added a comment -

        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.
        Show
        mikemccand Michael McCandless added a comment - 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.
        Hide
        eksdev Eks Dev added a comment -

        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.

        Show
        eksdev Eks Dev added a comment - 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.
        Hide
        mikemccand Michael McCandless added a comment -

        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.

        Show
        mikemccand Michael McCandless added a comment - 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.
        Hide
        eksdev Eks Dev added a comment -

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

        Show
        eksdev Eks Dev added a comment - 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...
        Hide
        mikemccand Michael McCandless added a comment -

        I just committed this. Thanks Jason!

        Show
        mikemccand Michael McCandless added a comment - I just committed this. Thanks Jason!

          People

          • Assignee:
            mikemccand Michael McCandless
            Reporter:
            jasonrutherglen Jason Rutherglen
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development