Lucene - Core
  1. Lucene - Core
  2. LUCENE-4156

Improve implementation of DirectoryTaxonomyWriter.getSize()

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.0-ALPHA, 6.0
    • Component/s: modules/facet
    • Labels:
      None
    • Lucene Fields:
      New, Patch Available

      Description

      Current implementation of DirectoryTaxonomyWriter.getSize() is synchrionized and invokes indexWriter.maxDoc(), both harming performance.

        Activity

        Hide
        Shai Erera added a comment -

        I think that we can remove the synchronized from getSize() and change nextID to either volatile, or AtomicInteger. getSize() is not only public API used by applications, but getParent() uses it too.

        Eliminating synchronization is good !

        Show
        Shai Erera added a comment - I think that we can remove the synchronized from getSize() and change nextID to either volatile, or AtomicInteger. getSize() is not only public API used by applications, but getParent() uses it too. Eliminating synchronization is good !
        Hide
        Shai Erera added a comment -

        Remove synchronization from getSize() and made nextID volatile. I also clarified some javadocs and improved the exception thrown from getParent().

        Show
        Shai Erera added a comment - Remove synchronization from getSize() and made nextID volatile. I also clarified some javadocs and improved the exception thrown from getParent().
        Hide
        Sivan Yogev added a comment -

        Patch looks good, I have one question. In line 829, can we replace getSize() with nextID?

        Show
        Sivan Yogev added a comment - Patch looks good, I have one question. In line 829, can we replace getSize() with nextID?
        Hide
        Shai Erera added a comment -

        I've been thinking about it when I fixed it ... eventually I chose this way because if we'll ever need to change getSize to call writer.maxDoc() again, or implement otherwise for correctness, getParent will benefit from it too. I don't know if it's critical though that there's a method call - is that what bothers you?

        Show
        Shai Erera added a comment - I've been thinking about it when I fixed it ... eventually I chose this way because if we'll ever need to change getSize to call writer.maxDoc() again, or implement otherwise for correctness, getParent will benefit from it too. I don't know if it's critical though that there's a method call - is that what bothers you?
        Hide
        Sivan Yogev added a comment -

        Since getParent() could be heavily used during facets indexing I was thinking that reducing the two method calls (one to getParent another for ensureOpen) could make a difference, but you are right that it is not safe for future implementation details. So best keep the patch as is.

        Show
        Sivan Yogev added a comment - Since getParent() could be heavily used during facets indexing I was thinking that reducing the two method calls (one to getParent another for ensureOpen) could make a difference, but you are right that it is not safe for future implementation details. So best keep the patch as is.
        Hide
        Shai Erera added a comment -

        You're right, 2 extra method calls are not worth the defensive code. If we'll change getSize(), I hope that we'll remember to fix getParent too. At any rate, the reason we check the size is for throwing a friendlier exception with text. If someone passes an invalid ordinal, he'll get AIOOBE anyway, just without the text.

        I'll commit this now.

        Show
        Shai Erera added a comment - You're right, 2 extra method calls are not worth the defensive code. If we'll change getSize(), I hope that we'll remember to fix getParent too. At any rate, the reason we check the size is for throwing a friendlier exception with text. If someone passes an invalid ordinal, he'll get AIOOBE anyway, just without the text. I'll commit this now.
        Hide
        Shai Erera added a comment -

        Committed revisions 1352038 and 1352039.

        Thanks Sivan !

        Show
        Shai Erera added a comment - Committed revisions 1352038 and 1352039. Thanks Sivan !

          People

          • Assignee:
            Shai Erera
            Reporter:
            Sivan Yogev
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development