Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 5.2, 6.0
    • Component/s: None
    • Labels:
      None

      Description

      Now that SOLR-7371 has made DocSet impls Accountable, we should add an option to LRUCache to limit itself by RAM.

      I propose to add a 'maxRamBytes' configuration parameter which it can use to evict items once the total RAM usage of the cache reaches this limit.

      1. SOLR-7372.patch
        0.8 kB
        Noble Paul
      2. SOLR-7372.patch
        17 kB
        Shalin Shekhar Mangar
      3. SOLR-7372.patch
        13 kB
        Shalin Shekhar Mangar
      4. SOLR-7372.patch
        12 kB
        Shalin Shekhar Mangar
      5. SOLR-7372.patch
        8 kB
        Shalin Shekhar Mangar

        Issue Links

          Activity

          Hide
          Shalin Shekhar Mangar added a comment -

          Simple patch:

          1. LRUCache implements Accountable
          2. The values are used to calculate the ram usage
          3. Added a simple test

          This makes it possible to limit the heap usage of the query result cache and the filter cache. This won't work for the document cache because the Document class isn't accountable.

          TODO:

          1. Take the keys's ram usage into account. For example, TermsQuery can be big.
          2. Have a humanized format for the maxRamBytes e.g. maxRamBytes=512M should be possible.
          Show
          Shalin Shekhar Mangar added a comment - Simple patch: LRUCache implements Accountable The values are used to calculate the ram usage Added a simple test This makes it possible to limit the heap usage of the query result cache and the filter cache. This won't work for the document cache because the Document class isn't accountable. TODO: Take the keys's ram usage into account. For example, TermsQuery can be big. Have a humanized format for the maxRamBytes e.g. maxRamBytes=512M should be possible.
          Hide
          Erick Erickson added a comment -

          I like this a lot. even when people have correctly sized the cache at first, they can add docs and have problems. I can imagine someone wanting to monitor when this limit is tripped rather than size, is that possible via JMX?

          Not asking you to add that, just wondering if it'd be easy some time in the future.

          Show
          Erick Erickson added a comment - I like this a lot. even when people have correctly sized the cache at first, they can add docs and have problems. I can imagine someone wanting to monitor when this limit is tripped rather than size, is that possible via JMX? Not asking you to add that, just wondering if it'd be easy some time in the future.
          Hide
          Shalin Shekhar Mangar added a comment -

          I like this a lot. even when people have correctly sized the cache at first, they can add docs and have problems. I can imagine someone wanting to monitor when this limit is tripped rather than size, is that possible via JMX?

          Yeah, good point. I can easily add another counter to track eviction due to RAM usage.

          Show
          Shalin Shekhar Mangar added a comment - I like this a lot. even when people have correctly sized the cache at first, they can add docs and have problems. I can imagine someone wanting to monitor when this limit is tripped rather than size, is that possible via JMX? Yeah, good point. I can easily add another counter to track eviction due to RAM usage.
          Hide
          Erick Erickson added a comment -

          Cool!

          Show
          Erick Erickson added a comment - Cool!
          Hide
          Yonik Seeley added a comment -

          We only need to throw an exception if not Accountable for items being put in the cache right?
          Old items that are bring removed (or overwritten) must be Accountable since they got added somehow.

          Also, it's going to be relatively easy to blow this out of the water on purpose, or even by accident.

          1) Do facet.method=enum on a high cardinality field like "ID" and thus put a million small items in the cache.
          2) start searching normally and the cache size will stay at a million, regardless of the size of items we put in (since removeEldestEntry is only called once for each put).

          After a quick browse of LinkedHashMap, I didn't see an obvious easy/fast way to remove the oldest entry, so I'm not sure how to fix.

          For the calculation of amount of RAM taken up... perhaps we should estimate the minimum that a key + internal map node would take up?
          For the query cache in particular, it's going to be common for query keys to take up more memory than the actual DocSlice.

          Show
          Yonik Seeley added a comment - We only need to throw an exception if not Accountable for items being put in the cache right? Old items that are bring removed (or overwritten) must be Accountable since they got added somehow. Also, it's going to be relatively easy to blow this out of the water on purpose, or even by accident. 1) Do facet.method=enum on a high cardinality field like "ID" and thus put a million small items in the cache. 2) start searching normally and the cache size will stay at a million, regardless of the size of items we put in (since removeEldestEntry is only called once for each put). After a quick browse of LinkedHashMap, I didn't see an obvious easy/fast way to remove the oldest entry, so I'm not sure how to fix. For the calculation of amount of RAM taken up... perhaps we should estimate the minimum that a key + internal map node would take up? For the query cache in particular, it's going to be common for query keys to take up more memory than the actual DocSlice.
          Hide
          Shalin Shekhar Mangar added a comment -

          Also, it's going to be relatively easy to blow this out of the water on purpose, or even by accident.
          1) Do facet.method=enum on a high cardinality field like "ID" and thus put a million small items in the cache.
          2) start searching normally and the cache size will stay at a million, regardless of the size of items we put in (since removeEldestEntry is only called once for each put).

          I don't understand. We evict items on each put when:

          if (size() > limit || (maxRamBytes != Long.MAX_VALUE && ramBytesUsed.get() > maxRamBytes)) {
          ...
          }
          

          I indexed 2.4M docs with a UUID 'id' and searched with:

          q=*:*&facet=on&facet.field=id&facet.method=enum&facet.limit=1000

          The filterCache stats are:

          class:org.apache.solr.search.LRUCache
          version:1.0
          description:LRU Cache(maxSize=512, initialSize=512)
          src:null
          stats:
          lookups:1103
          hits:102
          hitratio:0.09
          inserts:1001
          evictions:489
          size:512
          maxRamBytes:1048576
          ramBytesUsed:18528
          warmupTime:0
          cumulative_lookups:1103
          cumulative_hits:102
          cumulative_hitratio:0.09
          cumulative_inserts:1001
          cumulative_evictions:489
          

          Tried with facet.limit=1000000 too:

          class:org.apache.solr.search.LRUCache
          version:1.0
          description:LRU Cache(maxSize=512, initialSize=512)
          src:null
          stats:
          lookups:1001001
          hits:0
          hitratio:0
          inserts:1001002
          evictions:1000490
          size:512
          maxRamBytes:1048576
          ramBytesUsed:18528
          warmupTime:0
          cumulative_lookups:1001001
          cumulative_hits:0
          cumulative_hitratio:0
          cumulative_inserts:1001002
          cumulative_evictions:1000490
          
          Show
          Shalin Shekhar Mangar added a comment - Also, it's going to be relatively easy to blow this out of the water on purpose, or even by accident. 1) Do facet.method=enum on a high cardinality field like "ID" and thus put a million small items in the cache. 2) start searching normally and the cache size will stay at a million, regardless of the size of items we put in (since removeEldestEntry is only called once for each put). I don't understand. We evict items on each put when: if (size() > limit || (maxRamBytes != Long .MAX_VALUE && ramBytesUsed.get() > maxRamBytes)) { ... } I indexed 2.4M docs with a UUID 'id' and searched with: q=*:*&facet=on&facet.field=id&facet.method= enum &facet.limit=1000 The filterCache stats are: class:org.apache.solr.search.LRUCache version:1.0 description:LRU Cache(maxSize=512, initialSize=512) src: null stats: lookups:1103 hits:102 hitratio:0.09 inserts:1001 evictions:489 size:512 maxRamBytes:1048576 ramBytesUsed:18528 warmupTime:0 cumulative_lookups:1103 cumulative_hits:102 cumulative_hitratio:0.09 cumulative_inserts:1001 cumulative_evictions:489 Tried with facet.limit=1000000 too: class:org.apache.solr.search.LRUCache version:1.0 description:LRU Cache(maxSize=512, initialSize=512) src: null stats: lookups:1001001 hits:0 hitratio:0 inserts:1001002 evictions:1000490 size:512 maxRamBytes:1048576 ramBytesUsed:18528 warmupTime:0 cumulative_lookups:1001001 cumulative_hits:0 cumulative_hitratio:0 cumulative_inserts:1001002 cumulative_evictions:1000490
          Hide
          Shalin Shekhar Mangar added a comment -

          Okay, I understand what you were saying. We just evict one (oldest) entry per put operation but that is not sufficient to guarantee that the bytes occupied by the cache is less than the configured limit. We somehow need to keep removing the least recently used items until the ram size drops below the limit.

          Show
          Shalin Shekhar Mangar added a comment - Okay, I understand what you were saying. We just evict one (oldest) entry per put operation but that is not sufficient to guarantee that the bytes occupied by the cache is less than the configured limit. We somehow need to keep removing the least recently used items until the ram size drops below the limit.
          Hide
          Shalin Shekhar Mangar added a comment -

          Here's a patch which takes care of all of Yonik's comments:

          1. Removed the Accountable instanceof check for values in removeEldestEntry and the old items during put
          2. We use the LinkedHashMap.getEntrySet().iterator() to remove least-recently-used items until the ram usage drops below the limit. The removeEldestEntry javadocs say the following:
            • <p>This method typically does not modify the map in any way,
            • instead allowing the map to modify itself as directed by its
            • return value. It <i>is</i> permitted for this method to modify
            • the map directly, but if it does so, it <i>must</i> return
            • <tt>false</tt> (indicating that the map should not attempt any
            • further modification). The effects of returning <tt>true</tt>
            • after modifying the map from within this method are unspecified.
          3. An estimate of the LinkedHashMap.Entry and key is also used for the calculation.
          Show
          Shalin Shekhar Mangar added a comment - Here's a patch which takes care of all of Yonik's comments: Removed the Accountable instanceof check for values in removeEldestEntry and the old items during put We use the LinkedHashMap.getEntrySet().iterator() to remove least-recently-used items until the ram usage drops below the limit. The removeEldestEntry javadocs say the following: <p>This method typically does not modify the map in any way, instead allowing the map to modify itself as directed by its return value. It <i>is</i> permitted for this method to modify the map directly, but if it does so, it <i>must</i> return <tt>false</tt> (indicating that the map should not attempt any further modification). The effects of returning <tt>true</tt> after modifying the map from within this method are unspecified. An estimate of the LinkedHashMap.Entry and key is also used for the calculation.
          Hide
          Yonik Seeley added a comment -

          Ah, I had assumed that iterator() was most recent to least recent, but it looks like it's the reverse.
          Patch looks good!

          ramBytesUsed doesn't really need to be AtomicLong... it's only modified in synchronized contexts. Then ramBytesUsed() could just synchronize also... it shouldn't matter given how infrequently that will be called (just to show stats in the admin, right?). That's just a minor nit that doesn't really matter though - I imagine you'd be hard pressed to actually see any difference in performance.

          Show
          Yonik Seeley added a comment - Ah, I had assumed that iterator() was most recent to least recent, but it looks like it's the reverse. Patch looks good! ramBytesUsed doesn't really need to be AtomicLong... it's only modified in synchronized contexts. Then ramBytesUsed() could just synchronize also... it shouldn't matter given how infrequently that will be called (just to show stats in the admin, right?). That's just a minor nit that doesn't really matter though - I imagine you'd be hard pressed to actually see any difference in performance.
          Hide
          Shalin Shekhar Mangar added a comment -
          1. Changed AtomicLong to a simple long because it is always read/written inside synchronized(map)
          2. Changed ramBytesUsed() to also use the same synchronization
          3. Renamed 'evictionsDueToRamBytes' to 'evictionsRamUsage' – this counter will track number of evictions that happen because we exceeded the maxRamBytes - please note Erick Erickson.

          I'll commit this shortly.

          Show
          Shalin Shekhar Mangar added a comment - Changed AtomicLong to a simple long because it is always read/written inside synchronized(map) Changed ramBytesUsed() to also use the same synchronization Renamed 'evictionsDueToRamBytes' to 'evictionsRamUsage' – this counter will track number of evictions that happen because we exceeded the maxRamBytes - please note Erick Erickson . I'll commit this shortly.
          Hide
          Shalin Shekhar Mangar added a comment -
          1. I renamed the maxRamBytes parameter to maxRamMB so that people don't have to convert size in megabytes and gigabytes from bytes.
          2. I also added the description of maxRamMB parameter for LRUCache in solrconfig.xml (used only for query result cache).

          I think this is ready.

          Show
          Shalin Shekhar Mangar added a comment - I renamed the maxRamBytes parameter to maxRamMB so that people don't have to convert size in megabytes and gigabytes from bytes. I also added the description of maxRamMB parameter for LRUCache in solrconfig.xml (used only for query result cache). I think this is ready.
          Hide
          ASF subversion and git services added a comment -

          Commit 1672811 from shalin@apache.org in branch 'dev/trunk'
          [ https://svn.apache.org/r1672811 ]

          SOLR-7372: Limit memory consumed by LRUCache with a new 'maxRamMB' config parameter

          Show
          ASF subversion and git services added a comment - Commit 1672811 from shalin@apache.org in branch 'dev/trunk' [ https://svn.apache.org/r1672811 ] SOLR-7372 : Limit memory consumed by LRUCache with a new 'maxRamMB' config parameter
          Hide
          ASF subversion and git services added a comment -

          Commit 1672812 from shalin@apache.org in branch 'dev/branches/branch_5x'
          [ https://svn.apache.org/r1672812 ]

          SOLR-7372: Limit memory consumed by LRUCache with a new 'maxRamMB' config parameter

          Show
          ASF subversion and git services added a comment - Commit 1672812 from shalin@apache.org in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1672812 ] SOLR-7372 : Limit memory consumed by LRUCache with a new 'maxRamMB' config parameter
          Hide
          Shalin Shekhar Mangar added a comment -

          Thanks for the review, Yonik!

          Show
          Shalin Shekhar Mangar added a comment - Thanks for the review, Yonik!
          Hide
          Shalin Shekhar Mangar added a comment -

          Re-opening due to Java7 incompatibility on 5x

          Show
          Shalin Shekhar Mangar added a comment - Re-opening due to Java7 incompatibility on 5x
          Hide
          ASF subversion and git services added a comment -

          Commit 1672815 from shalin@apache.org in branch 'dev/branches/branch_5x'
          [ https://svn.apache.org/r1672815 ]

          SOLR-7372: Fix Java7 compatibility

          Show
          ASF subversion and git services added a comment - Commit 1672815 from shalin@apache.org in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1672815 ] SOLR-7372 : Fix Java7 compatibility
          Hide
          Erick Erickson added a comment -

          Cool, thanks!

          Show
          Erick Erickson added a comment - Cool, thanks!
          Hide
          Noble Paul added a comment -

          make these new fields editable

          Show
          Noble Paul added a comment - make these new fields editable
          Hide
          Shalin Shekhar Mangar added a comment -

          Thanks Noble Paul for pointing out that this needs to be added to ConfigOverlay. How do I specify this config parameter without a default value?

          Show
          Shalin Shekhar Mangar added a comment - Thanks Noble Paul for pointing out that this needs to be added to ConfigOverlay. How do I specify this config parameter without a default value?
          Hide
          Noble Paul added a comment -

          That is not the default value. It is the type information in this case it is "20" means an XML attribute with an integer type

          Show
          Noble Paul added a comment - That is not the default value. It is the type information in this case it is "20" means an XML attribute with an integer type
          Hide
          Shalin Shekhar Mangar added a comment -

          Ah, okay. Thanks. I'll commit your patch.

          Show
          Shalin Shekhar Mangar added a comment - Ah, okay. Thanks. I'll commit your patch.
          Hide
          ASF subversion and git services added a comment -

          Commit 1673358 from shalin@apache.org in branch 'dev/trunk'
          [ https://svn.apache.org/r1673358 ]

          SOLR-7372: Enable maxRamMB to be configured via the Config APIs on filterCache and queryResultCache

          Show
          ASF subversion and git services added a comment - Commit 1673358 from shalin@apache.org in branch 'dev/trunk' [ https://svn.apache.org/r1673358 ] SOLR-7372 : Enable maxRamMB to be configured via the Config APIs on filterCache and queryResultCache
          Hide
          ASF subversion and git services added a comment -

          Commit 1673359 from shalin@apache.org in branch 'dev/branches/branch_5x'
          [ https://svn.apache.org/r1673359 ]

          SOLR-7372: Enable maxRamMB to be configured via the Config APIs on filterCache and queryResultCache

          Show
          ASF subversion and git services added a comment - Commit 1673359 from shalin@apache.org in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1673359 ] SOLR-7372 : Enable maxRamMB to be configured via the Config APIs on filterCache and queryResultCache
          Hide
          Shalin Shekhar Mangar added a comment -

          This was released in 5.2

          Show
          Shalin Shekhar Mangar added a comment - This was released in 5.2

            People

            • Assignee:
              Shalin Shekhar Mangar
              Reporter:
              Shalin Shekhar Mangar
            • Votes:
              1 Vote for this issue
              Watchers:
              7 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development