Skip to content

Add back write caching, simplify caching, use sync I/O #3995

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

Closed
wants to merge 1 commit into from

Conversation

davidmorgan
Copy link
Contributor

@davidmorgan davidmorgan commented May 5, 2025

For #3811.

This is more about adding back functionality removed during the refactoring than performance. The reason write caching was added was to prevent files appearing during the build from causing reanalysis.

But it does seem to be about 10% faster.

  • Add write caching to the existing FilesystemCache
  • Simplify: make the cache synchronous, use sync I/O, remove LruCache
  • Because caching is now either passthrough or combined with write caching, invalidate on writing outputs is no longer needed
  • Remove a few more Future.wait in favour of for loops

Possibly some cache size limit is still a good idea, although for most projects the total source size seen should be small, and there is always the "low resource mode" flag to turn it off.

Copy link

github-actions bot commented May 5, 2025

PR Health

@davidmorgan davidmorgan force-pushed the write-caching branch 6 times, most recently from e991d71 to 5d163a4 Compare May 7, 2025 10:53
@davidmorgan davidmorgan changed the title Add write caching. Add back write caching, simplify caching, use sync I/O May 7, 2025
@davidmorgan davidmorgan requested a review from jensjoha May 7, 2025 14:25
@davidmorgan davidmorgan marked this pull request as ready for review May 7, 2025 14:36
@davidmorgan davidmorgan removed the request for review from jensjoha May 8, 2025 06:24
@davidmorgan
Copy link
Contributor Author

There is a possible issue with memory usage before this PR #3999

So I think it is not good to remove the lru cache and complicate things further :)

The write part of the cache needs to not do discards, so that needs a bit of separate logic if keeping the lru cache; it should be a lot simpler now that everything is sync. I'll update the PR to do that then send for review again. Sorry for the noise!

Copy link

@jensjoha jensjoha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving the comments and posting with your updated comment.


/// Pending [readAsBytes] operations.
final _pendingBytesContentCache = <AssetId, Future<Uint8List>>{};
final _bytesContentCache = <AssetId, Uint8List>{};

/// Cached results of [exists].
///
/// Don't bother using an LRU cache for this since it's just booleans.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment no longer makes sense.

bool exists(AssetId id, {required bool Function() ifAbsent}) {
final maybeResult = _canReadCache[id];
if (maybeResult != null) return maybeResult;
return _canReadCache[id] = ifAbsent();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got confused about the _canReadCache name. Maybe _existsCache would make more sense?

@jensjoha
Copy link

jensjoha commented May 8, 2025

One thing though, with you changing this anyway: If not too much trouble, could you try doing the sync change as a separate PR? It would be interesting to know how much that changes on its own.

@davidmorgan davidmorgan mentioned this pull request May 8, 2025
@davidmorgan
Copy link
Contributor Author

One thing though, with you changing this anyway: If not too much trouble, could you try doing the sync change as a separate PR? It would be interesting to know how much that changes on its own.

Thanks--done--yes, it's better to split out. I noticed I should remove all the async methods from Filesystem, done in #4000 .

@davidmorgan
Copy link
Contributor Author

Closing in favour of #4001

@davidmorgan davidmorgan closed this May 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants