Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Trivial Trivial
    • Resolution: Fixed
    • Affects Version/s: 5.3
    • Fix Version/s: 5.5, 6.0
    • Component/s: core/other
    • Labels:
      None
    • Lucene Fields:
      New, Patch Available

      Description

      The design of org.apache.lucene.facet.FacetsConfig encourages reuse of a single instance across multiple threads, yet the current synchronization model is too strict as it doesn't allow for concurrent read operations.

      I'll attach a trivial patch which removes the contention point.

        Activity

        Hide
        Sanne Grinovero added a comment -

        Trivial patch.

        The synchronization isn't needed on `getDimConfig` because it's reading from a ConcurrentMap.

        Synchronization is still needed on setters, but that's not a performance concern as the usage pattern is supposedly to configure the fields once and then reuse the instance mostly reading.

        Show
        Sanne Grinovero added a comment - Trivial patch. The synchronization isn't needed on `getDimConfig` because it's reading from a ConcurrentMap. Synchronization is still needed on setters, but that's not a performance concern as the usage pattern is supposedly to configure the fields once and then reuse the instance mostly reading.
        Hide
        Michael McCandless added a comment -

        Thanks Sanne Grinovero, I think it's safe to remove synchronized from getDimConfig, but why change the type declaration for fieldTypes? Can't it remain a Map (no usage requires specific methods from ConcurrentHashMap?).

        Show
        Michael McCandless added a comment - Thanks Sanne Grinovero , I think it's safe to remove synchronized from getDimConfig , but why change the type declaration for fieldTypes ? Can't it remain a Map (no usage requires specific methods from ConcurrentHashMap ?).
        Hide
        Sanne Grinovero added a comment -

        Hi Michael McCandless! Thanks for checking.
        Yes, of course that first changed line is not required. I just felt it was useful to make it explicit to the reader that these are concurrent maps. Just a matter of style, feel free to revert that if it doesn't fit the Lucene style? Or should I provide an alternative patch?

        Show
        Sanne Grinovero added a comment - Hi Michael McCandless ! Thanks for checking. Yes, of course that first changed line is not required. I just felt it was useful to make it explicit to the reader that these are concurrent maps. Just a matter of style, feel free to revert that if it doesn't fit the Lucene style? Or should I provide an alternative patch?
        Hide
        Michael McCandless added a comment -

        Thanks Sanne Grinovero, I'll put the first part back and commit the 2nd part!

        Show
        Michael McCandless added a comment - Thanks Sanne Grinovero , I'll put the first part back and commit the 2nd part!
        Hide
        ASF subversion and git services added a comment -

        Commit 1716476 from Michael McCandless in branch 'dev/trunk'
        [ https://svn.apache.org/r1716476 ]

        LUCENE-6909: remove unnecessary synchronized keyword

        Show
        ASF subversion and git services added a comment - Commit 1716476 from Michael McCandless in branch 'dev/trunk' [ https://svn.apache.org/r1716476 ] LUCENE-6909 : remove unnecessary synchronized keyword
        Hide
        ASF subversion and git services added a comment -

        Commit 1716477 from Michael McCandless in branch 'dev/branches/branch_5x'
        [ https://svn.apache.org/r1716477 ]

        LUCENE-6909: remove unnecessary synchronized keyword

        Show
        ASF subversion and git services added a comment - Commit 1716477 from Michael McCandless in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1716477 ] LUCENE-6909 : remove unnecessary synchronized keyword
        Hide
        Sanne Grinovero added a comment -

        Thanks!

        Show
        Sanne Grinovero added a comment - Thanks!

          People

          • Assignee:
            Unassigned
            Reporter:
            Sanne Grinovero
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development