Fix bug #7150 (Switching projects or quitting sometimes pauses for several seconds)#7155
Fix bug #7150 (Switching projects or quitting sometimes pauses for several seconds)#7155
Conversation
…veral seconds) - Don't let Directory try to clear its immediate childrens' caches. If we've already visited the parent dir and then we visit one of its children, that child would have no _content anymore and would resort to a full index-walk to try to find its own immediate children. This is slow to do many times, and redundant since we're already doing a full walk anyway.
|
Reviewing |
There was a problem hiding this comment.
Wouldn't the call to _clearCachedData on line 723 also need to pass true?
There was a problem hiding this comment.
Good catch - it has the same whole-subtree scoping as this loop.
There was a problem hiding this comment.
Though I'm actually confused as to why that visitAll()-_clearCachedData() loop is even needed -- non-watched entries are never supposed to cache anything to begin with, and unwatching clears the cache of anything that was previously watched (as we can see here). Maybe that code is a remnant from before one of those things was true?
@iwehrman, can you remember why we do that in _handleDirectoryChange()?
There was a problem hiding this comment.
I can't recall. Where did we leave the idea of caching directory contents even for unwatched directories. Could it have something to do with that?
There was a problem hiding this comment.
Yeah, maybe it's a remnant from that? We definitely don't cache unwatched directories now though -- getContents() checks isWatched() before storing anything.
I guess there's a potential edge case due to the 'relaxed' flag -- if you read a directory while watchers are starting up and then watching fails without watchers going fully offline, it looks like we might not clear the dir's cache. But otoh, we also won't get any change notifications for that dir later, so it still seems unlikely this code would help.
Anyway, I guess I'll push up the change to add a true arg there, and just take it on faith that it will work as well in the other two cases... since I can't seem to come up with any cases to exercise that code :-)
There was a problem hiding this comment.
I think this code could also get hit if you programmatically access a directory that's under the watch root but filtered out by it -- on a recursive-watcher platform you'll get notifications from the impl about it changing, since you forcibly created the entry it'll exist in the index and not bail early in _handleExternalChange(). But even in that case where I think the code runs, it won't actually do anything since the entry will never let itself cache anything (_isWatched() checks the watch root filtering).
I'll file a cleanup bug about removing this mystery code at a later point in time...
There was a problem hiding this comment.
I've spun off #7172 to consider ripping this code out eventually
There was a problem hiding this comment.
I've spun off #7172 to consider ripping this code out eventually
|
Review complete. The change looks good, there's just that one question about whether |
that follows the same potentially-inefficient pattern. Ian & I can't figure out when this code would ever be needed - receiving a change event for something that's not watched is nearly impossible, and it won't have any cached data to clear anyway. We should probably remove this code in the future. See #7155 (comment) & follow-on comments for more.
|
@dangoor Change pushed. The code's purpose remains mysterious :-) Will file cleanup bug for removing it. |
Fix bug #7150 (Switching projects or quitting sometimes pauses for several seconds)
3 participants
Fix bug #7150 -- Don't let Directory try to clear its immediate childrens' caches. If we've already visited the parent dir and then we visit one of its children, that child would have no _content anymore and would resort to a full index-walk to try to find its own immediate children. This is slow to do many times, and redundant since we're already doing a full walk anyway.