Details

    • Type: Improvement
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 6.4, 7.0
    • Component/s: None
    • Security Level: Public (Default Security Level. Issues are Public)
    • Labels:
      None

      Description

      SOLR-7372 added a maxRamMB parameter to LRUCache to evict items based on memory usage. That helps with the query result cache but not with the filter cache which defaults to FastLRUCache. This issue intends to add the same feature to FastLRUCache.

      1. SOLR-9633.patch
        29 kB
        Shalin Shekhar Mangar
      2. SOLR-9633.patch
        30 kB
        Shalin Shekhar Mangar

        Activity

        Hide
        shalinmangar Shalin Shekhar Mangar added a comment -

        First cut. This is slightly different than the impl for LRUCache because it completely ignores sizes when maxRamMB is specified. (We should probably throw an exception in that case than ignoring). Also the eviction logic is not as optimized as the one for the size based policy.

        Show
        shalinmangar Shalin Shekhar Mangar added a comment - First cut. This is slightly different than the impl for LRUCache because it completely ignores sizes when maxRamMB is specified. (We should probably throw an exception in that case than ignoring). Also the eviction logic is not as optimized as the one for the size based policy.
        Hide
        shalinmangar Shalin Shekhar Mangar added a comment -
        Show
        shalinmangar Shalin Shekhar Mangar added a comment - FYI Yonik Seeley
        Hide
        shalinmangar Shalin Shekhar Mangar added a comment -

        Yonik Seeley - do you mind taking a look at this? Is the simple ram size based eviction logic fine or do we need to implement something fancier as we have for size based eviction?

        Show
        shalinmangar Shalin Shekhar Mangar added a comment - Yonik Seeley - do you mind taking a look at this? Is the simple ram size based eviction logic fine or do we need to implement something fancier as we have for size based eviction?
        Hide
        yseeley@gmail.com Yonik Seeley added a comment -

        Taking a look...

        Show
        yseeley@gmail.com Yonik Seeley added a comment - Taking a look...
        Hide
        yseeley@gmail.com Yonik Seeley added a comment -

        Can you explain what logic changes are in markAndSweepByCacheSize()? It's hard to tell looking at the diff (lots of indent changes... not sure about other changes).

        Show
        yseeley@gmail.com Yonik Seeley added a comment - Can you explain what logic changes are in markAndSweepByCacheSize()? It's hard to tell looking at the diff (lots of indent changes... not sure about other changes).
        Hide
        shalinmangar Shalin Shekhar Mangar added a comment -

        The size based eviction logic in markAndSweep method is moved to markAndSweepByCacheSize – there are no logic changes when only item size is used for eviction. But if maxRamMB is configured then we use the markAndSweepByRamSize method which evicts by making two passes over the cache and a sort. After these changes, the FastLRUCache behaves differently than LRUCache. Instead of supporting both size and maxRamMB and evicting based on both limits, the FastLRUCache supports either size or maxRamMB but not both. This is also because I couldn't figure out a way to evict by ram as efficiently as we do today by size.

        We could support both size and ram together by evicting by size first and then fall back to the expensive two pass eviction based on ram.

        Show
        shalinmangar Shalin Shekhar Mangar added a comment - The size based eviction logic in markAndSweep method is moved to markAndSweepByCacheSize – there are no logic changes when only item size is used for eviction. But if maxRamMB is configured then we use the markAndSweepByRamSize method which evicts by making two passes over the cache and a sort. After these changes, the FastLRUCache behaves differently than LRUCache. Instead of supporting both size and maxRamMB and evicting based on both limits, the FastLRUCache supports either size or maxRamMB but not both. This is also because I couldn't figure out a way to evict by ram as efficiently as we do today by size. We could support both size and ram together by evicting by size first and then fall back to the expensive two pass eviction based on ram.
        Hide
        yseeley@gmail.com Yonik Seeley added a comment -

        OK, thanks for the explanation. That's clearer after I apply the patch too.
        Things look good... I think the only minor change I'd make is to wrap the other update to ramBytes in the put() method with the "ramUpperWatermark != Long.MAX_VALUE" check as you did in other places.

        Show
        yseeley@gmail.com Yonik Seeley added a comment - OK, thanks for the explanation. That's clearer after I apply the patch too. Things look good... I think the only minor change I'd make is to wrap the other update to ramBytes in the put() method with the "ramUpperWatermark != Long.MAX_VALUE" check as you did in other places.
        Hide
        michael.sun Michael Sun added a comment -

        The patch looks good to me. One suggestion is to add comments that RAM limit feature only works well when the value in cache is an Accountable, such as filter cache whose value is a DocSet, which is an Accountable. In case user want to use FastLRUCache for other cache, user need to make sure the type of value is Accountable.

        Another minor general suggestion is to use PriorityQueue in markAndSweepByRamSize(), instead of Collection.sort(). It probably doesn't make big difference since most cache size is less than 1000 though.

        Show
        michael.sun Michael Sun added a comment - The patch looks good to me. One suggestion is to add comments that RAM limit feature only works well when the value in cache is an Accountable, such as filter cache whose value is a DocSet, which is an Accountable. In case user want to use FastLRUCache for other cache, user need to make sure the type of value is Accountable. Another minor general suggestion is to use PriorityQueue in markAndSweepByRamSize(), instead of Collection.sort(). It probably doesn't make big difference since most cache size is less than 1000 though.
        Hide
        shalinmangar Shalin Shekhar Mangar added a comment -

        Thanks Yonik and Michael for the reviews. This patch wrap all updates to ramBytes in the put() method with the check as requested. I'll commit shortly.

        Michael, to your comment on this working only with Accountable, the example configurations specify this parameter only for filterCache so I feel it is unlikely that users will try to use FastLRUCache with maxRamMB for something else. But I'll make sure to include this point in the reference guide when documenting this feature.

        Show
        shalinmangar Shalin Shekhar Mangar added a comment - Thanks Yonik and Michael for the reviews. This patch wrap all updates to ramBytes in the put() method with the check as requested. I'll commit shortly. Michael, to your comment on this working only with Accountable, the example configurations specify this parameter only for filterCache so I feel it is unlikely that users will try to use FastLRUCache with maxRamMB for something else. But I'll make sure to include this point in the reference guide when documenting this feature.
        Hide
        shalinmangar Shalin Shekhar Mangar added a comment -

        I put SOLR-9366 instead of SOLR-9633 in the commit message by mistake. The commit messages are:

        Commit 487b0976eb3e98b78ab492f4969a2aa0373b626f in lucene-solr's branch refs/heads/master from Shalin Shekhar Mangar
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=487b097 ]
        
        SOLR-9366: Limit memory consumed by FastLRUCache with a new 'maxRamMB' config parameter
        
        Commit 53dad4f0d1c8f563810626ef9ab470fb3e5d1da9 in lucene-solr's branch refs/heads/branch_6x from Shalin Shekhar Mangar
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=53dad4f ]
        
        SOLR-9366: Limit memory consumed by FastLRUCache with a new 'maxRamMB' config parameter
        (cherry picked from commit 487b097)
        
        Show
        shalinmangar Shalin Shekhar Mangar added a comment - I put SOLR-9366 instead of SOLR-9633 in the commit message by mistake. The commit messages are: Commit 487b0976eb3e98b78ab492f4969a2aa0373b626f in lucene-solr's branch refs/heads/master from Shalin Shekhar Mangar [ https: //git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=487b097 ] SOLR-9366: Limit memory consumed by FastLRUCache with a new 'maxRamMB' config parameter Commit 53dad4f0d1c8f563810626ef9ab470fb3e5d1da9 in lucene-solr's branch refs/heads/branch_6x from Shalin Shekhar Mangar [ https: //git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=53dad4f ] SOLR-9366: Limit memory consumed by FastLRUCache with a new 'maxRamMB' config parameter (cherry picked from commit 487b097)
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit b57a5e41f82418621d8ac7e42edf0cd08dcee6be in lucene-solr's branch refs/heads/master from Shalin Shekhar Mangar
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=b57a5e4 ]

        SOLR-9633: Fix issue number in CHANGES.txt

        Show
        jira-bot ASF subversion and git services added a comment - Commit b57a5e41f82418621d8ac7e42edf0cd08dcee6be in lucene-solr's branch refs/heads/master from Shalin Shekhar Mangar [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=b57a5e4 ] SOLR-9633 : Fix issue number in CHANGES.txt
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit d3420c31a92180653b767850e82f55ec3d20fda2 in lucene-solr's branch refs/heads/branch_6x from Shalin Shekhar Mangar
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=d3420c3 ]

        SOLR-9633: Fix issue number in CHANGES.txt

        (cherry picked from commit b57a5e4)

        Show
        jira-bot ASF subversion and git services added a comment - Commit d3420c31a92180653b767850e82f55ec3d20fda2 in lucene-solr's branch refs/heads/branch_6x from Shalin Shekhar Mangar [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=d3420c3 ] SOLR-9633 : Fix issue number in CHANGES.txt (cherry picked from commit b57a5e4)
        Hide
        shalinmangar Shalin Shekhar Mangar added a comment -

        Thanks Yonik and Michael!

        Show
        shalinmangar Shalin Shekhar Mangar added a comment - Thanks Yonik and Michael!

          People

          • Assignee:
            shalinmangar Shalin Shekhar Mangar
            Reporter:
            shalinmangar Shalin Shekhar Mangar
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development