Uploaded image for project: 'Solr'
  1. Solr
  2. SOLR-6252

A couple of small improvements to UnInvertedField class.

    Details

    • Type: Improvement
    • Status: Resolved
    • Priority: 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-uninvertedfield-cache.patch
        6 kB
        Avishai Ish-Shalom
      2. SOLR-6252-v3.patch
        2 kB
        Vamsee Yarlagadda
      3. SOLR-6252v2.patch
        3 kB
        Vamsee Yarlagadda
      4. SOLR-6252.patch
        3 kB
        Vamsee Yarlagadda

        Issue Links

          Activity

          Hide
          avishai Avishai Ish-Shalom added a comment -

          done

          Show
          avishai Avishai Ish-Shalom added a comment - done
          Hide
          markrmiller@gmail.com 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
          markrmiller@gmail.com 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 Avishai Ish-Shalom added a comment -

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

          Show
          avishai Avishai Ish-Shalom added a comment - A patch using a single synchronized block and no .wait() calls. should be free of deadlocks.
          Hide
          avishai 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 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
          markrmiller@gmail.com Mark Miller added a comment -

          Thanks Vamsee and Greg!

          Show
          markrmiller@gmail.com Mark Miller added a comment - Thanks Vamsee and Greg!
          Hide
          jira-bot 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
          jira-bot 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
          jira-bot 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
          jira-bot 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
          vamsee Vamsee Yarlagadda added a comment -

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

          Show
          vamsee Vamsee Yarlagadda added a comment - Mark Miller Good point! Updated the patch to reflect the suggestions. Thanks Gregory Chanan .
          Hide
          gchanan 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
          gchanan 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
          markrmiller@gmail.com Mark Miller added a comment -

          Yeah, sounds reasonable to me.

          Can you take one more pass Vamsee Yarlagadda?

          Show
          markrmiller@gmail.com Mark Miller added a comment - Yeah, sounds reasonable to me. Can you take one more pass Vamsee Yarlagadda ?
          Hide
          gchanan 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
          gchanan 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
          markrmiller@gmail.com 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
          markrmiller@gmail.com 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
          markrmiller@gmail.com Mark Miller added a comment -

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

          Show
          markrmiller@gmail.com Mark Miller added a comment - Looks good to me. I'll commit this soon.
          Hide
          vamsee Vamsee Yarlagadda added a comment -

          Removed the extra comment as pointed by Greg.

          Show
          vamsee Vamsee Yarlagadda added a comment - Removed the extra comment as pointed by Greg.
          Hide
          gchanan Gregory Chanan added a comment -

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

          This comment no longer seems relevant

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

            People

            • Assignee:
              markrmiller@gmail.com Mark Miller
              Reporter:
              vamsee Vamsee Yarlagadda
            • Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development