Refactor CachingAssetReader to FilesystemCache.

[davidmorgan]

For #3811.

Move caching out of AssetReader and next to the filesystem.

For now, largely preserves existing functionality, for simplicity. I'll improve these in future PRs as needed:

• only async methods are cached
• the caching strategy is the same

But there is one behaviour change:

• the digest method used to bypass the cache, now it reads from the cache if there is one
• explicitly call invalidate in places that use digest and expect a fresh result

One other change / cleanup: remove test StubAssetReader, which always returned a fixed digest. Update tests to use InMemoryAssetReaderWriter and write files to it as needed.

[github-actions]

PR Health

Changelog Entry ✔️

Package	Changed Files

Changes to files need to be accounted for in their respective changelogs.

[davidmorgan]

FYI, while working on the next PR it looks like I probably want to keep Filesystem not having a cache, and call the cache from FileBasedAssetReader / InMemoryAssetReaderWriter; which probably do end up being the same class when all this is over.

I think this PR still works as is, just wanted to mention the change in direction coming :) thanks.

[jensjoha]

Why are all the copyWiths with a single optional argument, when - unless I missed somewhere - it ultimately just does a whole lot of nothing?

[jensjoha]

Here - and below - it talks about a specific implementation. Isn't that weird to have as documentation for the interface?

[davidmorgan]

It's a bit awkward :) while writing the next PR I realized it makes more sense to leave Filesystem always uncached and do the caching one level up in the reader. So that's the plan.

As the code is I do think it makes sense on the interface, because exactly how the cache is used has visible side effects. Which is also why it makes sense to move it out, it makes this type do too much :)

[jensjoha]

Are these numbers copied from somewhere?
Do we have any numbers on "normal" usage of these, because 2 x 0.5 GB seems quite high to me...

[davidmorgan]

Yes, it's copied.

Added a TODO to look into the caching strategy; I suspect we can do something simpler. It would be surprising if we can't fit one build's worth of input in memory ... particularly given that when running with the analyzer, there is exactly another copy of all the inputs in the in-memory filesystem we give to the analyzer :)

Thanks.

[davidmorgan]

Why are all the copyWiths with a single optional argument, when - unless I missed somewhere - it ultimately just does a whole lot of nothing?

Good question :) there will be more fields+arguments soon; the plan is that rather than nesting delegating readers you copyWith and set the functionality you want that way, so there is just one instance with a collection of things plugged into it.

So for example AssetPathProvider is currently "set" by wrapping in a BuildCacheReader, that will change to copyWith(assetPathProvider: ...).

Thanks.

[davidmorgan]

Yes, it's copied.

Added a TODO to look into the caching strategy; I suspect we can do something simpler. It would be surprising if we can't fit one build's worth of input in memory ... particularly given that when running with the analyzer, there is exactly another copy of all the inputs in the in-memory filesystem we give to the analyzer :)

Thanks.

[davidmorgan]

It's a bit awkward :) while writing the next PR I realized it makes more sense to leave Filesystem always uncached and do the caching one level up in the reader. So that's the plan.

As the code is I do think it makes sense on the interface, because exactly how the cache is used has visible side effects. Which is also why it makes sense to move it out, it makes this type do too much :)