-
Notifications
You must be signed in to change notification settings - Fork 505
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
Add a persistent cache #5800
Add a persistent cache #5800
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me take a closer look (probably after my break...). I need to play around with the patch to awake my C++ muscles...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
torch_xla/csrc/runtime/cache.h
Outdated
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks @jonb377 left minor comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.