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

LRUQueryCache should rather not cache than wait on a lock

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 6.1, master (7.0)
    • Component/s: None
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      This is an idea Robert just mentioned to me: currently the cache is using a lock to keep various data-structures in sync. It is a pity that you might have contention because of caching. So something we could do would be to not cache when the lock is already taken.

      1. LUCENE-7237.patch
        15 kB
        Adrien Grand

        Activity

        Hide
        jpountz Adrien Grand added a comment -

        I gave it a try. The patch is large, but the change is quite simple, most line changes are due to indentation since I replaced synchronized methods with calls to Lock.(un)lock() in try/finally blocks. The interesting part is in CachingWeightWrapper#scorer where we now do:

              // Short-circuit: Check whether this segment is eligible for caching
              // before we take a lock because of #get
              if (shouldCache(context) == false) {
                return in.scorer(context);
              }
        
              // If the lock is already busy, prefer using the uncached version than waiting
              if (lock.tryLock() == false) {
                return in.scorer(context);
              }
        
              DocIdSet docIdSet;
              try {
                docIdSet = get(in.getQuery(), context);
              } finally {
                lock.unlock();
              }
        
              if (docIdSet == null) {
                if (policy.shouldCache(in.getQuery(), context)) {
                  docIdSet = cache(context);
                  putIfAbsent(in.getQuery(), context, docIdSet);
                } else {
                  return in.scorer(context);
                }
              }
        
              assert docIdSet != null;
              if (docIdSet == DocIdSet.EMPTY) {
                return null;
              }
              final DocIdSetIterator disi = docIdSet.iterator();
              if (disi == null) {
                return null;
              }
        
              return new ConstantScoreScorer(this, 0f, disi);
        

        Because of the tryLock() call, if the lock is already held because another thread is using the cache, we just return a scorer produced by the original weight rather than wait.

        Show
        jpountz Adrien Grand added a comment - I gave it a try. The patch is large, but the change is quite simple, most line changes are due to indentation since I replaced synchronized methods with calls to Lock.(un)lock() in try/finally blocks. The interesting part is in CachingWeightWrapper#scorer where we now do: // Short -circuit: Check whether this segment is eligible for caching // before we take a lock because of #get if (shouldCache(context) == false ) { return in.scorer(context); } // If the lock is already busy, prefer using the uncached version than waiting if (lock.tryLock() == false ) { return in.scorer(context); } DocIdSet docIdSet; try { docIdSet = get(in.getQuery(), context); } finally { lock.unlock(); } if (docIdSet == null ) { if (policy.shouldCache(in.getQuery(), context)) { docIdSet = cache(context); putIfAbsent(in.getQuery(), context, docIdSet); } else { return in.scorer(context); } } assert docIdSet != null ; if (docIdSet == DocIdSet.EMPTY) { return null ; } final DocIdSetIterator disi = docIdSet.iterator(); if (disi == null ) { return null ; } return new ConstantScoreScorer( this , 0f, disi); Because of the tryLock() call, if the lock is already held because another thread is using the cache, we just return a scorer produced by the original weight rather than wait.
        Hide
        jpountz Adrien Grand added a comment -

        For the record, there is already a test for concurrency in TestLRUQueryCache.

        Show
        jpountz Adrien Grand added a comment - For the record, there is already a test for concurrency in TestLRUQueryCache.
        Hide
        rcmuir Robert Muir added a comment -

        +1

        Show
        rcmuir Robert Muir added a comment - +1
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit a123224d2efb07490a235569570c8f7b8fbd7c8f in lucene-solr's branch refs/heads/branch_6x from Adrien Grand
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=a123224 ]

        LUCENE-7237: LRUQueryCache now prefers returning an uncached Scorer than waiting on a lock.

        Show
        jira-bot ASF subversion and git services added a comment - Commit a123224d2efb07490a235569570c8f7b8fbd7c8f in lucene-solr's branch refs/heads/branch_6x from Adrien Grand [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=a123224 ] LUCENE-7237 : LRUQueryCache now prefers returning an uncached Scorer than waiting on a lock.
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit bf232d7635e1686cd6f5624525fa3e0b7820430f in lucene-solr's branch refs/heads/master from Adrien Grand
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=bf232d7 ]

        LUCENE-7237: LRUQueryCache now prefers returning an uncached Scorer than waiting on a lock.

        Show
        jira-bot ASF subversion and git services added a comment - Commit bf232d7635e1686cd6f5624525fa3e0b7820430f in lucene-solr's branch refs/heads/master from Adrien Grand [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=bf232d7 ] LUCENE-7237 : LRUQueryCache now prefers returning an uncached Scorer than waiting on a lock.
        Hide
        hossman Hoss Man added a comment -

        Manually correcting fixVersion per Step #S5 of LUCENE-7271

        Show
        hossman Hoss Man added a comment - Manually correcting fixVersion per Step #S5 of LUCENE-7271

          People

          • Assignee:
            jpountz Adrien Grand
            Reporter:
            jpountz Adrien Grand
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development