Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Core: filter cache heap usage should include RAM used by the cache keys (Filter) #8268

Closed
mikemccand opened this Issue Oct 29, 2014 · 12 comments

Comments

Projects
None yet
6 participants
@mikemccand
Copy link
Contributor

commented Oct 29, 2014

I looked at the heap dump from #8249 and it looks like the majority of the heap is consumed by filter cache keys (TermFilter in that case, where each term was quite large: ~200 bytes long). There were ~5.5 M such entries, I suspect most of which matching 0 docs.

We don't track cache key RAM usage today in Elasticsearch, because it's tricky. E.g. Lucene's Filter doesn't implement Accountable, so we can't (easily) ask it how much RAM it's using. In general there can be sharing between Filters so the RAM used across N filters is less than the sum of each, though I'm not sure Elasticsearch does this. Also, ideally we'd avoid over-counting when the same Filter is cached across N segments.

It's tricky, but I think we need to do something here...

Maybe a separate improvement we could make here is to not bother caching a filter that matched 0 docs? Such filters are usually (?) fast to re-execute...

@bleskes

This comment has been minimized.

Copy link
Member

commented Oct 29, 2014

Maybe a separate improvement we could make here is to not bother caching a filter that matched 0 docs? Such filters are usually (?) fast to re-execute...

That depends on how much effort it takes to come to that conclusion. Say a geo filter with an arc distance, or cached boolean clause etc. Maybe we can make it filter dependent like a term filter with 0 hits is not cached but others like Range Terms might still be worth it? I'm not sure we need this complexity...

@clintongormley

This comment has been minimized.

Copy link
Member

commented Oct 29, 2014

What about just limiting the number of filters that can be added to the cache?

@jpountz

This comment has been minimized.

Copy link
Contributor

commented Oct 29, 2014

Agreed with Clinton. I think part of the bug is that a filter cache should never reach 5.5M entries: it only makes sense to cache filters if they are going to be reused, otherwise the caching logic is going to add overhead by consuming all documents (as opposed to using skipping in the case of a conjunction) and increasing memory pressure by promoting object to the old gen.

So maybe the fix is to allow to configure an absolute maximum size on the filter cache (with a reasonable value) and to cache filters less aggressively?

@mikemccand

This comment has been minimized.

Copy link
Contributor Author

commented Oct 29, 2014

+1 for a simple size limit. I think Solr (example solrconfig.xml) "defaults" to 512...

@clintongormley

This comment has been minimized.

Copy link
Member

commented Oct 30, 2014

512 seems awful low to me. Is this number per-segment? So if you have one filter and 20 segments, you'd have 20 entries? Remember also that we have multiple shards per node.

I think we need to choose a big number, which still allows for a lot of caching, without letting it grow to a ridiculous number like 5 million. Perhaps 50,000?

@mikemccand

This comment has been minimized.

Copy link
Contributor Author

commented Oct 30, 2014

I think for Solr it's per-index, i.e. after 512 cached filters for the index it starts evicting by default.

I agree this may be low, since we now compactly store the sparse cases ... but still caching is "supposed" to be for cases where you expect high re-use of the given filter and the cost to re-generate it is highish.

In general I think Elasticsearch should be less aggressive about filter caching; I suspect in many cases where we are caching, we are not saving that much time vs. letting the OS take that RAM instead and cache the IO pages Lucene will access to recreate that filter. Running a TermFilter when the IO pages are hot should be quite fast.

Anyway 50K seems OK...

@clintongormley

This comment has been minimized.

Copy link
Member

commented Oct 30, 2014

@mikemccand take this example:

Users are only interested in posts from people that they follow (or even 2 degrees - the people followed by the people that they follow). eg there are 100,000 user IDs to filter on.

These can all be put into a terms query, potentially with a short custom cache key. The first execution takes a bit of time, but the filter remains valid for the rest of the user's session. This represents a huge saving in execution time. It is quite feasible that a big website could have 100k user sessions live at the same time.

@rmuir

This comment has been minimized.

Copy link
Contributor

commented Oct 30, 2014

I think 512 is actually overly large. way over the top

IMO: we should only cache filters that:

  • are slow (stuff like wildcards and ranges, but not individual terms)
  • clear evidence of reuse (e.g. not the first time we have seen the filter)
  • being intersected with dense queries (where the risk-reward tradeoff is more clear)

Retrieving every single matching document for a filter to put it into a bitset is extremely slow. Most times, its better to just intersect it on the fly. So this is a huge risk, and we should only do it when the reward is clear: and the reward is tiny unless the criteria above are being met.

I'm not interested in seeing flawed benchmarks around this within current elasticsearch, because it bogusly does this slow way pretty much all the time, even when not caching. So of course mechanisms like caching and bulk bitset intersection will always look like a huge win with the current code.

dakrone added a commit to dakrone/elasticsearch that referenced this issue Oct 31, 2014

Use a 1024 byte minimum weight for filter cache entries
This changes the weighing function for the filter cache to use a
configurable minimum weight for each filter cached. This value defaults
to 1kb and can be configured with the
`indices.cache.filter.minimum_entry_weight` setting.

This also fixes an issue with the filter cache where the concurrency
level of the cache was exposed as a setting, but not used in cache
construction.

Relates to elastic#8268

dakrone added a commit that referenced this issue Oct 31, 2014

Use a 1024 byte minimum weight for filter cache entries
This changes the weighing function for the filter cache to use a
configurable minimum weight for each filter cached. This value defaults
to 1kb and can be configured with the
`indices.cache.filter.minimum_entry_weight` setting.

This also fixes an issue with the filter cache where the concurrency
level of the cache was exposed as a setting, but not used in cache
construction.

Relates to #8268

dakrone added a commit that referenced this issue Oct 31, 2014

Use a 1024 byte minimum weight for filter cache entries
This changes the weighing function for the filter cache to use a
configurable minimum weight for each filter cached. This value defaults
to 1kb and can be configured with the
`indices.cache.filter.minimum_entry_weight` setting.

This also fixes an issue with the filter cache where the concurrency
level of the cache was exposed as a setting, but not used in cache
construction.

Relates to #8268

dakrone added a commit that referenced this issue Oct 31, 2014

Use a 1024 byte minimum weight for filter cache entries
This changes the weighing function for the filter cache to use a
configurable minimum weight for each filter cached. This value defaults
to 1kb and can be configured with the
`indices.cache.filter.minimum_entry_weight` setting.

This also fixes an issue with the filter cache where the concurrency
level of the cache was exposed as a setting, but not used in cache
construction.

Relates to #8268

Conflicts:
	src/main/java/org/elasticsearch/indices/cache/filter/IndicesFilterCache.java

@s1monw s1monw added v1.6.0 and removed v1.5.0 labels Mar 17, 2015

@s1monw

This comment has been minimized.

Copy link
Contributor

commented May 28, 2015

@mikemccand what's the status of this issue?

@rmuir

This comment has been minimized.

Copy link
Contributor

commented May 28, 2015

This is bounded to 100k currently in master. I don't much like this number, its ridiculously large, but other changes address the real problems (overcaching). For example only caching on large segments and only when reuse has been noted and so on.

@jpountz

This comment has been minimized.

Copy link
Contributor

commented May 28, 2015

I don't think this issue is relevant anymore now that we are on the lucene query cache:

  • keys are taken into account (the result of ramBytesUsed() if the query implements Accountable and a constant otherwise)
  • the cache has a limit on the number of filters that can be added to the cache to limit issues that would be caused by ram usage of keys being underestimated
  • also we require that segments have at least 10000 docs for caching so it should be less likely to have cache keys that are much larger than the values than before

@jpountz jpountz closed this May 28, 2015

@jpountz

This comment has been minimized.

Copy link
Contributor

commented May 28, 2015

Robert, you are right it's 100k currently, nor 10k. I agree we should reduce this number to a more reasonable value.

mute pushed a commit to mute/elasticsearch that referenced this issue Jul 29, 2015

Use a 1024 byte minimum weight for filter cache entries
This changes the weighing function for the filter cache to use a
configurable minimum weight for each filter cached. This value defaults
to 1kb and can be configured with the
`indices.cache.filter.minimum_entry_weight` setting.

This also fixes an issue with the filter cache where the concurrency
level of the cache was exposed as a setting, but not used in cache
construction.

Relates to elastic#8268

mute pushed a commit to mute/elasticsearch that referenced this issue Jul 29, 2015

Use a 1024 byte minimum weight for filter cache entries
This changes the weighing function for the filter cache to use a
configurable minimum weight for each filter cached. This value defaults
to 1kb and can be configured with the
`indices.cache.filter.minimum_entry_weight` setting.

This also fixes an issue with the filter cache where the concurrency
level of the cache was exposed as a setting, but not used in cache
construction.

Relates to elastic#8268

Conflicts:
	src/main/java/org/elasticsearch/indices/cache/filter/IndicesFilterCache.java
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.