Cache v3

[vmg]

Description

As discussed with the team on the Vitess open hour, this PR upgrades our cache implementation with a new one based on theine-go. Here’s some of the things I’ve considered when making this decision:

• Theine looks much more similar to Caffeine, and it has been reviewed by Ben Manes, which gives me confidence in it.
  • The implemented algorithms have been ported much more verbatim from Caffeine, which makes looking at the original code as a reference an option.
• The hit rate on synthetic benchmark traces is significantly improved compared to caffeine, see https://github.com/Yiling-J/go-cache-benchmark-plus
  • These benchmarks are not representative of query planning – I intend to gather some production traces of query plans to see how we behave with the new implementation
  • The poor hitrate performance of Ristretto is probably caused by some of the unfixed bugs which we’ve discussed during the call
• The raw performance (read/write) of the caches is comparable, with Theine being slightly slower at reads and slightly faster at writes.
• Theine is much more actively maintained

As you’re probably aware of, Vitess have some very peculiar behaviors when it comes to caching, as our caches are used for storing query plans and not the results of the queries themselves. As part of integrating Theine into our project, I removed some unused functionality (most notably: support for hybrid NVME caches which we’re never going to use and added a lot of complexity to the code), and implemented the following improvements:

Optimized 256-bit hashing

One of the many shortcomings of our existing cache implementation is that its keys can only be string. This is a limiting behavior, particularly because internally the cache doesn’t do full string comparison when caching elements: entries in the cache are keyed as a 128-bit hash of their contents. This implementation detail was detailed on the RFC for the current cache implementation and deemed acceptable, because the chances of colliding a 128-bit value are astronomically low. But despite the peculiar internal optimization, the fact that the input keys for the cache are strings resulted in an inefficient API for our plan caches.

Since plan cache keys in the vtgate can be arbitrarily large (as they contain information about the targets the query is planned against) and we don’t want to carry around memory strings that large, we’ve been generating their keys as a SHA256 of their contents. But because the actual key to the cache must be a string, the resulting 32 bytes of hash are then hex-encoded into string form. The hex string is then hashed again inside the library to generate the 128 bit key that is authoritatively used for caching.

This is a very inefficient process that is performed for all queries, whether they hit or miss on the cache. So for this new cache implementation, I’ve provided an API which is both more ergonomic and more efficient. Since our new cache API is generic, we can define custom key types. I’ve implemented several specialized ones, including a 256-bit hash key type which knows how to hash itself into the cache’s shards by extracting chunks of its own hash, and skipping the double-hashing that was happening in our previous implementation.

New 256 bit hashing package

To integrate efficient 256-bit hashing into the cache, I’ve expanded the vthash package, which already carries a 128-bit hash function which we use for Collations, to support 256 bit hashes based on highwayhash. Highway is a stable hash that performs very well in practice (one order of magnitude faster than SHA256), and I’ve extended it with a non-opaque implementation that inlines much better than crypto hashes in the standard library.

Since Highwayhash is a multi-size function that also supports 128bit output, I intend to update the vthash package in a further PR to replace our existing 128-bit hash altogether with it and drop a dependency.

Epoch-based expiration

Production-ready cache implementations do not have an “evict all” API for the same reason that commercial airplanes do not have a “roll down the windows” button. Our previous cache implementation did not have support for evicting all the data, but I had to implement it because of the way the query caches in both vtgate and vttablet work: when a new version of the underlying VSchema changes, we need to invalidate all the queries in the cache.

The full eviction implementation in our previous cache wasn’t great: it’s very hard to do atomic, full-eviction of all the data in a cache (particularly when it’s a highly concurrent one), and besides a performance cost, it had some poor behavior in practice: in large caches it would cause a stall on the whole caching system until the cache was fully evicted. That’s not great!

This new cache implementation also didn’t have support for evicting the whole cache at once, but now I spent some more time thinking about how to do an implementation without the previous shortcomings. Here’s what I came up with: the new cache has support for epoch-based expiration of entries. This means that all the entries in the cache are versioned to a current epoch. The epoch represents the current version of the VSchema. When reading from the cache, you pass in an epoch value, and any cached values from previous epochs are ignored and incrementally expired.

This has several very interesting properties:

• The cache is never fully flushed, so there is no stall, and all the internal state of the cache (e.g. the policy filters) never become cold.
• Particularly hot queries are never dropped from the cache: they’re updated in-place with their new plan for the new schema, and the new epoch. They don’t have to go through the doorkeeper again, so the thundering herd every time we change the schema and flush the caches is greatly reduced.
• Old data from the cache magically disappears for read queries atomically, and from the cache itself incrementally.
• We can control the versioning of all the data on the cache externally

…which leads us to our next improvement.

Atomic schema tracking in vttablet

The current implementation of the QueryEngine in vttablet is actually serial. All the queries that come into the tablet contend on a single Mutex, which is grabbed before we start planning the query into a TabletPlan and released after the planning is complete. With our new cache implementation, we can remove this single point of contention by adding an Epoch counter to the engine. The current applied schema is stored in an atomic structure called currentSchema, which contains the schema’s tables and the current epoch. When queries come in, we atomically load the schema, and we use the current epoch when accessing the cache, and the current set of tables when planning new queries that will be inserted into the cache.

Settings/plan caching in vttablet

As part of refactoring the serial contention in the tablets, I noticed that connection settings were being cached on the same cache as tablet plans in the QueryExecutor. This is generally not a good idea because it interweaves contention between two different resources, and with the existing implementation, it’s actually incorrect: calling any of the debug handlers on a vttablet will cause it to crash, because the HTTP endpoint that lists all cached queries in the tablet expects all the elements of the cache to be TabletPlans. It’ll panic when it finds a connection setting!

This has been fixed by splitting the two resources into two separate caches. The keying for the two caches has been optimized further: for the plans, the original SQL is used as a key. This is efficient in practice because the original SQL is kept as part of the plan, so using it as a key cache does not allocate more memory. For the settings, we hash the key/values into a 256 bit key, as they can be arbitrarily large.

Request Unification

Now that the tablets no longer need to serialize incoming requests, they are vulnerable to thundering herd issues, particularly every time the underlying schema changes and we have to flush the cache. The requests coming to the vtgates have always been vulnerable to this same issue. I’ve fixed both implementations with a proper Single Flight implementation that is embedded in the cache and that prevents the same query from being planned more than one simultaneously. Any other requests for the same plan will block until the initial planning has been completed and cached, and the plan will only be fetched once from the cache.

Deprecation: cache resizing

The existing cache implementation has rudimentary support for changing the capacity of the cache. This feature is never used on the vtgate‘s plan cache, even though it’s usually the hottest one, but it is exposed via a /debug endpoint for the vttablets TabletPlan cache. I believe resizing the system’s cache at runtime is a misfeature. The existing implementation is not fully operational, as it was implemented only to fulfill the API that we expose via /debug, so it only modifies the cache’s capacity in the policy acceptor. This means that reducing the cache’s size doesn’t actually evict any data from the cache right away, and increasing the cache’s size doesn’t necessarily allow more plans to be cached right away because the filters in the policy have been trained under the old size.

Overall, I think it’d be a mistake to implement cache resizing at runtime for this new iteration of the cache, so I’ve removed support from the /debug endpoint that exposed this functionality. My understanding is that /debug is not part of our stability guarantees so there’s not much to do here besides pointing it out on the release notes. Cache sizing should always be chosen when configuring a Vitess deployment. Resizing a cache at runtime is a recipe for an availability issue.

Deprecation: LRU Plan cache

When I initially shipped the second (current) version of our cache implementation, it was flagged behind an "lfu" flag, even though the default for the flag was on. The idea was making the transition from LRU to LFU as smooth as possible. After a couple years with the new implementation, the reality is that we’ve never found a single benchmark or use case where the LRU cache performs better than the LFU one. Our implementation for LRU is extremely primitive, whilst our current LFU, and this new replacement, are state-of-the art and based on well understood and well tested adaptive algorithms that are designed to perform well under any workload.

I see no reason to continue supporting LRU caches for our plan caches, so I’ve deprecated the CLI flag for lru and made it a no-op: Vitess will now always default to using an adaptive LFU cache for plans.

As for the migration from our current LFU cache to this new one: I briefly considered allowing the two implementations to be swappable, but the behavior of the existing cache when doing a full eviction is problematic, and it has been thoroughly fixed in both the vtgate and vttablet plan caches (see: epoch-based expiration, atomic schema tracking in vttablet). These fixes cannot be backported to the old cache, and given the amount of testing we have for the cache’s behavior, I feel confident on swapping the LFU cache implementation without providing a fallback to the old one.

cc @vitessio/query-serving

Related Issue(s)

Checklist

• "Backport to:" labels have been added if this change should be back-ported
• Tests were added or are not required
• Did the new or modified tests pass consistently locally and on the CI
• Documentation was added or is not required

Deployment Notes

[vitess-bot]

Review Checklist

Hello reviewers! 👋 Please follow this checklist when reviewing this Pull Request.

General

• Ensure that the Pull Request has a descriptive title.
• Ensure there is a link to an issue (except for internal cleanup and flaky test fixes), new features should have an RFC that documents use cases and test cases.

Tests

• Bug fixes should have at least one unit or end-to-end test, enhancement and new features should have a sufficient number of tests.

Documentation

• Apply the release notes (needs details) label if users need to know about this change.
• New features should be documented.
• There should be some code comments as to why things are implemented the way they are.
• There should be a comment at the top of each new or modified test to explain what the test does.

New flags

• Is this flag really necessary?
• Flag names must be clear and intuitive, use dashes (-), and have a clear help text.

If a workflow is added or modified:

• Each item in Jobs should be named in order to mark it as required.
• If the workflow needs to be marked as required, the maintainer team must be notified.

Backward compatibility

• Protobuf changes should be wire-compatible.
• Changes to _vt tables and RPCs need to be backward compatible.
• RPC changes should be compatible with vitess-operator
• If a flag is removed, then it should also be removed from vitess-operator and arewefastyet, if used there.
• vtctl command output order should be stable and awk-able.

[vmg]

Yoohoo! This is ready for review. I had to do a lot of cleanup and refactoring on the tests, because a lot of them assume that the cache is always deterministic and can be used to detect things like e.g. the number of times a query was run. This is not the case with an adaptive cache, because it uses heuristics to evict or not accept certain entries.

Overall, I think I made the endtoend tests much more resilient after these changes, particularly the ones for vreplication. Give it a go!

[systay]

4*1024*1024 isn't supposed to be configurable, right?

[vmg]

No, the cache in vtexplain is not particularly performance sensitive so I don't think it requires customization.

[deepthi]

Very nice implementation, especially the epoch-based expiration/eviction. I appreciated the detailed explanation in the PR description without which I would have had a hard time understanding it 😄
Most of my comments are nits / code re-org type of suggestions, so I'll go ahead and approve. You can resolve them at your convenience.
I assume that the reason the LRUCache implementation hasn't been removed completely is because it is still being used in a couple of places. It may be worth reviewing those to see if they should be rewritten in a separate PR.

[deepthi]

I realize that you didn't create this file, but I see no reason why it should be under go/vt. There's nothing Vitess-specific here, so it belongs better in a package like go/hash.
Also, we have another hash.go under go/textutil. Ideally we'd combine the two files and keep all hash functions together, except for the one in vindexes/hash.go which is its own thing.

[deepthi]

nit: assertQueryDoesNotExecuteOnTablet

[deepthi]

Why aren't you using assert.Equal?

[deepthi]

Should use assert.Equal.

[deepthi]

💯

[deepthi]

@mattlord / @rohit-nayak-ps can you look this change over and make sure it doesn't actually change test expectation?

[mattlord]

LGTM. I suspect we added those log messages for local debugging.

[deepthi]

Hmm.. this can be iffy. Not always guaranteed to work well in CI. Is there no condition we can wait for?

[deepthi]

Not picking on this one because the file is full of these. No point changing one instance to require.

[mattlord]

LGTM. I only had some minor nits along with Deepthi. I'm not adding formal approval as you've already got one (from Deepthi) and we should get at least one approval from someone in Query Serving.

Nice work on this, @vmg! I hope that this lowers vtgate's overall CPU usage (I suspect it will) -- which is a long standing complaint from many users.

[mattlord]

LGTM. I suspect we added those log messages for local debugging.

[mattlord]

So we're vendoring this then right, along with theine?

[systay]

LGTM

[vmg]

Yay! Thank you! @deepthi I'm gonna open a separate PR with hash improvements and move the packages as you suggested.