Skip to content

[volumecache 06/12] Index and evict disk cache entries - #376

Open
toloudis wants to merge 2 commits into
feature/volumecache-pr05-disk-persistencefrom
feature/volumecache-pr06-disk-index-eviction
Open

[volumecache 06/12] Index and evict disk cache entries#376
toloudis wants to merge 2 commits into
feature/volumecache-pr05-disk-persistencefrom
feature/volumecache-pr06-disk-index-eviction

Conversation

@toloudis

@toloudis toloudis commented Jun 7, 2026

Copy link
Copy Markdown
Contributor

Stacked draft PR 6 of 12 for the refreshed volume cache work.

This PR adds cold-start disk index loading, disk byte accounting, and LRU-style eviction when writes would exceed the configured disk cap.

The key thing here is that when we start up agave, we read the disk cache so that we have an in-memory index of it.

@toloudis
toloudis force-pushed the feature/volumecache-pr06-disk-index-eviction branch from fe14d86 to 545360a Compare June 22, 2026 03:27
@toloudis
toloudis force-pushed the feature/volumecache-pr06-disk-index-eviction branch from 545360a to d56be02 Compare August 14, 2026 21:45
@toloudis
toloudis force-pushed the feature/volumecache-pr06-disk-index-eviction branch from d56be02 to 61bc5ac Compare August 15, 2026 15:34
@toloudis
toloudis marked this pull request as ready for review August 27, 2026 18:22
if (m_currentDiskBytes >= it->second.bytes) {
m_currentDiskBytes -= it->second.bytes;
} else {
m_currentDiskBytes = 0;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe would also want to flag for a disk index rebuild here

Comment thread renderlib/CacheManager.h
Comment on lines +146 to +150
// Tracks the root the in-memory index was last built against (used to
// decide when a rebuild is needed).
// Set to m_cacheDir when the in-memory disk index is built, such as on startup.
// Clear this value to signal that the in-memory disk index is no longer valid.
std::string m_diskIndexRoot;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I feel slightly weird about m_diskIndexRoot being both the root of the disk index and a flag for when it needs to be rebuilt... If it's only ever set to m_cacheDir when active, could it just be a boolean flag for needsRebuild? Or is the concern that m_cacheDir might change from under it?

Comment on lines +303 to 304
std::scoped_lock lock(m_mutex);
evictIfNeededLocked(0);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: Should this be wrapped in a scope, in case this function is ever extended?

Comment on lines +851 to +853
if (!std::filesystem::exists(root)) {
return;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should this throw an error or log a warning?

// On Windows this can fail if another thread (e.g. loadFromDisk)
// still holds tensorstore file handles into the same path. The
// bookkeeping has already been updated to reflect eviction; the
// stale files will be picked up by the next clearDiskCache.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

When is clearDiskCache called? If it has to be manually triggered by the user, is it possible for the files to persist through multiple sessions with AGAVE and continually expand the cache?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants