Solr
  1. Solr
  2. SOLR-6252

A couple of small improvements to UnInvertedField class.

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 6.0
    • Fix Version/s: 4.10, 6.0
    • Component/s: search
    • Labels:
      None

      Description

      Looks like UnInvertedField#getUnInvertedField has implemented a bit additional synchronization module rather than what is required, and thereby increasing the complexity.

      https://github.com/apache/lucene-solr/blob/trunk/solr/core/src/java/org/apache/solr/request/UnInvertedField.java#L667

      As pointed out in the above link, as the synchronization is performed on the cache variable(which itself will protect the threads from obtaining access to the cache), we can safely remove all the placeholder flags. As long as cache.get() is in synchronized block, we can simply populate the cache with new entries and other threads will be able to see the changes.

      This change has been introduced in https://issues.apache.org/jira/browse/SOLR-2548 (Multithreaded faceting)

      1. SOLR-6252.patch
        3 kB
        Vamsee Yarlagadda
      2. SOLR-6252v2.patch
        3 kB
        Vamsee Yarlagadda
      3. SOLR-6252-v3.patch
        2 kB
        Vamsee Yarlagadda
      4. solr-uninvertedfield-cache.patch
        6 kB
        Avishai Ish-Shalom

        Issue Links

          Activity

          Hide
          Gregory Chanan added a comment -

          // This thread will load this field, don't let other threads try.

          This comment no longer seems relevant

          Show
          Gregory Chanan added a comment - // This thread will load this field, don't let other threads try. This comment no longer seems relevant
          Hide
          Vamsee Yarlagadda added a comment -

          Removed the extra comment as pointed by Greg.

          Show
          Vamsee Yarlagadda added a comment - Removed the extra comment as pointed by Greg.
          Hide
          Mark Miller added a comment -

          Looks good to me. I'll commit this soon.

          Show
          Mark Miller added a comment - Looks good to me. I'll commit this soon.
          Hide
          Mark Miller added a comment -

          Hmm...now I'm looking at this beyond the correctness of how it was taken out...

          Wasn't the intent to pull the creation of the UnInvertedField out of the sync block on cache so that more of them can be constructed in parallel rather than sequentially?

          Show
          Mark Miller added a comment - Hmm...now I'm looking at this beyond the correctness of how it was taken out... Wasn't the intent to pull the creation of the UnInvertedField out of the sync block on cache so that more of them can be constructed in parallel rather than sequentially?
          Hide
          Gregory Chanan added a comment -

          Good point Mark. Maybe a comment to that affect and removing isPlaceholder checks with == checks against the static variable is the right way to go here? [I'm assuming "==" on references vs booleans has similar performance, though I haven't actually checked that myself...]

          Show
          Gregory Chanan added a comment - Good point Mark. Maybe a comment to that affect and removing isPlaceholder checks with == checks against the static variable is the right way to go here? [I'm assuming "==" on references vs booleans has similar performance, though I haven't actually checked that myself...]
          Hide
          Mark Miller added a comment -

          Yeah, sounds reasonable to me.

          Can you take one more pass Vamsee Yarlagadda?

          Show
          Mark Miller added a comment - Yeah, sounds reasonable to me. Can you take one more pass Vamsee Yarlagadda ?
          Hide
          Gregory Chanan added a comment - - edited

          Also, the synchronized lock is held when checking the placeholder, when it really only needs to be held for the get/put.

          (it's subtle to get that correct though, because the non-placeholder needs to be notified correctly, so probably not worth doing.

          Show
          Gregory Chanan added a comment - - edited Also, the synchronized lock is held when checking the placeholder, when it really only needs to be held for the get/put. (it's subtle to get that correct though, because the non-placeholder needs to be notified correctly, so probably not worth doing.
          Hide
          Vamsee Yarlagadda added a comment -

          Mark Miller Good point!
          Updated the patch to reflect the suggestions.
          Thanks Gregory Chanan.

          Show
          Vamsee Yarlagadda added a comment - Mark Miller Good point! Updated the patch to reflect the suggestions. Thanks Gregory Chanan .
          Hide
          ASF subversion and git services added a comment -

          Commit 1612422 from Mark Miller in branch 'dev/trunk'
          [ https://svn.apache.org/r1612422 ]

          SOLR-6252: A couple of small improvements to UnInvertedField class.

          Show
          ASF subversion and git services added a comment - Commit 1612422 from Mark Miller in branch 'dev/trunk' [ https://svn.apache.org/r1612422 ] SOLR-6252 : A couple of small improvements to UnInvertedField class.
          Hide
          ASF subversion and git services added a comment -

          Commit 1612423 from Mark Miller in branch 'dev/branches/branch_4x'
          [ https://svn.apache.org/r1612423 ]

          SOLR-6252: A couple of small improvements to UnInvertedField class.

          Show
          ASF subversion and git services added a comment - Commit 1612423 from Mark Miller in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1612423 ] SOLR-6252 : A couple of small improvements to UnInvertedField class.
          Hide
          Mark Miller added a comment -

          Thanks Vamsee and Greg!

          Show
          Mark Miller added a comment - Thanks Vamsee and Greg!
          Hide
          Avishai Ish-Shalom added a comment -

          The new code is still vulnerable to deadlock: if a call gets to the 2nd synchronized block and blocks on cache.wait() then it will never be released since cache.notifyAll() is also inside a synchronized block of the same monitor object.

          Show
          Avishai Ish-Shalom added a comment - The new code is still vulnerable to deadlock: if a call gets to the 2nd synchronized block and blocks on cache.wait() then it will never be released since cache.notifyAll() is also inside a synchronized block of the same monitor object.
          Hide
          Avishai Ish-Shalom added a comment -

          A patch using a single synchronized block and no .wait() calls. should be free of deadlocks.

          Show
          Avishai Ish-Shalom added a comment - A patch using a single synchronized block and no .wait() calls. should be free of deadlocks.
          Hide
          Mark Miller added a comment -

          Thanks Avishai.

          Since this is a released issue, do you mind filing a new bug and attaching your patch? We can link that issue to this one.

          Show
          Mark Miller added a comment - Thanks Avishai. Since this is a released issue, do you mind filing a new bug and attaching your patch? We can link that issue to this one.
          Hide
          Avishai Ish-Shalom added a comment -

          done

          Show
          Avishai Ish-Shalom added a comment - done

            People

            • Assignee:
              Mark Miller
              Reporter:
              Vamsee Yarlagadda
            • Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development