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

Avoid taking the lock in LRUQueryCache when not necessary

    Details

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

      Description

      LRUQueryCache's CachingWeightWrapper works this way:

      • first it looks up the cache to see if there is an entry for the query in the current leaf
      • if yes, it returns it
      • otherwise it checks whether the query should be cached on this leaf
      • if yes, it builds a cache entry and returns it
      • otherwise it returns a scorer built from the wrapped weight

      The potential issue is that this first step always takes the lock, and I have seen a couple cases where indices were small and/or queries were very cheap and this showed up as a bottleneck. On the other hand, we have checks in step 3 that tell the cache to not cache on a particular segment regardless of the query. So I would like to move that part before 1 so that we do not even take the lock in that case.

      For instance right now we require that segments have at least 10k documents and 3% of all docs in the index to be cached. I just looked at a random index that contains 1.7m documents, and only 4 segments out of 29 met this criterion (yet they contain 1.1m documents: 65% of the total index size). So in the case of that index, we would take the lock 7x less often.

      1. LUCENE-7235.patch
        22 kB
        Adrien Grand

        Activity

        Hide
        jpountz Adrien Grand added a comment -

        Here is a patch. It shifts the responsibility to check whether a segment is eligible for caching (regardless of the query) from the caching policy to the cache itself.

        Show
        jpountz Adrien Grand added a comment - Here is a patch. It shifts the responsibility to check whether a segment is eligible for caching (regardless of the query) from the caching policy to the cache itself.
        Hide
        rcmuir Robert Muir added a comment -

        +1.

        I think we should also consider explicitly disabling the cache in tiny cases like MemoryIndex?

        Show
        rcmuir Robert Muir added a comment - +1. I think we should also consider explicitly disabling the cache in tiny cases like MemoryIndex?
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 7639abcd161b1feca01de28b56bec067b750891e 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=7639abc ]

        LUCENE-7235: Avoid taking the lock in LRUQueryCache when not necessary.

        Show
        jira-bot ASF subversion and git services added a comment - Commit 7639abcd161b1feca01de28b56bec067b750891e 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=7639abc ] LUCENE-7235 : Avoid taking the lock in LRUQueryCache when not necessary.
        Hide
        jira-bot ASF subversion and git services added a comment -

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

        LUCENE-7235: Avoid taking the lock in LRUQueryCache when not necessary.

        Show
        jira-bot ASF subversion and git services added a comment - Commit 81446cf34531d46f224beaf6c2bc70bdf53ee585 in lucene-solr's branch refs/heads/master from Adrien Grand [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=81446cf ] LUCENE-7235 : Avoid taking the lock in LRUQueryCache when not necessary.
        Hide
        jpountz Adrien Grand added a comment -

        I think we should also consider explicitly disabling the cache in tiny cases like MemoryIndex?

        I did it in LUCENE-7238.

        Show
        jpountz Adrien Grand added a comment - I think we should also consider explicitly disabling the cache in tiny cases like MemoryIndex? I did it in LUCENE-7238 .
        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:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development