Conversation
5e9e8d1 to
e60e76c
Compare
| // will go through the wrapped Cache. | ||
| template <typename K, typename T, typename H = std::hash<K>, | ||
| typename E = std::equal_to<K>> | ||
| class PersistentCache : public AbstractCache<K, T, H, E> { |
There was a problem hiding this comment.
Can we specialize the PersistentCache a bit more? Do we really need to carry that many template parameters?
There was a problem hiding this comment.
I don't see a use case for the H and E templates, so those can be dropped and the default value directly applied the parent class. Keeping the same interface as Cache lets this be a drop-in replacement though, maybe we can follow up to remove the unnecessary templates from both?
There was a problem hiding this comment.
Let me take a closer look (probably after my break...). I need to play around with the patch to awake my C++ muscles...
| std::lock_guard<std::mutex> slock(lock_); | ||
| memory_cache_.Clear(); | ||
| // Delete and recreate the cache directory on disk. | ||
| if (!readonly_) { |
There was a problem hiding this comment.
Maybe instead of conditioning on readonly_ here, introduce an optional arg for Clear to clear the persistent cache, if cache_dir is provided.
There was a problem hiding this comment.
I think we should respect the readonly_ setting here - the reason for it is in shared mounts, we only want the writer to modify the objects on disk.
alanwaketan
left a comment
There was a problem hiding this comment.
A few nit, but LGTM in general.
* Add a persistent cache * Add locking * Make serialize functions accept const parameters * Clarify readonly parameter
* Add a persistent cache * Add locking * Make serialize functions accept const parameters * Clarify readonly parameter
* Add a persistent cache * Add locking * Make serialize functions accept const parameters * Clarify readonly parameter
This changes adds a new PersistentCache instance which will read and write to disk to enable persistent compilation caching. The PersistentCache is currently not used anywhere, but in a later change, the PersistentCache will replace the Cache in the XlaGraphExecutor when persistent caching is enabled.