Don't modify the on disk cache in fine-grained mode #4664
Conversation
They'll still merge conflict, but the resolution will be trivial now (I'm trying avoid making one depend on the other)
They'll still merge conflict, but the resolution will be trivial now (I'm trying avoid making one depend on the other)
There was a problem hiding this comment.
Looks good, just a few notes below. Please test this change manually with and without a remote cache before merging.
It would be useful to have the motivation for this change spelled out in the commit message. Is it more about performance of incremental updates or about correctness?
Longer-term, we may want to write cache files in fine-grained incremental mode, at least when not using a remote cache. This could improve performance if users frequently restart the daemon and don't use a remote cache (or if we decide to shut down the daemon after some time of inactivity). Can you create an issue about this?
| if not self.options.use_fine_grained_cache: | ||
| # Stores the initial state of sources as a side effect. | ||
| self.fswatcher.find_changed() | ||
| # Stores the initial state of sources as a side effect. |
There was a problem hiding this comment.
Why was this changed to be executed unconditionally?
There was a problem hiding this comment.
So that we have accurate fswatcher cache information for files that we didn't read from the on-disk cache, now that we don't generate CacheMetas
mypy/dmypy_server.py
Outdated
| # Run a fine-grained update starting from the cached data | ||
| if self.options.use_fine_grained_cache: | ||
| # Pull times and hashes out of the saved_cache and stick them into | ||
| # the fswatcher, so we pick up the changes. | ||
| for state in self.fine_grained_manager.graph.values(): | ||
| meta = state.meta | ||
| # If there isn't a meta, that means the current | ||
| # version got checked in the initial build. |
There was a problem hiding this comment.
Relying on this logic here seems a bit fragile. Maybe move the meta is None check to a helper method in State and use the helper method here instead?
There was a problem hiding this comment.
Changing it to use is_cache_skeleton instead.
|
The main rationale is closer to correctness than performance. Currently we write and delete cache files during the initial load of fine-grained mode, but never during fine-grained updates. This means that fine-grained can mess the cache up but won't fix it without a restart, which seems not great. |
This is a little subtle, because interface_hash still needs to be computed, as it is a major driver of the coarse-grained build process. Since metas are no longer computed for files that get rechecked during build, to avoid spuriously reprocessing them we need to find initial file state in cache mdoe as well.
msullivan
force-pushed
the
no_fg_cache_write
branch
from
f9076ed to
8b3702d
Compare
|
Withdrawing this. Some changes I am making as part of my deletion optimization path is going to make this fall out much more simply. |
This is a little subtle, because interface_hash still needs to be
computed, as it is a major driver of the coarse-grained build process.
Since metas are no longer computed for files that get rechecked during
build, to avoid spuriously reprocessing them we need to find initial
file state in cache mode as well.