From 3de0f721cfcd277c86f627df5be40dd0bfaaffcd Mon Sep 17 00:00:00 2001 From: dmt Date: Wed, 18 Feb 2026 21:04:32 -0800 Subject: [PATCH 01/31] ai generated caching system. todo: review and test --- agave_app/CMakeLists.txt | 6 + agave_app/CacheSettings.cpp | 223 +++++++++ agave_app/CacheSettings.h | 37 ++ agave_app/CacheSettingsDockWidget.cpp | 8 + agave_app/CacheSettingsDockWidget.h | 18 + agave_app/CacheSettingsWidget.cpp | 76 +++ agave_app/CacheSettingsWidget.h | 36 ++ agave_app/agaveGui.cpp | 23 +- agave_app/agaveGui.h | 5 + renderlib/CMakeLists.txt | 3 + renderlib/CacheConfig.h | 17 + renderlib/CacheManager.cpp | 665 ++++++++++++++++++++++++++ renderlib/CacheManager.h | 110 +++++ renderlib/command.cpp | 11 +- renderlib/io/FileReader.cpp | 27 +- renderlib/io/FileReader.h | 2 - 16 files changed, 1242 insertions(+), 25 deletions(-) create mode 100644 agave_app/CacheSettings.cpp create mode 100644 agave_app/CacheSettings.h create mode 100644 agave_app/CacheSettingsDockWidget.cpp create mode 100644 agave_app/CacheSettingsDockWidget.h create mode 100644 agave_app/CacheSettingsWidget.cpp create mode 100644 agave_app/CacheSettingsWidget.h create mode 100644 renderlib/CacheConfig.h create mode 100644 renderlib/CacheManager.cpp create mode 100644 renderlib/CacheManager.h diff --git a/agave_app/CMakeLists.txt b/agave_app/CMakeLists.txt index 6f351a632..c7612d78b 100644 --- a/agave_app/CMakeLists.txt +++ b/agave_app/CMakeLists.txt @@ -27,6 +27,12 @@ target_sources(agaveapp PRIVATE "${CMAKE_CURRENT_SOURCE_DIR}/CameraDockWidget.h" "${CMAKE_CURRENT_SOURCE_DIR}/CameraWidget.cpp" "${CMAKE_CURRENT_SOURCE_DIR}/CameraWidget.h" + "${CMAKE_CURRENT_SOURCE_DIR}/CacheSettings.cpp" + "${CMAKE_CURRENT_SOURCE_DIR}/CacheSettings.h" + "${CMAKE_CURRENT_SOURCE_DIR}/CacheSettingsDockWidget.cpp" + "${CMAKE_CURRENT_SOURCE_DIR}/CacheSettingsDockWidget.h" + "${CMAKE_CURRENT_SOURCE_DIR}/CacheSettingsWidget.cpp" + "${CMAKE_CURRENT_SOURCE_DIR}/CacheSettingsWidget.h" "${CMAKE_CURRENT_SOURCE_DIR}/citationDialog.cpp" "${CMAKE_CURRENT_SOURCE_DIR}/citationDialog.h" "${CMAKE_CURRENT_SOURCE_DIR}/cgiparser.cpp" diff --git a/agave_app/CacheSettings.cpp b/agave_app/CacheSettings.cpp new file mode 100644 index 000000000..aedbf6714 --- /dev/null +++ b/agave_app/CacheSettings.cpp @@ -0,0 +1,223 @@ +#include "CacheSettings.h" + +#include "renderlib/CacheManager.h" +#include "renderlib/Logging.h" + +#include +#include +#include +#include + +#include + +#ifdef _WIN32 +#ifndef NOMINMAX +#define NOMINMAX +#endif +#include +#elif defined(__linux__) +#include +#endif + +CacheSettings::CacheSettings() {} + +CacheSettingsData +CacheSettings::defaultSettings() const +{ + CacheSettingsData data; + QString baseDir = QStandardPaths::writableLocation(QStandardPaths::CacheLocation); + if (baseDir.isEmpty()) { + baseDir = QStandardPaths::writableLocation(QStandardPaths::AppDataLocation); + } + if (baseDir.isEmpty()) { + baseDir = QDir::currentPath(); + } + data.cacheDir = QDir(baseDir).filePath("agave-cache").toStdString(); + return data; +} + +std::string +CacheSettings::configPath() const +{ + QString baseDir = QStandardPaths::writableLocation(QStandardPaths::AppConfigLocation); + if (baseDir.isEmpty()) { + baseDir = QStandardPaths::writableLocation(QStandardPaths::AppDataLocation); + } + if (baseDir.isEmpty()) { + baseDir = QDir::currentPath(); + } + QDir().mkpath(baseDir); + return QDir(baseDir).filePath("cache_settings.json").toStdString(); +} + +CacheSettingsData +CacheSettings::load() +{ + CacheSettingsData data = defaultSettings(); + QString path = QString::fromStdString(configPath()); + QFile file(path); + if (!file.exists()) { + return data; + } + if (!file.open(QIODevice::ReadOnly)) { + return data; + } + + QByteArray raw = file.readAll(); + try { + nlohmann::json doc = nlohmann::json::parse(raw.toStdString()); + if (doc.contains("enabled")) { + data.enabled = doc["enabled"].get(); + } + if (doc.contains("enableDisk")) { + data.enableDisk = doc["enableDisk"].get(); + } + if (doc.contains("maxRamBytes")) { + data.maxRamBytes = doc["maxRamBytes"].get(); + } + if (doc.contains("maxDiskBytes")) { + data.maxDiskBytes = doc["maxDiskBytes"].get(); + } + if (doc.contains("cacheDir")) { + data.cacheDir = doc["cacheDir"].get(); + } + } catch (...) { + return defaultSettings(); + } + + return data; +} + +bool +CacheSettings::save(const CacheSettingsData& data) const +{ + nlohmann::json doc; + doc["enabled"] = data.enabled; + doc["enableDisk"] = data.enableDisk; + doc["maxRamBytes"] = data.maxRamBytes; + doc["maxDiskBytes"] = data.maxDiskBytes; + doc["cacheDir"] = data.cacheDir; + + QString path = QString::fromStdString(configPath()); + QFile file(path); + if (!file.open(QIODevice::WriteOnly)) { + return false; + } + std::string out = doc.dump(2); + file.write(out.c_str(), static_cast(out.size())); + return true; +} + +renderlib::CacheConfig +CacheSettings::toRenderlibConfig(const CacheSettingsData& data) const +{ + CacheSettingsData normalized = data; + if (normalized.cacheDir.empty()) { + normalized.cacheDir = defaultSettings().cacheDir; + } + + renderlib::CacheConfig config; + config.enabled = normalized.enabled; + config.enableDisk = normalized.enableDisk; + config.cacheDir = normalized.cacheDir; + + std::uint64_t availableMem = availableMemoryBytes(); + if (availableMem > 0) { + config.maxRamBytes = std::min(normalized.maxRamBytes, availableMem); + } else { + config.maxRamBytes = normalized.maxRamBytes; + } + + std::uint64_t availableDisk = availableDiskBytes(normalized.cacheDir); + if (availableDisk > 0) { + config.maxDiskBytes = std::min(normalized.maxDiskBytes, availableDisk); + } else { + config.maxDiskBytes = normalized.maxDiskBytes; + } + + if (!config.enableDisk) { + config.maxDiskBytes = 0; + } + + return config; +} + +void +CacheSettings::applyToRenderlib(const CacheSettingsData& data) const +{ + renderlib::CacheConfig config = toRenderlibConfig(data); + if (config.enableDisk && !config.cacheDir.empty()) { + if (!canWriteCacheDir(config.cacheDir)) { + LOG_WARNING << "Cache disk disabled: cache directory not writable: " << config.cacheDir; + config.enableDisk = false; + config.maxDiskBytes = 0; + } + } + LOG_INFO << "Cache config: enabled=" << (config.enabled ? 1 : 0) << " ram_bytes=" << config.maxRamBytes + << " disk_enabled=" << (config.enableDisk ? 1 : 0) << " disk_bytes=" << config.maxDiskBytes + << " cache_dir=" << config.cacheDir; + renderlib::CacheManager::instance().setConfig(config); + auto applied = renderlib::CacheManager::instance().getConfig(); + LOG_INFO << "Cache config applied: enabled=" << (applied.enabled ? 1 : 0) << " ram_bytes=" << applied.maxRamBytes + << " disk_enabled=" << (applied.enableDisk ? 1 : 0) << " disk_bytes=" << applied.maxDiskBytes + << " cache_dir=" << applied.cacheDir; +} + +std::uint64_t +CacheSettings::availableMemoryBytes() const +{ +#ifdef _WIN32 + MEMORYSTATUSEX statex; + statex.dwLength = sizeof(statex); + if (GlobalMemoryStatusEx(&statex)) { + return static_cast(statex.ullAvailPhys); + } + return 0; +#elif defined(__linux__) + struct sysinfo info; + if (sysinfo(&info) == 0) { + return static_cast(info.freeram) * static_cast(info.mem_unit); + } + return 0; +#else + return 0; +#endif +} + +std::uint64_t +CacheSettings::availableDiskBytes(const std::string& path) const +{ + if (path.empty()) { + return 0; + } + QStorageInfo storage(QString::fromStdString(path)); + if (!storage.isValid() || !storage.isReady()) { + return 0; + } + return static_cast(storage.bytesAvailable()); +} + +bool +CacheSettings::canWriteCacheDir(const std::string& path) const +{ + if (path.empty()) { + return false; + } + + QString qPath = QString::fromStdString(path); + QDir dir(qPath); + if (!dir.exists()) { + if (!dir.mkpath(".")) { + return false; + } + } + + QString testPath = dir.filePath(".agave_cache_write_test"); + QFile testFile(testPath); + if (!testFile.open(QIODevice::WriteOnly | QIODevice::Truncate)) { + return false; + } + testFile.write("test"); + testFile.close(); + return testFile.remove(); +} diff --git a/agave_app/CacheSettings.h b/agave_app/CacheSettings.h new file mode 100644 index 000000000..838852e8e --- /dev/null +++ b/agave_app/CacheSettings.h @@ -0,0 +1,37 @@ +#pragma once + +#include +#include + +namespace renderlib { +struct CacheConfig; +} + +struct CacheSettingsData +{ + bool enabled = true; + bool enableDisk = true; + std::uint64_t maxRamBytes = 4ULL * 1024ULL * 1024ULL * 1024ULL; + std::uint64_t maxDiskBytes = 100ULL * 1024ULL * 1024ULL * 1024ULL; + std::string cacheDir; +}; + +class CacheSettings +{ +public: + CacheSettings(); + + CacheSettingsData load(); + bool save(const CacheSettingsData& data) const; + + renderlib::CacheConfig toRenderlibConfig(const CacheSettingsData& data) const; + void applyToRenderlib(const CacheSettingsData& data) const; + + CacheSettingsData defaultSettings() const; + std::string configPath() const; + +private: + std::uint64_t availableMemoryBytes() const; + std::uint64_t availableDiskBytes(const std::string& path) const; + bool canWriteCacheDir(const std::string& path) const; +}; diff --git a/agave_app/CacheSettingsDockWidget.cpp b/agave_app/CacheSettingsDockWidget.cpp new file mode 100644 index 000000000..69af9babd --- /dev/null +++ b/agave_app/CacheSettingsDockWidget.cpp @@ -0,0 +1,8 @@ +#include "CacheSettingsDockWidget.h" + +CacheSettingsDockWidget::CacheSettingsDockWidget(QWidget* parent) + : QDockWidget(parent) +{ + setWindowTitle(tr("Advanced Cache Settings")); + setWidget(&m_settingsWidget); +} diff --git a/agave_app/CacheSettingsDockWidget.h b/agave_app/CacheSettingsDockWidget.h new file mode 100644 index 000000000..f7625ebb4 --- /dev/null +++ b/agave_app/CacheSettingsDockWidget.h @@ -0,0 +1,18 @@ +#pragma once + +#include + +#include "CacheSettingsWidget.h" + +class CacheSettingsDockWidget : public QDockWidget +{ + Q_OBJECT + +public: + explicit CacheSettingsDockWidget(QWidget* parent = nullptr); + + CacheSettingsWidget* widget() { return &m_settingsWidget; } + +private: + CacheSettingsWidget m_settingsWidget; +}; diff --git a/agave_app/CacheSettingsWidget.cpp b/agave_app/CacheSettingsWidget.cpp new file mode 100644 index 000000000..e824b86ba --- /dev/null +++ b/agave_app/CacheSettingsWidget.cpp @@ -0,0 +1,76 @@ +#include "CacheSettingsWidget.h" + +#include +#include +#include +#include + +CacheSettingsWidget::CacheSettingsWidget(QWidget* parent) + : QWidget(parent) +{ + auto* layout = new QFormLayout(this); + + m_enableCache = new QCheckBox(tr("Enable cache"), this); + m_enableDisk = new QCheckBox(tr("Enable disk cache"), this); + + m_ramLimitMB = new QSpinBox(this); + m_ramLimitMB->setRange(0, 1024 * 1024); + m_ramLimitMB->setSuffix(tr(" MB")); + m_ramLimitMB->setSingleStep(256); + + m_diskLimitGB = new QSpinBox(this); + m_diskLimitGB->setRange(0, 1024 * 1024); + m_diskLimitGB->setSuffix(tr(" GB")); + m_diskLimitGB->setSingleStep(10); + + m_cacheDirEdit = new QLineEdit(this); + m_browseButton = new QPushButton(tr("Browse"), this); + connect(m_browseButton, &QPushButton::clicked, this, &CacheSettingsWidget::browseForCacheDir); + + auto* dirLayout = new QHBoxLayout(); + dirLayout->addWidget(m_cacheDirEdit); + dirLayout->addWidget(m_browseButton); + + m_applyButton = new QPushButton(tr("Apply"), this); + m_clearDiskButton = new QPushButton(tr("Clear disk cache"), this); + + layout->addRow(m_enableCache); + layout->addRow(m_enableDisk); + layout->addRow(tr("RAM limit"), m_ramLimitMB); + layout->addRow(tr("Disk limit"), m_diskLimitGB); + layout->addRow(tr("Cache directory"), dirLayout); + layout->addRow(QString(), m_applyButton); + layout->addRow(QString(), m_clearDiskButton); + setLayout(layout); +} + +void +CacheSettingsWidget::setSettings(const CacheSettingsData& data) +{ + m_enableCache->setChecked(data.enabled); + m_enableDisk->setChecked(data.enableDisk); + m_ramLimitMB->setValue(static_cast(data.maxRamBytes / (1024ULL * 1024ULL))); + m_diskLimitGB->setValue(static_cast(data.maxDiskBytes / (1024ULL * 1024ULL * 1024ULL))); + m_cacheDirEdit->setText(QString::fromStdString(data.cacheDir)); +} + +CacheSettingsData +CacheSettingsWidget::getSettings() const +{ + CacheSettingsData data; + data.enabled = m_enableCache->isChecked(); + data.enableDisk = m_enableDisk->isChecked(); + data.maxRamBytes = static_cast(m_ramLimitMB->value()) * 1024ULL * 1024ULL; + data.maxDiskBytes = static_cast(m_diskLimitGB->value()) * 1024ULL * 1024ULL * 1024ULL; + data.cacheDir = m_cacheDirEdit->text().toStdString(); + return data; +} + +void +CacheSettingsWidget::browseForCacheDir() +{ + QString dir = QFileDialog::getExistingDirectory(this, tr("Select cache directory")); + if (!dir.isEmpty()) { + m_cacheDirEdit->setText(dir); + } +} diff --git a/agave_app/CacheSettingsWidget.h b/agave_app/CacheSettingsWidget.h new file mode 100644 index 000000000..a5434a1fc --- /dev/null +++ b/agave_app/CacheSettingsWidget.h @@ -0,0 +1,36 @@ +#pragma once + +#include "CacheSettings.h" + +#include +#include +#include +#include +#include + +class CacheSettingsWidget : public QWidget +{ + Q_OBJECT + +public: + explicit CacheSettingsWidget(QWidget* parent = nullptr); + + void setSettings(const CacheSettingsData& data); + CacheSettingsData getSettings() const; + + QPushButton* applyButton() const { return m_applyButton; } + QPushButton* clearDiskButton() const { return m_clearDiskButton; } + +private slots: + void browseForCacheDir(); + +private: + QCheckBox* m_enableCache = nullptr; + QCheckBox* m_enableDisk = nullptr; + QSpinBox* m_ramLimitMB = nullptr; + QSpinBox* m_diskLimitGB = nullptr; + QLineEdit* m_cacheDirEdit = nullptr; + QPushButton* m_browseButton = nullptr; + QPushButton* m_applyButton = nullptr; + QPushButton* m_clearDiskButton = nullptr; +}; diff --git a/agave_app/agaveGui.cpp b/agave_app/agaveGui.cpp index 71bbbd8b1..824af6927 100644 --- a/agave_app/agaveGui.cpp +++ b/agave_app/agaveGui.cpp @@ -10,10 +10,12 @@ #include "renderlib/Logging.h" #include "renderlib/Status.h" #include "renderlib/VolumeDimensions.h" +#include "renderlib/CacheManager.h" #include "renderlib/io/FileReader.h" #include "renderlib/version.hpp" #include "AppearanceDockWidget.h" +#include "CacheSettingsDockWidget.h" #include "CameraDockWidget.h" #include "Serialize.h" #include "StatisticsDockWidget.h" @@ -98,6 +100,18 @@ agaveGui::agaveGui(QWidget* parent) createDockWindows(); setDockOptions(AllowTabbedDocks); + CacheSettingsData cacheData = m_cacheSettings.load(); + m_cacheSettingsDockWidget->widget()->setSettings(cacheData); + m_cacheSettings.applyToRenderlib(cacheData); + connect(m_cacheSettingsDockWidget->widget()->applyButton(), &QPushButton::clicked, this, [this]() { + CacheSettingsData data = m_cacheSettingsDockWidget->widget()->getSettings(); + m_cacheSettings.save(data); + m_cacheSettings.applyToRenderlib(data); + }); + connect(m_cacheSettingsDockWidget->widget()->clearDiskButton(), &QPushButton::clicked, this, [this]() { + renderlib::CacheManager::instance().clearDiskCache(); + }); + m_tabs = new QTabWidget(this); QHBoxLayout* mainLayout = new QHBoxLayout; @@ -368,6 +382,11 @@ agaveGui::createDockWindows() m_statisticsDockWidget->setAllowedAreas(Qt::AllDockWidgetAreas); addDockWidget(Qt::RightDockWidgetArea, m_statisticsDockWidget); + m_cacheSettingsDockWidget = new CacheSettingsDockWidget(this); + m_cacheSettingsDockWidget->setAllowedAreas(Qt::AllDockWidgetAreas); + addDockWidget(Qt::LeftDockWidgetArea, m_cacheSettingsDockWidget); + m_cacheSettingsDockWidget->setVisible(false); + m_viewMenu->addSeparator(); m_viewMenu->addAction(m_cameradock->toggleViewAction()); m_viewMenu->addSeparator(); @@ -376,6 +395,8 @@ agaveGui::createDockWindows() m_viewMenu->addAction(m_appearanceDockWidget->toggleViewAction()); m_viewMenu->addSeparator(); m_viewMenu->addAction(m_statisticsDockWidget->toggleViewAction()); + m_viewMenu->addSeparator(); + m_viewMenu->addAction(m_cacheSettingsDockWidget->toggleViewAction()); } QSlider* @@ -828,7 +849,7 @@ agaveGui::open(const std::string& file, const Serialize::ViewerState* vs, bool i // We can update the render and gui progressively as chunks are loaded. // Also, this would allow renders to be cancelled during loading. QApplication::setOverrideCursor(QCursor(Qt::WaitCursor)); - std::shared_ptr image = reader->loadFromFile(loadSpec); + std::shared_ptr image = FileReader::loadAndCache(loadSpec); QApplication::restoreOverrideCursor(); if (!image) { LOG_DEBUG << "Failed to open " << file; diff --git a/agave_app/agaveGui.h b/agave_app/agaveGui.h index 4e97c3f9a..e5f65d9b2 100644 --- a/agave_app/agaveGui.h +++ b/agave_app/agaveGui.h @@ -3,6 +3,7 @@ #include "ui_agaveGui.h" #include "Camera.h" +#include "CacheSettings.h" #include "GLView3D.h" #include "QRenderSettings.h" #include "ViewerState.h" @@ -18,6 +19,7 @@ class QAppearanceDockWidget; class QCameraDockWidget; class QStatisticsDockWidget; class QTimelineDockWidget; +class CacheSettingsDockWidget; class IFileReader; class ViewToolbar; @@ -152,6 +154,7 @@ private slots: QAppearanceDockWidget* m_appearanceDockWidget; QStatisticsDockWidget* m_statisticsDockWidget; + CacheSettingsDockWidget* m_cacheSettingsDockWidget = nullptr; QTabWidget* m_tabs; GLView3D* m_glView; @@ -172,6 +175,8 @@ private slots: Scene m_appScene; int m_currentScene = 0; + CacheSettings m_cacheSettings; + QAction* m_recentFileActs[MaxRecentFiles]; QAction* m_recentFileSeparator; QAction* m_recentFileSubMenuAct; diff --git a/renderlib/CMakeLists.txt b/renderlib/CMakeLists.txt index c73b80ffa..11f91b380 100644 --- a/renderlib/CMakeLists.txt +++ b/renderlib/CMakeLists.txt @@ -24,6 +24,9 @@ target_sources(renderlib PRIVATE "${CMAKE_CURRENT_SOURCE_DIR}/BoundingBox.h" "${CMAKE_CURRENT_SOURCE_DIR}/BoundingBoxTool.cpp" "${CMAKE_CURRENT_SOURCE_DIR}/BoundingBoxTool.h" + "${CMAKE_CURRENT_SOURCE_DIR}/CacheConfig.h" + "${CMAKE_CURRENT_SOURCE_DIR}/CacheManager.cpp" + "${CMAKE_CURRENT_SOURCE_DIR}/CacheManager.h" "${CMAKE_CURRENT_SOURCE_DIR}/CCamera.h" "${CMAKE_CURRENT_SOURCE_DIR}/CCamera.cpp" "${CMAKE_CURRENT_SOURCE_DIR}/ClipPlaneTool.cpp" diff --git a/renderlib/CacheConfig.h b/renderlib/CacheConfig.h new file mode 100644 index 000000000..7697dae00 --- /dev/null +++ b/renderlib/CacheConfig.h @@ -0,0 +1,17 @@ +#pragma once + +#include +#include + +namespace renderlib { + +struct CacheConfig +{ + bool enabled = false; + bool enableDisk = false; + std::uint64_t maxRamBytes = 0; + std::uint64_t maxDiskBytes = 0; + std::string cacheDir; +}; + +} // namespace renderlib diff --git a/renderlib/CacheManager.cpp b/renderlib/CacheManager.cpp new file mode 100644 index 000000000..b619161e3 --- /dev/null +++ b/renderlib/CacheManager.cpp @@ -0,0 +1,665 @@ +#include "CacheManager.h" + +#include "ImageXYZC.h" +#include "Logging.h" + +#include "tensorstore/array.h" +#include "tensorstore/context.h" +#include "tensorstore/tensorstore.h" + +// must include after tensorstore so that tensorstore picks up its own internal json impl +#include "json/json.hpp" + +#include +#include +#include +#include +#include +#include +#include + +namespace renderlib { + +namespace { + +inline void +hashCombine(std::size_t& seed, std::size_t value) +{ + seed ^= value + 0x9e3779b9 + (seed << 6) + (seed >> 2); +} + +std::uint64_t +nowMillis() +{ + return static_cast( + std::chrono::duration_cast(std::chrono::system_clock::now().time_since_epoch()).count()); +} + +std::string +toHex(std::size_t value) +{ + std::ostringstream stream; + stream << std::hex << value; + return stream.str(); +} + +std::vector +channelNamesFromImage(const ImageXYZC& image) +{ + std::vector names; + names.reserve(image.sizeC()); + for (size_t i = 0; i < image.sizeC(); ++i) { + names.push_back(image.channel(static_cast(i))->m_name); + } + return names; +} + +} // namespace + +bool +CacheKey::operator==(const CacheKey& other) const +{ + return filepath == other.filepath && subpath == other.subpath && scene == other.scene && time == other.time && + channels == other.channels && minx == other.minx && maxx == other.maxx && miny == other.miny && + maxy == other.maxy && minz == other.minz && maxz == other.maxz && isImageSequence == other.isImageSequence; +} + +std::size_t +CacheKeyHash::operator()(const CacheKey& key) const +{ + std::size_t seed = 0; + hashCombine(seed, std::hash{}(key.filepath)); + hashCombine(seed, std::hash{}(key.subpath)); + hashCombine(seed, std::hash{}(key.scene)); + hashCombine(seed, std::hash{}(key.time)); + hashCombine(seed, std::hash{}(key.minx)); + hashCombine(seed, std::hash{}(key.maxx)); + hashCombine(seed, std::hash{}(key.miny)); + hashCombine(seed, std::hash{}(key.maxy)); + hashCombine(seed, std::hash{}(key.minz)); + hashCombine(seed, std::hash{}(key.maxz)); + hashCombine(seed, std::hash{}(key.isImageSequence)); + for (auto ch : key.channels) { + hashCombine(seed, std::hash{}(ch)); + } + return seed; +} + +CacheManager& +CacheManager::instance() +{ + static CacheManager manager; + return manager; +} + +void +CacheManager::setConfig(const CacheConfig& config) +{ + CacheConfig configCopy; + bool rebuildDiskIndex = false; + { + std::lock_guard lock(m_mutex); + m_config = config; + if (!m_config.enabled) { + m_entries.clear(); + m_lruKeys.clear(); + m_currentRamBytes = 0; + m_diskEntries.clear(); + m_currentDiskBytes = 0; + m_diskIndexRoot.clear(); + return; + } + + if (!m_config.enableDisk || m_config.cacheDir.empty()) { + m_diskEntries.clear(); + m_currentDiskBytes = 0; + m_diskIndexRoot.clear(); + } else if (m_diskIndexRoot != m_config.cacheDir) { + m_diskEntries.clear(); + m_currentDiskBytes = 0; + m_diskIndexRoot = m_config.cacheDir; + rebuildDiskIndex = true; + configCopy = m_config; + } + } + + if (rebuildDiskIndex) { + loadDiskIndex(configCopy); + evictDiskIfNeeded(configCopy, 0); + } + + evictIfNeeded(0); +} + +CacheConfig +CacheManager::getConfig() const +{ + std::lock_guard lock(m_mutex); + return m_config; +} + +std::shared_ptr +CacheManager::findImage(const LoadSpec& loadSpec) +{ + CacheConfig configCopy; + CacheKey key = makeKey(loadSpec); + { + std::lock_guard lock(m_mutex); + configCopy = m_config; + if (m_config.enabled && m_config.maxRamBytes > 0) { + auto it = m_entries.find(key); + if (it != m_entries.end()) { + touchEntry(it->second.lruIt); + m_stats.ramHits++; + LOG_DEBUG << "Cache stats: ram_hits=" << m_stats.ramHits << " disk_hits=" << m_stats.diskHits + << " misses=" << m_stats.misses << " disk_writes=" << m_stats.diskWrites; + return it->second.image; + } + } + } + + if (configCopy.enabled && configCopy.enableDisk && configCopy.maxDiskBytes > 0) { + auto diskImage = loadFromDisk(key, configCopy); + if (diskImage) { + { + std::lock_guard lock(m_mutex); + m_stats.diskHits++; + LOG_DEBUG << "Cache stats: ram_hits=" << m_stats.ramHits << " disk_hits=" << m_stats.diskHits + << " misses=" << m_stats.misses << " disk_writes=" << m_stats.diskWrites; + } + storeImageInMemory(key, diskImage); + return diskImage; + } + } + + { + std::lock_guard lock(m_mutex); + m_stats.misses++; + LOG_DEBUG << "Cache stats: ram_hits=" << m_stats.ramHits << " disk_hits=" << m_stats.diskHits + << " misses=" << m_stats.misses << " disk_writes=" << m_stats.diskWrites; + } + + return nullptr; +} + +void +CacheManager::storeImage(const LoadSpec& loadSpec, const std::shared_ptr& image) +{ + if (!image) { + return; + } + + CacheConfig configCopy; + { + std::lock_guard lock(m_mutex); + configCopy = m_config; + } + + if (configCopy.enabled && configCopy.enableDisk && configCopy.maxDiskBytes > 0) { + storeToDisk(makeKey(loadSpec), image, configCopy); + } + + storeImageInMemory(makeKey(loadSpec), image); +} + +void +CacheManager::clear() +{ + std::lock_guard lock(m_mutex); + m_entries.clear(); + m_lruKeys.clear(); + m_currentRamBytes = 0; +} + +void +CacheManager::clearDiskCache() +{ + CacheConfig configCopy; + { + std::lock_guard lock(m_mutex); + configCopy = m_config; + m_diskEntries.clear(); + m_currentDiskBytes = 0; + } + + if (configCopy.cacheDir.empty()) { + return; + } + + std::error_code ec; + std::filesystem::remove_all(configCopy.cacheDir, ec); +} + +CacheManager::CacheStats +CacheManager::getStats() const +{ + std::lock_guard lock(m_mutex); + return m_stats; +} + +void +CacheManager::resetStats() +{ + std::lock_guard lock(m_mutex); + m_stats = CacheStats{}; +} + +CacheKey +CacheManager::makeKey(const LoadSpec& loadSpec) const +{ + CacheKey key; + key.filepath = loadSpec.filepath; + key.subpath = loadSpec.subpath; + key.scene = loadSpec.scene; + key.time = loadSpec.time; + key.channels = loadSpec.channels; + key.minx = loadSpec.minx; + key.maxx = loadSpec.maxx; + key.miny = loadSpec.miny; + key.maxy = loadSpec.maxy; + key.minz = loadSpec.minz; + key.maxz = loadSpec.maxz; + key.isImageSequence = loadSpec.isImageSequence; + return key; +} + +std::string +CacheManager::keyToString(const CacheKey& key) const +{ + std::ostringstream stream; + stream << key.filepath << "|" << key.subpath << "|" << key.scene << "|" << key.time << "|"; + stream << key.minx << "," << key.maxx << "," << key.miny << "," << key.maxy << "," << key.minz << "," << key.maxz + << "|" << (key.isImageSequence ? 1 : 0) << "|"; + for (size_t i = 0; i < key.channels.size(); ++i) { + if (i > 0) { + stream << ","; + } + stream << key.channels[i]; + } + return stream.str(); +} + +std::string +CacheManager::diskCacheId(const CacheKey& key) const +{ + std::size_t hashValue = std::hash{}(keyToString(key)); + return toHex(hashValue); +} + +std::uint64_t +CacheManager::estimateImageBytes(const ImageXYZC& image) const +{ + std::uint64_t bytesPerPixel = static_cast(ImageXYZC::IN_MEMORY_BPP / 8); + std::uint64_t numPixels = static_cast(image.sizeX()) * static_cast(image.sizeY()) * + static_cast(image.sizeZ()) * static_cast(image.sizeC()); + return numPixels * bytesPerPixel; +} + +void +CacheManager::touchEntry(std::list::iterator it) +{ + if (it == m_lruKeys.begin()) { + return; + } + m_lruKeys.splice(m_lruKeys.begin(), m_lruKeys, it); +} + +void +CacheManager::evictIfNeeded(std::uint64_t incomingBytes) +{ + if (m_config.maxRamBytes == 0) { + return; + } + + while (!m_lruKeys.empty() && (m_currentRamBytes + incomingBytes) > m_config.maxRamBytes) { + const CacheKey& key = m_lruKeys.back(); + auto it = m_entries.find(key); + if (it != m_entries.end()) { + m_currentRamBytes -= it->second.bytes; + m_entries.erase(it); + } + m_lruKeys.pop_back(); + } +} + +void +CacheManager::storeImageInMemory(const CacheKey& key, const std::shared_ptr& image) +{ + std::lock_guard lock(m_mutex); + if (!m_config.enabled || m_config.maxRamBytes == 0) { + return; + } + + std::uint64_t bytes = estimateImageBytes(*image); + if (bytes == 0 || bytes > m_config.maxRamBytes) { + return; + } + + auto existing = m_entries.find(key); + if (existing != m_entries.end()) { + m_currentRamBytes -= existing->second.bytes; + m_lruKeys.erase(existing->second.lruIt); + m_entries.erase(existing); + } + + evictIfNeeded(bytes); + + m_lruKeys.push_front(key); + CacheEntry entry; + entry.image = image; + entry.bytes = bytes; + entry.lruIt = m_lruKeys.begin(); + m_entries.emplace(key, entry); + m_currentRamBytes += bytes; +} + +std::shared_ptr +CacheManager::loadFromDisk(const CacheKey& key, const CacheConfig& config) +{ + if (!config.enableDisk || config.cacheDir.empty()) { + return nullptr; + } + + std::filesystem::path entryPath = std::filesystem::path(config.cacheDir) / diskCacheId(key); + std::filesystem::path metaPath = entryPath / "meta.json"; + std::filesystem::path dataPath = entryPath / "data.zarr"; + if (!std::filesystem::exists(metaPath) || !std::filesystem::exists(dataPath)) { + return nullptr; + } + + nlohmann::json meta; + try { + std::ifstream metaFile(metaPath.string()); + metaFile >> meta; + } catch (...) { + return nullptr; + } + + if (!meta.contains("key") || meta["key"].get() != keyToString(key)) { + return nullptr; + } + + if (!meta.contains("sizeX") || !meta.contains("sizeY") || !meta.contains("sizeZ") || !meta.contains("sizeC")) { + return nullptr; + } + + std::uint32_t sizeX = meta["sizeX"].get(); + std::uint32_t sizeY = meta["sizeY"].get(); + std::uint32_t sizeZ = meta["sizeZ"].get(); + std::uint32_t sizeC = meta["sizeC"].get(); + std::uint32_t bpp = meta.value("bpp", static_cast(ImageXYZC::IN_MEMORY_BPP)); + float sx = meta.value("physicalSizeX", 1.0f); + float sy = meta.value("physicalSizeY", 1.0f); + float sz = meta.value("physicalSizeZ", 1.0f); + std::string spatialUnits = meta.value("spatialUnits", std::string("units")); + + std::uint64_t bytes = static_cast(sizeX) * static_cast(sizeY) * + static_cast(sizeZ) * static_cast(sizeC) * + static_cast(bpp / 8); + if (bytes == 0) { + return nullptr; + } + + std::unique_ptr data(new uint8_t[bytes]); + + auto openFuture = + tensorstore::Open({ { "driver", "zarr3" }, { "kvstore", { { "driver", "file" }, { "path", dataPath.string() } } } }, + tensorstore::OpenMode::open, + tensorstore::ReadWriteMode::read); + auto result = openFuture.result(); + if (!result.ok()) { + return nullptr; + } + + auto store = result.value(); + std::vector shape = { sizeC, sizeZ, sizeY, sizeX }; + auto arr = tensorstore::Array(reinterpret_cast(data.get()), shape); + auto readResult = tensorstore::Read(store, tensorstore::UnownedToShared(arr)).result(); + if (!readResult.ok()) { + return nullptr; + } + + ImageXYZC* image = new ImageXYZC(sizeX, sizeY, sizeZ, sizeC, bpp, data.release(), sx, sy, sz, spatialUnits); + std::shared_ptr sharedImage(image); + + if (meta.contains("channelNames")) { + std::vector channelNames; + for (auto& item : meta["channelNames"]) { + channelNames.push_back(item.get()); + } + image->setChannelNames(channelNames); + } + + meta["lastAccess"] = nowMillis(); + try { + std::ofstream metaOut(metaPath.string(), std::ios::trunc); + metaOut << meta.dump(2); + } catch (...) { + // best effort + } + + { + std::lock_guard lock(m_mutex); + auto it = m_diskEntries.find(diskCacheId(key)); + if (it != m_diskEntries.end()) { + it->second.lastAccess = meta["lastAccess"].get(); + } + } + + return sharedImage; +} + +void +CacheManager::storeToDisk(const CacheKey& key, const std::shared_ptr& image, const CacheConfig& config) +{ + if (!image || !config.enableDisk || config.cacheDir.empty()) { + return; + } + + std::filesystem::path entryPath = std::filesystem::path(config.cacheDir) / diskCacheId(key); + std::filesystem::path dataPath = entryPath / "data.zarr"; + std::filesystem::path metaPath = entryPath / "meta.json"; + std::error_code ec; + std::filesystem::create_directories(entryPath, ec); + + std::uint32_t sizeX = static_cast(image->sizeX()); + std::uint32_t sizeY = static_cast(image->sizeY()); + std::uint32_t sizeZ = static_cast(image->sizeZ()); + std::uint32_t sizeC = static_cast(image->sizeC()); + std::uint32_t bpp = static_cast(ImageXYZC::IN_MEMORY_BPP); + + std::vector shape = { sizeC, sizeZ, sizeY, sizeX }; + std::vector chunkShape = { 1, + std::min(16, sizeZ), + std::min(256, sizeY), + std::min(256, sizeX) }; + + nlohmann::json schema = { { "dtype", "uint16" }, + { "domain", { { "shape", shape } } }, + { "chunk_layout", + { { "read_chunk", { { "shape", chunkShape } } }, + { "write_chunk", { { "shape", chunkShape } } } } } }; + + auto openFuture = tensorstore::Open({ { "driver", "zarr3" }, + { "kvstore", { { "driver", "file" }, { "path", dataPath.string() } } }, + { "schema", schema } }, + tensorstore::OpenMode::create | tensorstore::OpenMode::open, + tensorstore::ReadWriteMode::read_write); + auto result = openFuture.result(); + if (!result.ok()) { + return; + } + + auto store = result.value(); + auto arr = tensorstore::Array(reinterpret_cast(image->ptr()), shape); + auto writeResult = tensorstore::Write(arr, store).result(); + if (!writeResult.ok()) { + return; + } + + std::uint64_t bytes = estimateImageBytes(*image); + std::uint64_t accessTime = nowMillis(); + nlohmann::json meta = { { "key", keyToString(key) }, + { "sizeX", sizeX }, + { "sizeY", sizeY }, + { "sizeZ", sizeZ }, + { "sizeC", sizeC }, + { "bpp", bpp }, + { "physicalSizeX", image->physicalSizeX() }, + { "physicalSizeY", image->physicalSizeY() }, + { "physicalSizeZ", image->physicalSizeZ() }, + { "spatialUnits", image->spatialUnits() }, + { "channelNames", channelNamesFromImage(*image) }, + { "bytes", bytes }, + { "lastAccess", accessTime } }; + + try { + std::ofstream metaOut(metaPath.string(), std::ios::trunc); + metaOut << meta.dump(2); + } catch (...) { + return; + } + + { + std::lock_guard lock(m_mutex); + m_stats.diskWrites++; + DiskEntry entry; + entry.path = entryPath.string(); + entry.bytes = bytes; + entry.lastAccess = accessTime; + auto it = m_diskEntries.find(diskCacheId(key)); + if (it != m_diskEntries.end()) { + m_currentDiskBytes -= it->second.bytes; + } + m_diskEntries[diskCacheId(key)] = entry; + m_currentDiskBytes += bytes; + } + + evictDiskIfNeeded(config, 0); +} + +void +CacheManager::loadDiskIndex(const CacheConfig& config) +{ + if (config.cacheDir.empty() || !config.enableDisk) { + return; + } + + std::filesystem::path root(config.cacheDir); + std::error_code ec; + std::filesystem::create_directories(root, ec); + if (!std::filesystem::exists(root)) { + return; + } + + std::unordered_map entries; + std::uint64_t totalBytes = 0; + + for (const auto& dirEntry : std::filesystem::directory_iterator(root)) { + if (!dirEntry.is_directory()) { + continue; + } + + std::filesystem::path metaPath = dirEntry.path() / "meta.json"; + if (!std::filesystem::exists(metaPath)) { + continue; + } + + try { + nlohmann::json meta; + std::ifstream metaFile(metaPath.string()); + metaFile >> meta; + if (!meta.contains("lastAccess")) { + continue; + } + + DiskEntry entry; + entry.path = dirEntry.path().string(); + entry.lastAccess = meta["lastAccess"].get(); + if (meta.contains("bytes")) { + entry.bytes = meta["bytes"].get(); + } else { + entry.bytes = directorySizeBytes(entry.path); + } + + std::string id = dirEntry.path().filename().string(); + entries[id] = entry; + totalBytes += entry.bytes; + } catch (...) { + continue; + } + } + + std::lock_guard lock(m_mutex); + m_diskEntries = std::move(entries); + m_currentDiskBytes = totalBytes; +} + +void +CacheManager::evictDiskIfNeeded(const CacheConfig& config, std::uint64_t incomingBytes) +{ + if (!config.enableDisk || config.maxDiskBytes == 0) { + return; + } + + while (true) { + std::string oldestKey; + std::uint64_t oldestAccess = std::numeric_limits::max(); + std::uint64_t currentBytes = 0; + { + std::lock_guard lock(m_mutex); + currentBytes = m_currentDiskBytes; + if ((currentBytes + incomingBytes) <= config.maxDiskBytes || m_diskEntries.empty()) { + break; + } + for (const auto& item : m_diskEntries) { + if (item.second.lastAccess < oldestAccess) { + oldestAccess = item.second.lastAccess; + oldestKey = item.first; + } + } + } + + if (oldestKey.empty()) { + break; + } + + std::string removePath; + std::uint64_t removedBytes = 0; + { + std::lock_guard lock(m_mutex); + auto it = m_diskEntries.find(oldestKey); + if (it == m_diskEntries.end()) { + continue; + } + removePath = it->second.path; + removedBytes = it->second.bytes; + m_diskEntries.erase(it); + if (m_currentDiskBytes >= removedBytes) { + m_currentDiskBytes -= removedBytes; + } else { + m_currentDiskBytes = 0; + } + } + + if (!removePath.empty()) { + std::error_code ec; + std::filesystem::remove_all(removePath, ec); + } + } +} + +std::uint64_t +CacheManager::directorySizeBytes(const std::string& path) const +{ + std::uint64_t total = 0; + std::error_code ec; + for (const auto& entry : std::filesystem::recursive_directory_iterator(path, ec)) { + if (entry.is_regular_file()) { + total += static_cast(entry.file_size()); + } + } + return total; +} + +} // namespace renderlib diff --git a/renderlib/CacheManager.h b/renderlib/CacheManager.h new file mode 100644 index 000000000..13675418d --- /dev/null +++ b/renderlib/CacheManager.h @@ -0,0 +1,110 @@ +#pragma once + +#include "CacheConfig.h" +#include "IFileReader.h" + +#include +#include +#include +#include +#include +#include +#include + +class ImageXYZC; + +namespace renderlib { + +struct CacheKey +{ + std::string filepath; + std::string subpath; + int scene = 0; + std::uint32_t time = 0; + std::vector channels; + std::uint32_t minx = 0; + std::uint32_t maxx = 0; + std::uint32_t miny = 0; + std::uint32_t maxy = 0; + std::uint32_t minz = 0; + std::uint32_t maxz = 0; + bool isImageSequence = false; + + bool operator==(const CacheKey& other) const; +}; + +struct CacheKeyHash +{ + std::size_t operator()(const CacheKey& key) const; +}; + +class CacheManager +{ +public: + struct CacheStats + { + std::uint64_t ramHits = 0; + std::uint64_t diskHits = 0; + std::uint64_t misses = 0; + std::uint64_t diskWrites = 0; + }; + + static CacheManager& instance(); + + void setConfig(const CacheConfig& config); + CacheConfig getConfig() const; + + std::shared_ptr findImage(const LoadSpec& loadSpec); + void storeImage(const LoadSpec& loadSpec, const std::shared_ptr& image); + void clear(); + void clearDiskCache(); + + CacheStats getStats() const; + void resetStats(); + +private: + CacheManager() = default; + + CacheKey makeKey(const LoadSpec& loadSpec) const; + std::string keyToString(const CacheKey& key) const; + std::string diskCacheId(const CacheKey& key) const; + std::uint64_t estimateImageBytes(const ImageXYZC& image) const; + void touchEntry(std::list::iterator it); + void evictIfNeeded(std::uint64_t incomingBytes); + void storeImageInMemory(const CacheKey& key, const std::shared_ptr& image); + + std::shared_ptr loadFromDisk(const CacheKey& key, const CacheConfig& config); + void storeToDisk(const CacheKey& key, const std::shared_ptr& image, const CacheConfig& config); + void loadDiskIndex(const CacheConfig& config); + void evictDiskIfNeeded(const CacheConfig& config, std::uint64_t incomingBytes); + std::uint64_t directorySizeBytes(const std::string& path) const; + + mutable std::mutex m_mutex; + CacheConfig m_config; + std::uint64_t m_currentRamBytes = 0; + std::list m_lruKeys; + + struct CacheEntry + { + std::shared_ptr image; + std::uint64_t bytes = 0; + std::list::iterator lruIt; + }; + + std::unordered_map m_entries; + + struct DiskEntry + { + std::string path; + std::uint64_t bytes = 0; + std::uint64_t lastAccess = 0; + }; + + std::unordered_map m_diskEntries; + std::uint64_t m_currentDiskBytes = 0; + std::string m_diskIndexRoot; + + CacheStats m_stats; +}; + +} // namespace renderlib diff --git a/renderlib/command.cpp b/renderlib/command.cpp index ad486ed1a..9d43f136d 100644 --- a/renderlib/command.cpp +++ b/renderlib/command.cpp @@ -571,14 +571,7 @@ SetTimeCommand::execute(ExecutionContext* c) std::shared_ptr image; try { - std::unique_ptr reader(FileReader::getReader(loadSpec.filepath, loadSpec.isImageSequence)); - if (!reader) { - LOG_ERROR << "Could not find a reader for file " << loadSpec.filepath; - image = nullptr; - return; - } - - image = reader->loadFromFile(loadSpec); + image = FileReader::loadAndCache(loadSpec); } catch (...) { LOG_ERROR << "Failed to load time " << m_data.m_time << " from file " << c->m_loadSpec.toString(); image = nullptr; @@ -678,7 +671,7 @@ LoadDataCommand::execute(ExecutionContext* c) VolumeDimensions dims = reader->loadDimensions(m_data.m_path, m_data.m_scene); - std::shared_ptr image = reader->loadFromFile(c->m_loadSpec); + std::shared_ptr image = FileReader::loadAndCache(c->m_loadSpec); if (!image) { return; } diff --git a/renderlib/io/FileReader.cpp b/renderlib/io/FileReader.cpp index 5aa33839b..71b5e5e95 100644 --- a/renderlib/io/FileReader.cpp +++ b/renderlib/io/FileReader.cpp @@ -7,12 +7,10 @@ #include "FileReaderZarr.h" #include "ImageXYZC.h" #include "Logging.h" +#include "CacheManager.h" #include #include -#include - -std::map> FileReader::sPreloadedImageCache; // return file extension as lowercase std::string @@ -68,10 +66,9 @@ FileReader::getReader(const std::string& filepath, bool isImageSequence) std::shared_ptr FileReader::loadAndCache(const LoadSpec& loadSpec) { - // check cache first of all. - auto cached = sPreloadedImageCache.find(loadSpec.filepath); - if (cached != sPreloadedImageCache.end()) { - return cached->second; + auto cached = renderlib::CacheManager::instance().findImage(loadSpec); + if (cached) { + return cached; } VolumeDimensions dims; @@ -80,7 +77,7 @@ FileReader::loadAndCache(const LoadSpec& loadSpec) std::string filepath = loadSpec.filepath; - std::unique_ptr reader(FileReader::getReader(filepath)); + std::unique_ptr reader(FileReader::getReader(filepath, loadSpec.isImageSequence)); if (!reader) { LOG_ERROR << "Could not find a reader for file " << filepath; return nullptr; @@ -89,7 +86,7 @@ FileReader::loadAndCache(const LoadSpec& loadSpec) image = reader->loadFromFile(loadSpec); if (image) { - sPreloadedImageCache[filepath] = image; + renderlib::CacheManager::instance().storeImage(loadSpec, image); } return image; @@ -106,9 +103,11 @@ FileReader::loadFromArray_4D(uint8_t* dataArray, bool addToCache) { // check cache first of all. - auto cached = sPreloadedImageCache.find(name); - if (cached != sPreloadedImageCache.end()) { - return cached->second; + LoadSpec cacheSpec; + cacheSpec.filepath = name; + auto cached = renderlib::CacheManager::instance().findImage(cacheSpec); + if (cached) { + return cached; } // assume data is in CZYX order: @@ -145,7 +144,9 @@ FileReader::loadFromArray_4D(uint8_t* dataArray, std::shared_ptr sharedImage(im); if (addToCache) { - sPreloadedImageCache[name] = sharedImage; + LoadSpec newSpec; + newSpec.filepath = name; + renderlib::CacheManager::instance().storeImage(newSpec, sharedImage); } return sharedImage; } diff --git a/renderlib/io/FileReader.h b/renderlib/io/FileReader.h index 1effb5089..26c20dfcd 100644 --- a/renderlib/io/FileReader.h +++ b/renderlib/io/FileReader.h @@ -2,7 +2,6 @@ #include "IFileReader.h" -#include #include #include #include @@ -31,5 +30,4 @@ class FileReader bool addToCache = false); private: - static std::map> sPreloadedImageCache; }; From f0055279dba8edf4d96376622b6f84ede5a3a61e Mon Sep 17 00:00:00 2001 From: toloudis Date: Thu, 19 Feb 2026 10:01:01 -0800 Subject: [PATCH 02/31] fix compilation --- agave_app/CacheSettings.cpp | 78 +++++++++++++++++++------------------ agave_app/CacheSettings.h | 8 +--- agave_app/agaveGui.cpp | 2 +- renderlib/CacheConfig.h | 4 -- renderlib/CacheManager.cpp | 17 ++++---- renderlib/CacheManager.h | 4 -- renderlib/io/FileReader.cpp | 8 ++-- 7 files changed, 55 insertions(+), 66 deletions(-) diff --git a/agave_app/CacheSettings.cpp b/agave_app/CacheSettings.cpp index aedbf6714..55715c193 100644 --- a/agave_app/CacheSettings.cpp +++ b/agave_app/CacheSettings.cpp @@ -19,6 +19,44 @@ #include #endif +namespace { + +::CacheConfig +toRenderlibConfig(const CacheSettings& settings, const CacheSettingsData& data) +{ + CacheSettingsData normalized = data; + if (normalized.cacheDir.empty()) { + normalized.cacheDir = settings.defaultSettings().cacheDir; + } + + ::CacheConfig config; + config.enabled = normalized.enabled; + config.enableDisk = normalized.enableDisk; + config.cacheDir = normalized.cacheDir; + + std::uint64_t availableMem = settings.availableMemoryBytes(); + if (availableMem > 0) { + config.maxRamBytes = std::min(normalized.maxRamBytes, availableMem); + } else { + config.maxRamBytes = normalized.maxRamBytes; + } + + std::uint64_t availableDisk = settings.availableDiskBytes(normalized.cacheDir); + if (availableDisk > 0) { + config.maxDiskBytes = std::min(normalized.maxDiskBytes, availableDisk); + } else { + config.maxDiskBytes = normalized.maxDiskBytes; + } + + if (!config.enableDisk) { + config.maxDiskBytes = 0; + } + + return config; +} + +} // namespace + CacheSettings::CacheSettings() {} CacheSettingsData @@ -108,44 +146,10 @@ CacheSettings::save(const CacheSettingsData& data) const return true; } -renderlib::CacheConfig -CacheSettings::toRenderlibConfig(const CacheSettingsData& data) const -{ - CacheSettingsData normalized = data; - if (normalized.cacheDir.empty()) { - normalized.cacheDir = defaultSettings().cacheDir; - } - - renderlib::CacheConfig config; - config.enabled = normalized.enabled; - config.enableDisk = normalized.enableDisk; - config.cacheDir = normalized.cacheDir; - - std::uint64_t availableMem = availableMemoryBytes(); - if (availableMem > 0) { - config.maxRamBytes = std::min(normalized.maxRamBytes, availableMem); - } else { - config.maxRamBytes = normalized.maxRamBytes; - } - - std::uint64_t availableDisk = availableDiskBytes(normalized.cacheDir); - if (availableDisk > 0) { - config.maxDiskBytes = std::min(normalized.maxDiskBytes, availableDisk); - } else { - config.maxDiskBytes = normalized.maxDiskBytes; - } - - if (!config.enableDisk) { - config.maxDiskBytes = 0; - } - - return config; -} - void CacheSettings::applyToRenderlib(const CacheSettingsData& data) const { - renderlib::CacheConfig config = toRenderlibConfig(data); + ::CacheConfig config = toRenderlibConfig(*this, data); if (config.enableDisk && !config.cacheDir.empty()) { if (!canWriteCacheDir(config.cacheDir)) { LOG_WARNING << "Cache disk disabled: cache directory not writable: " << config.cacheDir; @@ -156,8 +160,8 @@ CacheSettings::applyToRenderlib(const CacheSettingsData& data) const LOG_INFO << "Cache config: enabled=" << (config.enabled ? 1 : 0) << " ram_bytes=" << config.maxRamBytes << " disk_enabled=" << (config.enableDisk ? 1 : 0) << " disk_bytes=" << config.maxDiskBytes << " cache_dir=" << config.cacheDir; - renderlib::CacheManager::instance().setConfig(config); - auto applied = renderlib::CacheManager::instance().getConfig(); + CacheManager::instance().setConfig(config); + auto applied = CacheManager::instance().getConfig(); LOG_INFO << "Cache config applied: enabled=" << (applied.enabled ? 1 : 0) << " ram_bytes=" << applied.maxRamBytes << " disk_enabled=" << (applied.enableDisk ? 1 : 0) << " disk_bytes=" << applied.maxDiskBytes << " cache_dir=" << applied.cacheDir; diff --git a/agave_app/CacheSettings.h b/agave_app/CacheSettings.h index 838852e8e..adb4b0325 100644 --- a/agave_app/CacheSettings.h +++ b/agave_app/CacheSettings.h @@ -3,10 +3,6 @@ #include #include -namespace renderlib { -struct CacheConfig; -} - struct CacheSettingsData { bool enabled = true; @@ -24,14 +20,14 @@ class CacheSettings CacheSettingsData load(); bool save(const CacheSettingsData& data) const; - renderlib::CacheConfig toRenderlibConfig(const CacheSettingsData& data) const; void applyToRenderlib(const CacheSettingsData& data) const; CacheSettingsData defaultSettings() const; std::string configPath() const; -private: std::uint64_t availableMemoryBytes() const; std::uint64_t availableDiskBytes(const std::string& path) const; + +private: bool canWriteCacheDir(const std::string& path) const; }; diff --git a/agave_app/agaveGui.cpp b/agave_app/agaveGui.cpp index 824af6927..bba47cd94 100644 --- a/agave_app/agaveGui.cpp +++ b/agave_app/agaveGui.cpp @@ -109,7 +109,7 @@ agaveGui::agaveGui(QWidget* parent) m_cacheSettings.applyToRenderlib(data); }); connect(m_cacheSettingsDockWidget->widget()->clearDiskButton(), &QPushButton::clicked, this, [this]() { - renderlib::CacheManager::instance().clearDiskCache(); + CacheManager::instance().clearDiskCache(); }); m_tabs = new QTabWidget(this); diff --git a/renderlib/CacheConfig.h b/renderlib/CacheConfig.h index 7697dae00..b777cfa18 100644 --- a/renderlib/CacheConfig.h +++ b/renderlib/CacheConfig.h @@ -3,8 +3,6 @@ #include #include -namespace renderlib { - struct CacheConfig { bool enabled = false; @@ -13,5 +11,3 @@ struct CacheConfig std::uint64_t maxDiskBytes = 0; std::string cacheDir; }; - -} // namespace renderlib diff --git a/renderlib/CacheManager.cpp b/renderlib/CacheManager.cpp index b619161e3..ece17fc2d 100644 --- a/renderlib/CacheManager.cpp +++ b/renderlib/CacheManager.cpp @@ -5,6 +5,7 @@ #include "tensorstore/array.h" #include "tensorstore/context.h" +#include "tensorstore/open.h" #include "tensorstore/tensorstore.h" // must include after tensorstore so that tensorstore picks up its own internal json impl @@ -18,8 +19,6 @@ #include #include -namespace renderlib { - namespace { inline void @@ -480,11 +479,11 @@ CacheManager::storeToDisk(const CacheKey& key, const std::shared_ptr& { { "read_chunk", { { "shape", chunkShape } } }, { "write_chunk", { { "shape", chunkShape } } } } } }; - auto openFuture = tensorstore::Open({ { "driver", "zarr3" }, - { "kvstore", { { "driver", "file" }, { "path", dataPath.string() } } }, - { "schema", schema } }, - tensorstore::OpenMode::create | tensorstore::OpenMode::open, - tensorstore::ReadWriteMode::read_write); + auto openFuture = tensorstore::Open( + { { "driver", "zarr3" }, + { "kvstore", { { "driver", "file" }, { "path", dataPath.string() } } }, + { "schema", schema } }, + tensorstore::OpenMode::create | tensorstore::OpenMode::open); auto result = openFuture.result(); if (!result.ok()) { return; @@ -492,7 +491,7 @@ CacheManager::storeToDisk(const CacheKey& key, const std::shared_ptr& auto store = result.value(); auto arr = tensorstore::Array(reinterpret_cast(image->ptr()), shape); - auto writeResult = tensorstore::Write(arr, store).result(); + auto writeResult = tensorstore::Write(tensorstore::UnownedToShared(arr), store).result(); if (!writeResult.ok()) { return; } @@ -661,5 +660,3 @@ CacheManager::directorySizeBytes(const std::string& path) const } return total; } - -} // namespace renderlib diff --git a/renderlib/CacheManager.h b/renderlib/CacheManager.h index 13675418d..41ee660e7 100644 --- a/renderlib/CacheManager.h +++ b/renderlib/CacheManager.h @@ -13,8 +13,6 @@ class ImageXYZC; -namespace renderlib { - struct CacheKey { std::string filepath; @@ -106,5 +104,3 @@ class CacheManager CacheStats m_stats; }; - -} // namespace renderlib diff --git a/renderlib/io/FileReader.cpp b/renderlib/io/FileReader.cpp index 71b5e5e95..77bf1d97d 100644 --- a/renderlib/io/FileReader.cpp +++ b/renderlib/io/FileReader.cpp @@ -66,7 +66,7 @@ FileReader::getReader(const std::string& filepath, bool isImageSequence) std::shared_ptr FileReader::loadAndCache(const LoadSpec& loadSpec) { - auto cached = renderlib::CacheManager::instance().findImage(loadSpec); + auto cached = CacheManager::instance().findImage(loadSpec); if (cached) { return cached; } @@ -86,7 +86,7 @@ FileReader::loadAndCache(const LoadSpec& loadSpec) image = reader->loadFromFile(loadSpec); if (image) { - renderlib::CacheManager::instance().storeImage(loadSpec, image); + CacheManager::instance().storeImage(loadSpec, image); } return image; @@ -105,7 +105,7 @@ FileReader::loadFromArray_4D(uint8_t* dataArray, // check cache first of all. LoadSpec cacheSpec; cacheSpec.filepath = name; - auto cached = renderlib::CacheManager::instance().findImage(cacheSpec); + auto cached = CacheManager::instance().findImage(cacheSpec); if (cached) { return cached; } @@ -146,7 +146,7 @@ FileReader::loadFromArray_4D(uint8_t* dataArray, if (addToCache) { LoadSpec newSpec; newSpec.filepath = name; - renderlib::CacheManager::instance().storeImage(newSpec, sharedImage); + CacheManager::instance().storeImage(newSpec, sharedImage); } return sharedImage; } From 218e9f623495c42e9dae2f1e04997e7b1c72d25b Mon Sep 17 00:00:00 2001 From: toloudis Date: Thu, 19 Feb 2026 10:21:55 -0800 Subject: [PATCH 03/31] use cache on time slider change --- agave_app/TimelineDockWidget.cpp | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/agave_app/TimelineDockWidget.cpp b/agave_app/TimelineDockWidget.cpp index d4050b060..8cee28d7d 100644 --- a/agave_app/TimelineDockWidget.cpp +++ b/agave_app/TimelineDockWidget.cpp @@ -82,16 +82,8 @@ QTimelineWidget::OnTimeChanged(int newTime) LoadSpec loadSpec = m_loadSpec; loadSpec.time = newTime; - if (!m_reader) { - m_reader = std::shared_ptr(FileReader::getReader(loadSpec.filepath, loadSpec.isImageSequence)); - if (!m_reader) { - LOG_ERROR << "Could not find a reader for file " << loadSpec.filepath; - return; - } - } - QApplication::setOverrideCursor(QCursor(Qt::WaitCursor)); - std::shared_ptr image = m_reader->loadFromFile(loadSpec); + std::shared_ptr image = FileReader::loadAndCache(loadSpec); QApplication::restoreOverrideCursor(); if (!image) { // TODO FIXME if we fail to set the new time, then reset the GUI to previous time From 9aa11750588b6e757f6dcc10addbdb4b4e2955fc Mon Sep 17 00:00:00 2001 From: toloudis Date: Sat, 2 May 2026 14:18:41 -0700 Subject: [PATCH 04/31] add unit tests for cache mgr --- test/CMakeLists.txt | 1 + test/test_cacheManager.cpp | 241 +++++++++++++++++++++++++++++++++++++ 2 files changed, 242 insertions(+) create mode 100644 test/test_cacheManager.cpp diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 42dd01e20..32b210b81 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -14,6 +14,7 @@ target_include_directories(agave_test PUBLIC "${glm_SOURCE_DIR}" ) target_sources(agave_test PRIVATE + "${CMAKE_CURRENT_SOURCE_DIR}/test_cacheManager.cpp" "${CMAKE_CURRENT_SOURCE_DIR}/test_commands.cpp" "${CMAKE_CURRENT_SOURCE_DIR}/test_histogram.cpp" "${CMAKE_CURRENT_SOURCE_DIR}/test_main.cpp" diff --git a/test/test_cacheManager.cpp b/test/test_cacheManager.cpp new file mode 100644 index 000000000..4903fc4b7 --- /dev/null +++ b/test/test_cacheManager.cpp @@ -0,0 +1,241 @@ +#include + +#include "renderlib/CacheConfig.h" +#include "renderlib/CacheManager.h" +#include "renderlib/IFileReader.h" +#include "renderlib/ImageXYZC.h" + +#include +#include +#include +#include + +namespace { + +// Bytes per pixel that CacheManager uses when estimating an image's RAM cost. +// IN_MEMORY_BPP is a static const member without an out-of-class definition, +// so we copy its value into a constexpr to avoid ODR-use at the link step. +constexpr std::uint32_t kInMemoryBpp = 16; +constexpr std::uint64_t kBytesPerPixel = kInMemoryBpp / 8; +static_assert(kInMemoryBpp == ImageXYZC::IN_MEMORY_BPP, "IN_MEMORY_BPP changed"); + +// Build a minimal in-memory ImageXYZC for cache testing. Memory is owned by the +// returned ImageXYZC (it deletes m_data in its destructor). +std::shared_ptr +makeImage(uint32_t x, uint32_t y, uint32_t z, uint32_t c) +{ + const std::uint64_t bytes = + static_cast(x) * y * z * c * kBytesPerPixel; + auto* data = new uint8_t[bytes]; + std::memset(data, 0, bytes); + return std::make_shared(x, y, z, c, kInMemoryBpp, data, 1.0f, 1.0f, 1.0f, "units"); +} + +LoadSpec +makeSpec(const std::string& filepath, uint32_t time = 0) +{ + LoadSpec s; + s.filepath = filepath; + s.time = time; + return s; +} + +std::uint64_t +imageBytes(uint32_t x, uint32_t y, uint32_t z, uint32_t c) +{ + return static_cast(x) * y * z * c * kBytesPerPixel; +} + +// Reset the singleton CacheManager to a known state for each test. +void +resetCache() +{ + CacheManager::instance().clear(); + CacheManager::instance().resetStats(); +} + +CacheConfig +ramOnlyConfig(std::uint64_t maxRamBytes) +{ + CacheConfig cfg; + cfg.enabled = true; + cfg.enableDisk = false; + cfg.maxRamBytes = maxRamBytes; + cfg.maxDiskBytes = 0; + return cfg; +} + +} // namespace + +TEST_CASE("CacheManager respects RAM limit and evicts LRU entries", "[cache]") +{ + // Each 4x4x4x1 image is 4*4*4*1*2 = 128 bytes. + const std::uint64_t oneImage = imageBytes(4, 4, 4, 1); + + SECTION("Store and retrieve a single image (RAM hit)") + { + resetCache(); + CacheManager::instance().setConfig(ramOnlyConfig(oneImage * 4)); + + auto img = makeImage(4, 4, 4, 1); + auto spec = makeSpec("a"); + + CacheManager::instance().storeImage(spec, img); + auto retrieved = CacheManager::instance().findImage(spec); + + REQUIRE(retrieved != nullptr); + REQUIRE(retrieved.get() == img.get()); + + auto stats = CacheManager::instance().getStats(); + REQUIRE(stats.ramHits == 1); + REQUIRE(stats.misses == 0); + } + + SECTION("Miss returns null and increments miss counter") + { + resetCache(); + CacheManager::instance().setConfig(ramOnlyConfig(oneImage * 4)); + + auto retrieved = CacheManager::instance().findImage(makeSpec("missing")); + REQUIRE(retrieved == nullptr); + REQUIRE(CacheManager::instance().getStats().misses == 1); + } + + SECTION("Cache stays under maxRamBytes when limit is reached") + { + resetCache(); + // Limit is exactly 2 images. Storing a 3rd must evict. + CacheManager::instance().setConfig(ramOnlyConfig(oneImage * 2)); + + CacheManager::instance().storeImage(makeSpec("a"), makeImage(4, 4, 4, 1)); + CacheManager::instance().storeImage(makeSpec("b"), makeImage(4, 4, 4, 1)); + CacheManager::instance().storeImage(makeSpec("c"), makeImage(4, 4, 4, 1)); + + // "a" was the least recently used and must be evicted. + REQUIRE(CacheManager::instance().findImage(makeSpec("a")) == nullptr); + REQUIRE(CacheManager::instance().findImage(makeSpec("b")) != nullptr); + REQUIRE(CacheManager::instance().findImage(makeSpec("c")) != nullptr); + } + + SECTION("LRU ordering is updated on access (touched entry survives eviction)") + { + resetCache(); + CacheManager::instance().setConfig(ramOnlyConfig(oneImage * 2)); + + CacheManager::instance().storeImage(makeSpec("a"), makeImage(4, 4, 4, 1)); + CacheManager::instance().storeImage(makeSpec("b"), makeImage(4, 4, 4, 1)); + + // Touch "a" so that "b" becomes least recently used. + REQUIRE(CacheManager::instance().findImage(makeSpec("a")) != nullptr); + + CacheManager::instance().storeImage(makeSpec("c"), makeImage(4, 4, 4, 1)); + + REQUIRE(CacheManager::instance().findImage(makeSpec("a")) != nullptr); + REQUIRE(CacheManager::instance().findImage(makeSpec("b")) == nullptr); + REQUIRE(CacheManager::instance().findImage(makeSpec("c")) != nullptr); + } + + SECTION("Storing many images keeps total RAM usage bounded") + { + resetCache(); + const std::uint64_t cap = oneImage * 3; + CacheManager::instance().setConfig(ramOnlyConfig(cap)); + + for (int i = 0; i < 20; ++i) { + CacheManager::instance().storeImage(makeSpec("file_" + std::to_string(i)), makeImage(4, 4, 4, 1)); + } + + // Count how many of the 20 inserts are still resident. With a cap of 3 + // images, no more than 3 can be present at once. + CacheManager::instance().resetStats(); + int present = 0; + for (int i = 0; i < 20; ++i) { + if (CacheManager::instance().findImage(makeSpec("file_" + std::to_string(i)))) { + present++; + } + } + REQUIRE(present <= 3); + // The most recently inserted entries should still be in the cache. + REQUIRE(present >= 1); + REQUIRE(CacheManager::instance().findImage(makeSpec("file_19")) != nullptr); + } + + SECTION("Image larger than maxRamBytes is not stored") + { + resetCache(); + // Cap at less than the size of one image. + CacheManager::instance().setConfig(ramOnlyConfig(oneImage / 2)); + + CacheManager::instance().storeImage(makeSpec("too_big"), makeImage(4, 4, 4, 1)); + REQUIRE(CacheManager::instance().findImage(makeSpec("too_big")) == nullptr); + } + + SECTION("Re-storing the same key replaces the entry without exceeding the limit") + { + resetCache(); + CacheManager::instance().setConfig(ramOnlyConfig(oneImage * 2)); + + auto first = makeImage(4, 4, 4, 1); + auto second = makeImage(4, 4, 4, 1); + auto spec = makeSpec("a"); + + CacheManager::instance().storeImage(spec, first); + CacheManager::instance().storeImage(spec, second); + + // Fill the cache up to the limit with another entry. If the duplicate + // key had double-counted bytes, this would evict "a". + CacheManager::instance().storeImage(makeSpec("b"), makeImage(4, 4, 4, 1)); + + auto retrieved = CacheManager::instance().findImage(spec); + REQUIRE(retrieved != nullptr); + REQUIRE(retrieved.get() == second.get()); + REQUIRE(CacheManager::instance().findImage(makeSpec("b")) != nullptr); + } + + SECTION("Disabling the cache clears all entries") + { + resetCache(); + CacheManager::instance().setConfig(ramOnlyConfig(oneImage * 4)); + CacheManager::instance().storeImage(makeSpec("a"), makeImage(4, 4, 4, 1)); + REQUIRE(CacheManager::instance().findImage(makeSpec("a")) != nullptr); + + CacheConfig disabled; + disabled.enabled = false; + CacheManager::instance().setConfig(disabled); + + REQUIRE(CacheManager::instance().findImage(makeSpec("a")) == nullptr); + } + + SECTION("Shrinking the limit evicts existing entries") + { + resetCache(); + CacheManager::instance().setConfig(ramOnlyConfig(oneImage * 4)); + CacheManager::instance().storeImage(makeSpec("a"), makeImage(4, 4, 4, 1)); + CacheManager::instance().storeImage(makeSpec("b"), makeImage(4, 4, 4, 1)); + CacheManager::instance().storeImage(makeSpec("c"), makeImage(4, 4, 4, 1)); + + // Reconfigure to a smaller limit; oldest entries must be evicted. + CacheManager::instance().setConfig(ramOnlyConfig(oneImage)); + + int present = 0; + for (const char* name : { "a", "b", "c" }) { + if (CacheManager::instance().findImage(makeSpec(name))) { + present++; + } + } + REQUIRE(present <= 1); + } + + SECTION("clear() empties the cache") + { + resetCache(); + CacheManager::instance().setConfig(ramOnlyConfig(oneImage * 4)); + CacheManager::instance().storeImage(makeSpec("a"), makeImage(4, 4, 4, 1)); + CacheManager::instance().storeImage(makeSpec("b"), makeImage(4, 4, 4, 1)); + + CacheManager::instance().clear(); + + REQUIRE(CacheManager::instance().findImage(makeSpec("a")) == nullptr); + REQUIRE(CacheManager::instance().findImage(makeSpec("b")) == nullptr); + } +} From 74130322eeb991202740defd33a68af282cc4e5a Mon Sep 17 00:00:00 2001 From: toloudis Date: Sun, 3 May 2026 13:54:55 -0700 Subject: [PATCH 05/31] improve cache tests --- renderlib/ImageXYZC.h | 4 +-- test/test_cacheManager.cpp | 57 ++++++++++++++++++++++++++++++++------ 2 files changed, 51 insertions(+), 10 deletions(-) diff --git a/renderlib/ImageXYZC.h b/renderlib/ImageXYZC.h index d0fa4e483..67f21433a 100644 --- a/renderlib/ImageXYZC.h +++ b/renderlib/ImageXYZC.h @@ -69,9 +69,9 @@ class ImageXYZC { public: // how many channels to enable on first load by default - static const int FIRST_N_CHANNELS = 1; + static constexpr int FIRST_N_CHANNELS = 1; - static const size_t IN_MEMORY_BPP = 16; + static constexpr size_t IN_MEMORY_BPP = 16; ImageXYZC(uint32_t x, uint32_t y, uint32_t z, diff --git a/test/test_cacheManager.cpp b/test/test_cacheManager.cpp index 4903fc4b7..1187e96e8 100644 --- a/test/test_cacheManager.cpp +++ b/test/test_cacheManager.cpp @@ -13,22 +13,17 @@ namespace { // Bytes per pixel that CacheManager uses when estimating an image's RAM cost. -// IN_MEMORY_BPP is a static const member without an out-of-class definition, -// so we copy its value into a constexpr to avoid ODR-use at the link step. -constexpr std::uint32_t kInMemoryBpp = 16; -constexpr std::uint64_t kBytesPerPixel = kInMemoryBpp / 8; -static_assert(kInMemoryBpp == ImageXYZC::IN_MEMORY_BPP, "IN_MEMORY_BPP changed"); +constexpr size_t kBytesPerPixel = ImageXYZC::IN_MEMORY_BPP / 8; // Build a minimal in-memory ImageXYZC for cache testing. Memory is owned by the // returned ImageXYZC (it deletes m_data in its destructor). std::shared_ptr makeImage(uint32_t x, uint32_t y, uint32_t z, uint32_t c) { - const std::uint64_t bytes = - static_cast(x) * y * z * c * kBytesPerPixel; + const std::uint64_t bytes = static_cast(x) * y * z * c * kBytesPerPixel; auto* data = new uint8_t[bytes]; std::memset(data, 0, bytes); - return std::make_shared(x, y, z, c, kInMemoryBpp, data, 1.0f, 1.0f, 1.0f, "units"); + return std::make_shared(x, y, z, c, ImageXYZC::IN_MEMORY_BPP, data, 1.0f, 1.0f, 1.0f, "units"); } LoadSpec @@ -226,6 +221,52 @@ TEST_CASE("CacheManager respects RAM limit and evicts LRU entries", "[cache]") REQUIRE(present <= 1); } + SECTION("Reducing cache size immediately evicts LRU entries to fit") + { + resetCache(); + // Fill the cache exactly to its 4-image cap. + CacheManager::instance().setConfig(ramOnlyConfig(oneImage * 4)); + CacheManager::instance().storeImage(makeSpec("a"), makeImage(4, 4, 4, 1)); + CacheManager::instance().storeImage(makeSpec("b"), makeImage(4, 4, 4, 1)); + CacheManager::instance().storeImage(makeSpec("c"), makeImage(4, 4, 4, 1)); + CacheManager::instance().storeImage(makeSpec("d"), makeImage(4, 4, 4, 1)); + + // All four are resident. LRU order (most recent first) is: d, c, b, a. + // Reset stats so the find() calls below count cleanly. + CacheManager::instance().resetStats(); + + // User shrinks the cache to hold only 2 images. Eviction must occur + // synchronously inside setConfig() — not lazily on the next store. + CacheManager::instance().setConfig(ramOnlyConfig(oneImage * 2)); + + // The two least recently used entries ("a" and "b") must be gone, and + // the two most recently used ("c" and "d") must still be present. + REQUIRE(CacheManager::instance().findImage(makeSpec("a")) == nullptr); + REQUIRE(CacheManager::instance().findImage(makeSpec("b")) == nullptr); + REQUIRE(CacheManager::instance().findImage(makeSpec("c")) != nullptr); + REQUIRE(CacheManager::instance().findImage(makeSpec("d")) != nullptr); + + auto stats = CacheManager::instance().getStats(); + REQUIRE(stats.misses == 2); + REQUIRE(stats.ramHits == 2); + } + + SECTION("Reducing cache size below a single image evicts everything") + { + resetCache(); + CacheManager::instance().setConfig(ramOnlyConfig(oneImage * 3)); + CacheManager::instance().storeImage(makeSpec("a"), makeImage(4, 4, 4, 1)); + CacheManager::instance().storeImage(makeSpec("b"), makeImage(4, 4, 4, 1)); + CacheManager::instance().storeImage(makeSpec("c"), makeImage(4, 4, 4, 1)); + + // Shrink below the size of a single entry — everything must go. + CacheManager::instance().setConfig(ramOnlyConfig(oneImage / 2)); + + REQUIRE(CacheManager::instance().findImage(makeSpec("a")) == nullptr); + REQUIRE(CacheManager::instance().findImage(makeSpec("b")) == nullptr); + REQUIRE(CacheManager::instance().findImage(makeSpec("c")) == nullptr); + } + SECTION("clear() empties the cache") { resetCache(); From abd8c8d21febd701c49f696797c0c68442737241 Mon Sep 17 00:00:00 2001 From: toloudis Date: Tue, 19 May 2026 15:53:24 -0700 Subject: [PATCH 06/31] give macos a fighting chance to get available memory --- .github/agents/reviewer.agent.md | 87 ++++++++++++++++++++++++++++++++ AGENTS.md | 2 +- agave_app/CacheSettings.cpp | 22 +++++++- 3 files changed, 108 insertions(+), 3 deletions(-) create mode 100644 .github/agents/reviewer.agent.md diff --git a/.github/agents/reviewer.agent.md b/.github/agents/reviewer.agent.md new file mode 100644 index 000000000..1347f1377 --- /dev/null +++ b/.github/agents/reviewer.agent.md @@ -0,0 +1,87 @@ +--- +description: Code reviewer for the AGAVE codebase. Reviews diffs and individual files for correctness, style, and architectural fit. +tools: + [ + "codebase", + "search", + "usages", + "problems", + "changes", + "githubRepo", + "editFiles", + ] +--- + +# AGAVE Code Reviewer + +You review code changes in the AGAVE repository (C++17/Qt6 desktop app + Python client + JS web client). Be concise, specific, and cite file paths with line numbers. + +## Review Scope + +When invoked, determine what to review: + +1. If the user names files or a PR, review those. +2. If the reviewer names a branch, review all changes in that branch relative to its base branch +3. Otherwise, inspect uncommitted/staged changes via the changes tool. +4. For each changed file, read enough surrounding context to understand intent — do not review lines in isolation. + +## What To Check + +### Architecture + +- `renderlib/` must not depend on Qt. Flag any `Q*` types, `QObject`, signals/slots, or Qt headers leaking into `renderlib/`. +- GUI logic belongs in `agave_app/`. Rendering, I/O, scene, and serialization belong in `renderlib/`. It is preferable to have anything not necessary for the GUI to be pushed down into `renderlib/`. +- New commands must be added to **all** of: `renderlib/command.h`, `renderlib/command.cpp`, `agave_app/commandBuffer.cpp`, `test/test_commands.cpp`, `agave_pyclient/agave_pyclient/commandbuffer.py`, `agave_pyclient/agave_pyclient/agave.py`, `webclient/src/commandbuffer.ts`, `webclient/src/agave.ts`. Verify the integer ID is unique and consistent, and that the argument list/order matches across all locations. + +### C++ Style + +- C++17 only — no later-standard features. +- Classes / methods / enums: `PascalCase`. Member variables: `m_` prefix. +- Headers use `#pragma once`. +- Include order: local project headers → standard C++ → third-party → Qt. +- Prefer `const`, `constexpr`, references, and RAII. Flag raw `new`/`delete` outside ownership-transfer patterns already in use. +- Watch for missing `override`, unnecessary copies in range-for, signed/unsigned comparisons, and narrowing conversions (especially `size_t` ↔ `int`). +- We use clang-format with Mozilla-style (see .clang-format). Code should already be autoformatted. +- We use clang-tidy with a custom config (see .clang-tidy). Flag any warnings that are not explicitly disabled there. + +### Python Style + +- PEP 8 / `snake_case`. +- Should pass `ruff check`, `ruff format`. + +### Correctness & Safety + +- Qt signal/slot connections: confirm sender/receiver lifetimes and that lambdas capturing `this` are safe. +- OpenGL / GPU code in `renderlib/graphics/`: check resource cleanup, context currency, and that GL calls aren't made from non-GL threads. +- File I/O in `renderlib/io/`: validate bounds, handle malformed input, and avoid blocking the UI thread. +- Command protocol: `parse()` / `write()` field order must match the `CMD_ARGS` declaration exactly. +- OWASP-relevant issues: unchecked input sizes, path traversal in file loaders, integer overflow in image dimension math. + +### Tests + +- New commands require a round-trip test in `test/test_commands.cpp` (see `AGENTS.md` for the pattern). +- Non-trivial logic in `renderlib/` should have a Catch2 test. +- Python client changes should have a corresponding test in `agave_pyclient/tests/`. + +### Build & Versioning + +- New source files must be added to the relevant `CMakeLists.txt`. +- Version bumps go through `tbump` — flag manual edits to version strings. + +## Output Format + +Group findings by severity: + +- **Blocking** — bugs, protocol mismatches, broken builds, security issues. +- **Should fix** — style violations, missing tests, architectural drift. +- **Nits** — minor suggestions, naming, comments. + +For each finding, cite the file and line(s) and give a short rationale plus a concrete suggested change. If a finding is speculative, say so. + +End with a one-line overall recommendation: _approve_, _approve with comments_, or _request changes_. + +## Constraints + +- Do not rewrite the whole change. Suggest targeted edits. +- Do not run the build or tests unless asked. +- If you edit files, limit edits to the specific fixes you flagged; do not opportunistically refactor unrelated code. diff --git a/AGENTS.md b/AGENTS.md index 8a3459e97..dcc3d4bbb 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -90,7 +90,7 @@ clang-tidy -p build --fix renderlib/RenderSettings.cpp ### Python - PEP 8 / snake_case -- Tooling: black, flake8, pyright (see `pyrightconfig.json`) +- Tooling: - `ruff check`, `ruff format`. ## Conventions diff --git a/agave_app/CacheSettings.cpp b/agave_app/CacheSettings.cpp index 55715c193..e495d6cd0 100644 --- a/agave_app/CacheSettings.cpp +++ b/agave_app/CacheSettings.cpp @@ -17,6 +17,9 @@ #include #elif defined(__linux__) #include +#else // macOS +#include +#include #endif namespace { @@ -183,9 +186,24 @@ CacheSettings::availableMemoryBytes() const return static_cast(info.freeram) * static_cast(info.mem_unit); } return 0; -#else - return 0; +#else // macos + mach_msg_type_number_t count = HOST_VM_INFO64_COUNT; + vm_statistics64_data_t vm_stats; + mach_port_t host_port = mach_host_self(); + + if (host_statistics64(host_port, HOST_VM_INFO64, (host_info64_t)&vm_stats, &count) == KERN_SUCCESS) { + // Get page size from the system + vm_size_t page_size; + host_page_size(host_port, &page_size); + + // Calculate free memory + long long free_memory = (long long)vm_stats.free_count * page_size; + return static_cast(free_memory); + } else { + return 0; + } #endif + return 0; } std::uint64_t From 0e583cd788c93fbbc5ae8b9e0f2337c1378b2b02 Mon Sep 17 00:00:00 2001 From: dmt Date: Sat, 23 May 2026 16:07:42 -0700 Subject: [PATCH 07/31] Fix data race: lock m_mutex around evictIfNeeded in setConfig evictIfNeeded mutates m_lruKeys, m_entries, and m_currentRamBytes, but setConfig was calling it after releasing the lock. Rename to evictIfNeededLocked to document the precondition and reacquire the lock before the call in setConfig. Co-Authored-By: Claude Opus 4.7 --- renderlib/CacheManager.cpp | 7 ++++--- renderlib/CacheManager.h | 3 ++- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/renderlib/CacheManager.cpp b/renderlib/CacheManager.cpp index ece17fc2d..7ba6f1ffc 100644 --- a/renderlib/CacheManager.cpp +++ b/renderlib/CacheManager.cpp @@ -127,7 +127,8 @@ CacheManager::setConfig(const CacheConfig& config) evictDiskIfNeeded(configCopy, 0); } - evictIfNeeded(0); + std::lock_guard lock(m_mutex); + evictIfNeededLocked(0); } CacheConfig @@ -304,7 +305,7 @@ CacheManager::touchEntry(std::list::iterator it) } void -CacheManager::evictIfNeeded(std::uint64_t incomingBytes) +CacheManager::evictIfNeededLocked(std::uint64_t incomingBytes) { if (m_config.maxRamBytes == 0) { return; @@ -341,7 +342,7 @@ CacheManager::storeImageInMemory(const CacheKey& key, const std::shared_ptr::iterator it); - void evictIfNeeded(std::uint64_t incomingBytes); + // Precondition: caller must hold m_mutex. + void evictIfNeededLocked(std::uint64_t incomingBytes); void storeImageInMemory(const CacheKey& key, const std::shared_ptr& image); std::shared_ptr loadFromDisk(const CacheKey& key, const CacheConfig& config); From 9604a43ee474cd341aeff96dd1b0a3bc3db9cda3 Mon Sep 17 00:00:00 2001 From: dmt Date: Sat, 23 May 2026 16:35:50 -0700 Subject: [PATCH 08/31] Guard clearDiskCache against deleting non-cache directories Two layers of protection: 1. CacheManager writes a marker file (.agave-cache-dir) into any directory it manages as a disk cache root, and clearDiskCache refuses to delete anything unless the marker is present. This stops a user-typed path like "C:\" or "/home/me" from being wiped if it ends up in the cache_dir setting. 2. Instead of remove_all on the whole cache dir, clearDiskCache now only removes per-entry subdirectories (those containing a meta.json). Files the user happens to keep alongside the cache are preserved, and the marker file itself survives. Also adds a QMessageBox confirmation in agaveGui showing the actual applied cache path before deletion. Co-Authored-By: Claude Opus 4.7 --- agave_app/agaveGui.cpp | 14 ++++++- renderlib/CacheManager.cpp | 80 ++++++++++++++++++++++++++++++++++++-- renderlib/CacheManager.h | 5 +++ 3 files changed, 94 insertions(+), 5 deletions(-) diff --git a/agave_app/agaveGui.cpp b/agave_app/agaveGui.cpp index bba47cd94..eb7489c42 100644 --- a/agave_app/agaveGui.cpp +++ b/agave_app/agaveGui.cpp @@ -109,7 +109,19 @@ agaveGui::agaveGui(QWidget* parent) m_cacheSettings.applyToRenderlib(data); }); connect(m_cacheSettingsDockWidget->widget()->clearDiskButton(), &QPushButton::clicked, this, [this]() { - CacheManager::instance().clearDiskCache(); + // Show the path that will actually be cleared (the applied config), which + // may differ from the widget's current text if the user edited the path + // without clicking Apply. + QString cacheDir = QString::fromStdString(CacheManager::instance().getConfig().cacheDir); + QMessageBox::StandardButton reply = QMessageBox::question( + this, + tr("Clear disk cache"), + tr("Delete all cached volume data in:\n%1\n\nThis cannot be undone.").arg(cacheDir), + QMessageBox::Yes | QMessageBox::No, + QMessageBox::No); + if (reply == QMessageBox::Yes) { + CacheManager::instance().clearDiskCache(); + } }); m_tabs = new QTabWidget(this); diff --git a/renderlib/CacheManager.cpp b/renderlib/CacheManager.cpp index 7ba6f1ffc..1976ad038 100644 --- a/renderlib/CacheManager.cpp +++ b/renderlib/CacheManager.cpp @@ -21,6 +21,12 @@ namespace { +// Marker file written into any directory we manage as our own disk cache root. +// clearDiskCache requires this file to be present before it will delete +// anything, which protects against the user pointing the cache dir at a path +// like "C:\" or "/home/me" and then clicking "Clear disk cache". +constexpr const char* kCacheMarkerFilename = ".agave-cache-dir"; + inline void hashCombine(std::size_t& seed, std::size_t value) { @@ -214,20 +220,83 @@ CacheManager::clear() void CacheManager::clearDiskCache() { - CacheConfig configCopy; + std::string cacheDir; + std::vector knownEntryPaths; { std::lock_guard lock(m_mutex); - configCopy = m_config; + cacheDir = m_config.cacheDir; + knownEntryPaths.reserve(m_diskEntries.size()); + for (const auto& kv : m_diskEntries) { + knownEntryPaths.push_back(kv.second.path); + } m_diskEntries.clear(); m_currentDiskBytes = 0; } - if (configCopy.cacheDir.empty()) { + if (cacheDir.empty()) { return; } + if (!isAgaveCacheDir(cacheDir)) { + LOG_WARNING << "Refusing to clear disk cache: directory missing AGAVE cache marker file (" << kCacheMarkerFilename + << "): " << cacheDir; + return; + } + + // Remove the per-entry subdirectories we know about. + for (const auto& path : knownEntryPaths) { + std::error_code ec; + std::filesystem::remove_all(path, ec); + } + + // Also remove any orphan per-entry subdirectories left behind by prior + // sessions or partial writes. We only touch subdirectories that contain a + // meta.json (i.e. look like cache entries we wrote) — anything else the user + // may have placed in the cache dir is preserved. + std::error_code dirEc; + for (auto it = std::filesystem::directory_iterator(cacheDir, dirEc); + it != std::filesystem::directory_iterator() && !dirEc; + it.increment(dirEc)) { + if (!it->is_directory()) { + continue; + } + std::filesystem::path metaPath = it->path() / "meta.json"; + std::error_code existEc; + if (std::filesystem::exists(metaPath, existEc)) { + std::error_code rmEc; + std::filesystem::remove_all(it->path(), rmEc); + } + } +} + +void +CacheManager::writeCacheMarker(const std::string& path) const +{ + if (path.empty()) { + return; + } + std::error_code ec; + std::filesystem::create_directories(path, ec); + std::filesystem::path marker = std::filesystem::path(path) / kCacheMarkerFilename; + std::error_code existEc; + if (std::filesystem::exists(marker, existEc)) { + return; + } + std::ofstream out(marker.string(), std::ios::trunc); + if (out) { + out << "AGAVE disk cache root. Safe to delete this directory and its contents.\n"; + } +} + +bool +CacheManager::isAgaveCacheDir(const std::string& path) const +{ + if (path.empty()) { + return false; + } std::error_code ec; - std::filesystem::remove_all(configCopy.cacheDir, ec); + std::filesystem::path marker = std::filesystem::path(path) / kCacheMarkerFilename; + return std::filesystem::exists(marker, ec); } CacheManager::CacheStats @@ -456,6 +525,8 @@ CacheManager::storeToDisk(const CacheKey& key, const std::shared_ptr& return; } + writeCacheMarker(config.cacheDir); + std::filesystem::path entryPath = std::filesystem::path(config.cacheDir) / diskCacheId(key); std::filesystem::path dataPath = entryPath / "data.zarr"; std::filesystem::path metaPath = entryPath / "meta.json"; @@ -551,6 +622,7 @@ CacheManager::loadDiskIndex(const CacheConfig& config) if (!std::filesystem::exists(root)) { return; } + writeCacheMarker(config.cacheDir); std::unordered_map entries; std::uint64_t totalBytes = 0; diff --git a/renderlib/CacheManager.h b/renderlib/CacheManager.h index 9d8bfd038..3e1f028ad 100644 --- a/renderlib/CacheManager.h +++ b/renderlib/CacheManager.h @@ -77,6 +77,11 @@ class CacheManager void loadDiskIndex(const CacheConfig& config); void evictDiskIfNeeded(const CacheConfig& config, std::uint64_t incomingBytes); std::uint64_t directorySizeBytes(const std::string& path) const; + // Writes a marker file to a directory we manage as our own disk cache root. + // clearDiskCache refuses to delete anything unless this marker is present, + // protecting against accidental wipes of user-typed paths (e.g. "C:\"). + void writeCacheMarker(const std::string& path) const; + bool isAgaveCacheDir(const std::string& path) const; mutable std::mutex m_mutex; CacheConfig m_config; From 443e797eabff3138ff906b3d7f66e7de502d3d59 Mon Sep 17 00:00:00 2001 From: dmt Date: Sat, 23 May 2026 16:59:48 -0700 Subject: [PATCH 09/31] Silence C4267 narrowing warning in cache manager test ImageXYZC::IN_MEMORY_BPP is size_t but the ImageXYZC constructor takes uint32_t bpp. Cast explicitly at the call site. Co-Authored-By: Claude Opus 4.7 --- test/test_cacheManager.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/test_cacheManager.cpp b/test/test_cacheManager.cpp index 1187e96e8..cb01341ef 100644 --- a/test/test_cacheManager.cpp +++ b/test/test_cacheManager.cpp @@ -23,7 +23,8 @@ makeImage(uint32_t x, uint32_t y, uint32_t z, uint32_t c) const std::uint64_t bytes = static_cast(x) * y * z * c * kBytesPerPixel; auto* data = new uint8_t[bytes]; std::memset(data, 0, bytes); - return std::make_shared(x, y, z, c, ImageXYZC::IN_MEMORY_BPP, data, 1.0f, 1.0f, 1.0f, "units"); + return std::make_shared( + x, y, z, c, static_cast(ImageXYZC::IN_MEMORY_BPP), data, 1.0f, 1.0f, 1.0f, "units"); } LoadSpec From 39bc99765e567248bd46dd3018225a000ac26fa4 Mon Sep 17 00:00:00 2001 From: dmt Date: Sat, 23 May 2026 17:27:09 -0700 Subject: [PATCH 10/31] Invalidate cache entries when the source file changes CacheKey now includes the filepath's last_write_time and file_size, so overwriting a file produces a different key and a cache miss instead of silently returning the old image. Both fields are folded into the hash, the equality check, and keyToString (which is stored as meta["key"] in disk entries). Caveats: - Remote URLs (http://, s3:, gs:) are not stat'd; their mtime/size stay zero. These typically point to immutable bucket objects. - For zarr directories the directory's last_write_time is used, which most filesystems update on entry add/remove at the top level but not necessarily on chunk rewrites in subdirs. Best-effort, not airtight. - Image sequences key on the representative filepath only; regenerating a non-representative frame won't invalidate. - Existing on-disk entries from prior versions of this branch will no longer match (their meta["key"] format changed) and will accumulate as orphans until normal eviction or a Clear Disk Cache. Co-Authored-By: Claude Opus 4.7 --- renderlib/CacheManager.cpp | 50 +++++++++++++++++++++++++++++++++++++- renderlib/CacheManager.h | 8 ++++++ 2 files changed, 57 insertions(+), 1 deletion(-) diff --git a/renderlib/CacheManager.cpp b/renderlib/CacheManager.cpp index 1976ad038..a93d4ca1d 100644 --- a/renderlib/CacheManager.cpp +++ b/renderlib/CacheManager.cpp @@ -48,6 +48,47 @@ toHex(std::size_t value) return stream.str(); } +// Paths beginning with these schemes are treated as remote; we don't try to +// stat them and the cache key omits mtime/size for them. +bool +isRemotePath(const std::string& path) +{ + return path.rfind("http", 0) == 0 || path.rfind("s3:", 0) == 0 || path.rfind("gs:", 0) == 0; +} + +// Returns (mtime_ns, file_size). Either or both may be 0 if the path is +// remote, missing, or otherwise unreadable. For directories (zarr) file_size +// is 0; mtime is the directory's last_write_time, which most filesystems +// update when entries are added/removed at the top level. This is best-effort +// invalidation — a zarr whose chunks were rewritten without touching the +// root directory will not be invalidated by this check. +std::pair +statForKey(const std::string& path) +{ + if (path.empty() || isRemotePath(path)) { + return { 0, 0 }; + } + std::error_code ec; + std::filesystem::file_status status = std::filesystem::status(path, ec); + if (ec || !std::filesystem::exists(status)) { + return { 0, 0 }; + } + std::uint64_t mtimeNs = 0; + auto writeTime = std::filesystem::last_write_time(path, ec); + if (!ec) { + mtimeNs = static_cast( + std::chrono::duration_cast(writeTime.time_since_epoch()).count()); + } + std::uint64_t size = 0; + if (std::filesystem::is_regular_file(status)) { + auto s = std::filesystem::file_size(path, ec); + if (!ec) { + size = static_cast(s); + } + } + return { mtimeNs, size }; +} + std::vector channelNamesFromImage(const ImageXYZC& image) { @@ -66,7 +107,8 @@ CacheKey::operator==(const CacheKey& other) const { return filepath == other.filepath && subpath == other.subpath && scene == other.scene && time == other.time && channels == other.channels && minx == other.minx && maxx == other.maxx && miny == other.miny && - maxy == other.maxy && minz == other.minz && maxz == other.maxz && isImageSequence == other.isImageSequence; + maxy == other.maxy && minz == other.minz && maxz == other.maxz && isImageSequence == other.isImageSequence && + fileMtimeNs == other.fileMtimeNs && fileSize == other.fileSize; } std::size_t @@ -84,6 +126,8 @@ CacheKeyHash::operator()(const CacheKey& key) const hashCombine(seed, std::hash{}(key.minz)); hashCombine(seed, std::hash{}(key.maxz)); hashCombine(seed, std::hash{}(key.isImageSequence)); + hashCombine(seed, std::hash{}(key.fileMtimeNs)); + hashCombine(seed, std::hash{}(key.fileSize)); for (auto ch : key.channels) { hashCombine(seed, std::hash{}(ch)); } @@ -329,6 +373,9 @@ CacheManager::makeKey(const LoadSpec& loadSpec) const key.minz = loadSpec.minz; key.maxz = loadSpec.maxz; key.isImageSequence = loadSpec.isImageSequence; + auto stat = statForKey(loadSpec.filepath); + key.fileMtimeNs = stat.first; + key.fileSize = stat.second; return key; } @@ -339,6 +386,7 @@ CacheManager::keyToString(const CacheKey& key) const stream << key.filepath << "|" << key.subpath << "|" << key.scene << "|" << key.time << "|"; stream << key.minx << "," << key.maxx << "," << key.miny << "," << key.maxy << "," << key.minz << "," << key.maxz << "|" << (key.isImageSequence ? 1 : 0) << "|"; + stream << "m=" << key.fileMtimeNs << ",s=" << key.fileSize << "|"; for (size_t i = 0; i < key.channels.size(); ++i) { if (i > 0) { stream << ","; diff --git a/renderlib/CacheManager.h b/renderlib/CacheManager.h index 3e1f028ad..cf5aad239 100644 --- a/renderlib/CacheManager.h +++ b/renderlib/CacheManager.h @@ -27,6 +27,14 @@ struct CacheKey std::uint32_t minz = 0; std::uint32_t maxz = 0; bool isImageSequence = false; + // last_write_time of the filepath (or directory) at the time the key was + // built, expressed as nanoseconds since epoch. Zero for remote URLs and for + // paths we couldn't stat. Folding this into the key invalidates cache + // entries when the source file is overwritten. + std::uint64_t fileMtimeNs = 0; + // file_size of filepath at the time the key was built. Zero for + // directories (zarr) and remote URLs. + std::uint64_t fileSize = 0; bool operator==(const CacheKey& other) const; }; From 832b1a9bbe8cfa9f2506ee38a0c30f842c4613ce Mon Sep 17 00:00:00 2001 From: dmt Date: Sat, 23 May 2026 17:47:08 -0700 Subject: [PATCH 11/31] Move available memory/disk queries into renderlib (no Qt deps) Adds renderlib/SystemInfo.{h,cpp} with two free functions in a SystemInfo namespace, using only std::filesystem and std::ifstream instead of QStorageInfo/QFile so the helpers can be reused from non-Qt code paths. availableMemoryBytes() and availableDiskBytes() now also report realistic numbers on Linux and macOS instead of just truly-free pages: - Linux: parses MemAvailable from /proc/meminfo (kernel's own estimate of allocatable memory including reclaimable cache); falls back to sysinfo (freeram + bufferram) on older kernels. - macOS: includes inactive + purgeable + speculative pages alongside free pages, matching Activity Monitor's "Available". - Windows: unchanged (MEMORYSTATUSEX::ullAvailPhys). - Disk: std::filesystem::space().available, with a parent_path fallback if the path doesn't exist yet. CacheSettings::availableMemoryBytes / availableDiskBytes deleted in favor of the new helpers. toRenderlibConfig now also LOG_WARNINGs when its silent clamp reduces the user's requested RAM or disk limit, so the reduction is discoverable. Co-Authored-By: Claude Opus 4.7 --- agave_app/CacheSettings.cpp | 75 ++++--------------------- agave_app/CacheSettings.h | 3 - renderlib/CMakeLists.txt | 2 + renderlib/SystemInfo.cpp | 108 ++++++++++++++++++++++++++++++++++++ renderlib/SystemInfo.h | 25 +++++++++ 5 files changed, 146 insertions(+), 67 deletions(-) create mode 100644 renderlib/SystemInfo.cpp create mode 100644 renderlib/SystemInfo.h diff --git a/agave_app/CacheSettings.cpp b/agave_app/CacheSettings.cpp index e495d6cd0..0522b6f5c 100644 --- a/agave_app/CacheSettings.cpp +++ b/agave_app/CacheSettings.cpp @@ -2,26 +2,14 @@ #include "renderlib/CacheManager.h" #include "renderlib/Logging.h" +#include "renderlib/SystemInfo.h" #include #include #include -#include #include -#ifdef _WIN32 -#ifndef NOMINMAX -#define NOMINMAX -#endif -#include -#elif defined(__linux__) -#include -#else // macOS -#include -#include -#endif - namespace { ::CacheConfig @@ -37,16 +25,24 @@ toRenderlibConfig(const CacheSettings& settings, const CacheSettingsData& data) config.enableDisk = normalized.enableDisk; config.cacheDir = normalized.cacheDir; - std::uint64_t availableMem = settings.availableMemoryBytes(); + std::uint64_t availableMem = SystemInfo::availableMemoryBytes(); if (availableMem > 0) { config.maxRamBytes = std::min(normalized.maxRamBytes, availableMem); + if (config.maxRamBytes < normalized.maxRamBytes) { + LOG_WARNING << "Cache RAM limit reduced from requested " << normalized.maxRamBytes << " to " << config.maxRamBytes + << " bytes to fit available memory."; + } } else { config.maxRamBytes = normalized.maxRamBytes; } - std::uint64_t availableDisk = settings.availableDiskBytes(normalized.cacheDir); + std::uint64_t availableDisk = SystemInfo::availableDiskBytes(normalized.cacheDir); if (availableDisk > 0) { config.maxDiskBytes = std::min(normalized.maxDiskBytes, availableDisk); + if (config.maxDiskBytes < normalized.maxDiskBytes && normalized.enableDisk) { + LOG_WARNING << "Cache disk limit reduced from requested " << normalized.maxDiskBytes << " to " + << config.maxDiskBytes << " bytes to fit available disk space."; + } } else { config.maxDiskBytes = normalized.maxDiskBytes; } @@ -170,55 +166,6 @@ CacheSettings::applyToRenderlib(const CacheSettingsData& data) const << " cache_dir=" << applied.cacheDir; } -std::uint64_t -CacheSettings::availableMemoryBytes() const -{ -#ifdef _WIN32 - MEMORYSTATUSEX statex; - statex.dwLength = sizeof(statex); - if (GlobalMemoryStatusEx(&statex)) { - return static_cast(statex.ullAvailPhys); - } - return 0; -#elif defined(__linux__) - struct sysinfo info; - if (sysinfo(&info) == 0) { - return static_cast(info.freeram) * static_cast(info.mem_unit); - } - return 0; -#else // macos - mach_msg_type_number_t count = HOST_VM_INFO64_COUNT; - vm_statistics64_data_t vm_stats; - mach_port_t host_port = mach_host_self(); - - if (host_statistics64(host_port, HOST_VM_INFO64, (host_info64_t)&vm_stats, &count) == KERN_SUCCESS) { - // Get page size from the system - vm_size_t page_size; - host_page_size(host_port, &page_size); - - // Calculate free memory - long long free_memory = (long long)vm_stats.free_count * page_size; - return static_cast(free_memory); - } else { - return 0; - } -#endif - return 0; -} - -std::uint64_t -CacheSettings::availableDiskBytes(const std::string& path) const -{ - if (path.empty()) { - return 0; - } - QStorageInfo storage(QString::fromStdString(path)); - if (!storage.isValid() || !storage.isReady()) { - return 0; - } - return static_cast(storage.bytesAvailable()); -} - bool CacheSettings::canWriteCacheDir(const std::string& path) const { diff --git a/agave_app/CacheSettings.h b/agave_app/CacheSettings.h index adb4b0325..bfac1329d 100644 --- a/agave_app/CacheSettings.h +++ b/agave_app/CacheSettings.h @@ -25,9 +25,6 @@ class CacheSettings CacheSettingsData defaultSettings() const; std::string configPath() const; - std::uint64_t availableMemoryBytes() const; - std::uint64_t availableDiskBytes(const std::string& path) const; - private: bool canWriteCacheDir(const std::string& path) const; }; diff --git a/renderlib/CMakeLists.txt b/renderlib/CMakeLists.txt index 11f91b380..eef768bed 100644 --- a/renderlib/CMakeLists.txt +++ b/renderlib/CMakeLists.txt @@ -82,6 +82,8 @@ target_sources(renderlib PRIVATE "${CMAKE_CURRENT_SOURCE_DIR}/Status.h" "${CMAKE_CURRENT_SOURCE_DIR}/StringUtil.h" "${CMAKE_CURRENT_SOURCE_DIR}/StringUtil.cpp" + "${CMAKE_CURRENT_SOURCE_DIR}/SystemInfo.h" + "${CMAKE_CURRENT_SOURCE_DIR}/SystemInfo.cpp" "${CMAKE_CURRENT_SOURCE_DIR}/threading.cpp" "${CMAKE_CURRENT_SOURCE_DIR}/threading.h" "${CMAKE_CURRENT_SOURCE_DIR}/Timeline.cpp" diff --git a/renderlib/SystemInfo.cpp b/renderlib/SystemInfo.cpp new file mode 100644 index 000000000..d5a725a03 --- /dev/null +++ b/renderlib/SystemInfo.cpp @@ -0,0 +1,108 @@ +#include "SystemInfo.h" + +#include +#include +#include +#include + +#ifdef _WIN32 +#ifndef NOMINMAX +#define NOMINMAX +#endif +#include +#elif defined(__linux__) +#include +#else // macOS +#include +#include +#endif + +namespace SystemInfo { + +std::uint64_t +availableMemoryBytes() +{ +#ifdef _WIN32 + MEMORYSTATUSEX statex; + statex.dwLength = sizeof(statex); + if (GlobalMemoryStatusEx(&statex)) { + return static_cast(statex.ullAvailPhys); + } + return 0; +#elif defined(__linux__) + // Prefer MemAvailable from /proc/meminfo: this is the kernel's own + // estimate of how much memory can be allocated without swapping, and + // unlike sysinfo.freeram it includes reclaimable page cache and slab. + // On a typical box freeram alone is under 5% of total even when most + // memory is reclaimable. + std::ifstream meminfo("/proc/meminfo"); + if (meminfo.is_open()) { + std::string line; + while (std::getline(meminfo, line)) { + // Format: "MemAvailable: 12345678 kB" + if (line.rfind("MemAvailable:", 0) == 0) { + std::istringstream iss(line.substr(13)); + std::uint64_t kb = 0; + iss >> kb; + if (kb > 0) { + return kb * 1024ULL; + } + break; + } + } + } + // Older kernels (<3.14) don't expose MemAvailable. Fall back to + // sysinfo, and at least add bufferram to freeram so we don't grossly + // under-report. + struct sysinfo info; + if (sysinfo(&info) == 0) { + std::uint64_t bytes = static_cast(info.freeram) + static_cast(info.bufferram); + return bytes * static_cast(info.mem_unit); + } + return 0; +#else // macOS + mach_msg_type_number_t count = HOST_VM_INFO64_COUNT; + vm_statistics64_data_t vm_stats; + mach_port_t host_port = mach_host_self(); + + if (host_statistics64(host_port, HOST_VM_INFO64, (host_info64_t)&vm_stats, &count) == KERN_SUCCESS) { + vm_size_t page_size = 0; + host_page_size(host_port, &page_size); + + // free_count alone is what's truly free right now; macOS keeps that + // small on purpose and uses inactive/purgeable/speculative pages as + // reclaimable buffer (this is what Activity Monitor reports as + // "Available"). Including them avoids massive under-reporting. + std::uint64_t reclaimablePages = static_cast(vm_stats.free_count) + + static_cast(vm_stats.inactive_count) + + static_cast(vm_stats.purgeable_count) + + static_cast(vm_stats.speculative_count); + return reclaimablePages * static_cast(page_size); + } + return 0; +#endif +} + +std::uint64_t +availableDiskBytes(const std::string& path) +{ + if (path.empty()) { + return 0; + } + std::error_code ec; + std::filesystem::space_info info = std::filesystem::space(path, ec); + if (ec) { + // path may not exist yet; try its parent. + std::filesystem::path parent = std::filesystem::path(path).parent_path(); + if (parent.empty()) { + return 0; + } + info = std::filesystem::space(parent, ec); + if (ec) { + return 0; + } + } + return static_cast(info.available); +} + +} // namespace SystemInfo diff --git a/renderlib/SystemInfo.h b/renderlib/SystemInfo.h new file mode 100644 index 000000000..c4c7ef02c --- /dev/null +++ b/renderlib/SystemInfo.h @@ -0,0 +1,25 @@ +#pragma once + +#include +#include + +namespace SystemInfo { + +// Returns an estimate of physical memory that can be allocated without +// swapping, in bytes. Returns 0 if the value cannot be determined. +// +// Per-platform definition: +// Linux: MemAvailable from /proc/meminfo (kernel's own estimate that +// includes reclaimable page cache and slab). Falls back to +// sysinfo (freeram + bufferram) on older kernels. +// macOS: free + inactive + purgeable + speculative pages, matching +// what Activity Monitor reports as "Available". +// Windows: MEMORYSTATUSEX::ullAvailPhys. +std::uint64_t availableMemoryBytes(); + +// Returns bytes available to a non-privileged user on the filesystem +// containing `path`. Returns 0 if `path` is empty or the value cannot +// be determined. +std::uint64_t availableDiskBytes(const std::string& path); + +} // namespace SystemInfo From f4fea24fde1cc51ef1e0ece80a238a1940fa1d49 Mon Sep 17 00:00:00 2001 From: dmt Date: Sat, 23 May 2026 18:06:25 -0700 Subject: [PATCH 12/31] Disk cache: evict before write, and close eviction race MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit storeToDisk now: - Estimates bytes up front and refuses to write images larger than the entire disk cap instead of evicting everything and overshooting. - Calls evictDiskIfNeeded(bytes) before the tensorstore write, so the cap is never temporarily exceeded and we don't waste a large write that would immediately have to be undone. - Cleans up the per-entry directory (remove_all) on every failure path — tensorstore open, tensorstore write, or meta.json write — so half-written entries don't leak into the cache dir. - Saturates instead of underflowing if the existing entry's bookkeeping bytes exceed m_currentDiskBytes. evictDiskIfNeeded now: - Runs under a single lock_guard for the entire operation (sort, select, bookkeeping update, filesystem remove_all). The previous code released and re-acquired the lock between victim selection and bookkeeping mutation, leaving a window in which a concurrent storeToDisk could refresh the entry we were about to delete on disk — wiping the fresh file. Holding the lock for the whole operation closes that race. - Sorts entries by lastAccess once instead of doing an O(N) scan per victim, so eviction is O(N log N) total. - Logs a warning if remove_all fails (e.g. Windows file-handle contention from a concurrent loadFromDisk) — the bookkeeping is still updated to reflect eviction; the stale files get cleaned by the next clearDiskCache. The redundant post-write evictDiskIfNeeded(config, 0) in storeToDisk is now dropped. Co-Authored-By: Claude Opus 4.7 --- renderlib/CacheManager.cpp | 110 +++++++++++++++++++++++-------------- 1 file changed, 68 insertions(+), 42 deletions(-) diff --git a/renderlib/CacheManager.cpp b/renderlib/CacheManager.cpp index a93d4ca1d..bb1eec5f3 100644 --- a/renderlib/CacheManager.cpp +++ b/renderlib/CacheManager.cpp @@ -573,6 +573,22 @@ CacheManager::storeToDisk(const CacheKey& key, const std::shared_ptr& return; } + std::uint64_t bytes = estimateImageBytes(*image); + if (bytes == 0) { + return; + } + if (bytes > config.maxDiskBytes) { + // A single image larger than the entire disk cap; refuse rather than + // evict everything and overshoot. + LOG_WARNING << "Disk cache: skipping store of " << bytes + << " byte image — larger than disk cap " << config.maxDiskBytes; + return; + } + + // Make room before writing, so we never temporarily exceed the cap on + // disk and don't waste a large write that would have to be undone. + evictDiskIfNeeded(config, bytes); + writeCacheMarker(config.cacheDir); std::filesystem::path entryPath = std::filesystem::path(config.cacheDir) / diskCacheId(key); @@ -606,6 +622,9 @@ CacheManager::storeToDisk(const CacheKey& key, const std::shared_ptr& tensorstore::OpenMode::create | tensorstore::OpenMode::open); auto result = openFuture.result(); if (!result.ok()) { + std::error_code rmEc; + std::filesystem::remove_all(entryPath, rmEc); + LOG_WARNING << "Disk cache store: tensorstore open failed for " << dataPath.string(); return; } @@ -613,10 +632,12 @@ CacheManager::storeToDisk(const CacheKey& key, const std::shared_ptr& auto arr = tensorstore::Array(reinterpret_cast(image->ptr()), shape); auto writeResult = tensorstore::Write(tensorstore::UnownedToShared(arr), store).result(); if (!writeResult.ok()) { + std::error_code rmEc; + std::filesystem::remove_all(entryPath, rmEc); + LOG_WARNING << "Disk cache store: tensorstore write failed for " << dataPath.string(); return; } - std::uint64_t bytes = estimateImageBytes(*image); std::uint64_t accessTime = nowMillis(); nlohmann::json meta = { { "key", keyToString(key) }, { "sizeX", sizeX }, @@ -636,6 +657,9 @@ CacheManager::storeToDisk(const CacheKey& key, const std::shared_ptr& std::ofstream metaOut(metaPath.string(), std::ios::trunc); metaOut << meta.dump(2); } catch (...) { + std::error_code rmEc; + std::filesystem::remove_all(entryPath, rmEc); + LOG_WARNING << "Disk cache store: meta.json write failed for " << metaPath.string(); return; } @@ -648,13 +672,15 @@ CacheManager::storeToDisk(const CacheKey& key, const std::shared_ptr& entry.lastAccess = accessTime; auto it = m_diskEntries.find(diskCacheId(key)); if (it != m_diskEntries.end()) { - m_currentDiskBytes -= it->second.bytes; + if (m_currentDiskBytes >= it->second.bytes) { + m_currentDiskBytes -= it->second.bytes; + } else { + m_currentDiskBytes = 0; + } } m_diskEntries[diskCacheId(key)] = entry; m_currentDiskBytes += bytes; } - - evictDiskIfNeeded(config, 0); } void @@ -722,49 +748,49 @@ CacheManager::evictDiskIfNeeded(const CacheConfig& config, std::uint64_t incomin return; } - while (true) { - std::string oldestKey; - std::uint64_t oldestAccess = std::numeric_limits::max(); - std::uint64_t currentBytes = 0; - { - std::lock_guard lock(m_mutex); - currentBytes = m_currentDiskBytes; - if ((currentBytes + incomingBytes) <= config.maxDiskBytes || m_diskEntries.empty()) { - break; - } - for (const auto& item : m_diskEntries) { - if (item.second.lastAccess < oldestAccess) { - oldestAccess = item.second.lastAccess; - oldestKey = item.first; - } - } - } + // Hold the lock for the entire eviction so a concurrent storeToDisk can't + // re-populate an entry we're about to delete on disk (which would + // otherwise let us nuke the fresh file). Each per-entry remove_all is a + // single small zarr directory, so keeping the lock held is acceptable. + std::lock_guard lock(m_mutex); - if (oldestKey.empty()) { + if ((m_currentDiskBytes + incomingBytes) <= config.maxDiskBytes) { + return; + } + + // Sort entries by lastAccess ascending so we evict the oldest first. + std::vector> byAge; + byAge.reserve(m_diskEntries.size()); + for (const auto& kv : m_diskEntries) { + byAge.emplace_back(kv.second.lastAccess, kv.first); + } + std::sort(byAge.begin(), byAge.end()); + + for (const auto& aged : byAge) { + if ((m_currentDiskBytes + incomingBytes) <= config.maxDiskBytes) { break; } - - std::string removePath; - std::uint64_t removedBytes = 0; - { - std::lock_guard lock(m_mutex); - auto it = m_diskEntries.find(oldestKey); - if (it == m_diskEntries.end()) { - continue; - } - removePath = it->second.path; - removedBytes = it->second.bytes; - m_diskEntries.erase(it); - if (m_currentDiskBytes >= removedBytes) { - m_currentDiskBytes -= removedBytes; - } else { - m_currentDiskBytes = 0; - } + auto it = m_diskEntries.find(aged.second); + if (it == m_diskEntries.end()) { + continue; } + std::string path = it->second.path; + std::uint64_t bytes = it->second.bytes; + if (m_currentDiskBytes >= bytes) { + m_currentDiskBytes -= bytes; + } else { + m_currentDiskBytes = 0; + } + m_diskEntries.erase(it); - if (!removePath.empty()) { - std::error_code ec; - std::filesystem::remove_all(removePath, ec); + std::error_code ec; + std::filesystem::remove_all(path, ec); + if (ec) { + // 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. + LOG_WARNING << "Disk cache eviction: failed to remove " << path << ": " << ec.message(); } } } From f7630a8cec16f3d5e39bb3dce07ef5c2e5affa7c Mon Sep 17 00:00:00 2001 From: dmt Date: Sat, 23 May 2026 19:20:50 -0700 Subject: [PATCH 13/31] Add disk-tier and mtime-invalidation tests for CacheManager MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds a TempCacheDir RAII helper that gives each test a unique sub-directory under the system temp dir (auto-cleaned in the destructor), plus a diskConfig() builder and a countSubdirs() helper. New disk-tier test cases: - Initializing disk cache writes the .agave-cache-dir marker. - Round-trip: store with disk enabled, drop RAM, find reloads from disk; pixel data is bit-identical and stats.diskHits is 1. - Image larger than maxDiskBytes is not written (no entry subdir). - LRU disk eviction: store 3 entries at a 2-entry cap, oldest is evicted and subdir count stays at <= 2. - clearDiskCache wipes entry subdirs but keeps the marker file. - clearDiskCache refuses to touch a directory whose marker has been removed (file count unchanged). - Re-pointing setConfig away and back rebuilds the disk index from the on-disk meta files (simulates session restart). New mtime test case: a real file on disk is used as the cache key source; bumping its last_write_time forward via std::filesystem::last_write_time(path, future_time) makes a subsequent findImage miss instead of returning the stale image. Uses an explicit time set rather than real-sleep so the test is deterministic on coarse-mtime filesystems. Per-test disk usage stays in the low-KB range — sized for 4x4x4x1 uint16 images (128 raw bytes each) with caps of a few image-widths or 1 MB. Co-Authored-By: Claude Opus 4.7 --- test/test_cacheManager.cpp | 269 +++++++++++++++++++++++++++++++++++++ 1 file changed, 269 insertions(+) diff --git a/test/test_cacheManager.cpp b/test/test_cacheManager.cpp index cb01341ef..31f18d71b 100644 --- a/test/test_cacheManager.cpp +++ b/test/test_cacheManager.cpp @@ -5,10 +5,15 @@ #include "renderlib/IFileReader.h" #include "renderlib/ImageXYZC.h" +#include +#include #include #include +#include +#include #include #include +#include namespace { @@ -61,6 +66,78 @@ ramOnlyConfig(std::uint64_t maxRamBytes) return cfg; } +// An image whose pixel bytes match a known pattern (data[i] = i & 0xFF), used +// to verify the disk round-trip didn't corrupt the data. +std::shared_ptr +makeImageWithPattern(uint32_t x, uint32_t y, uint32_t z, uint32_t c) +{ + const std::uint64_t bytes = static_cast(x) * y * z * c * kBytesPerPixel; + auto* data = new uint8_t[bytes]; + for (std::uint64_t i = 0; i < bytes; ++i) { + data[i] = static_cast(i & 0xFF); + } + return std::make_shared( + x, y, z, c, static_cast(ImageXYZC::IN_MEMORY_BPP), data, 1.0f, 1.0f, 1.0f, "units"); +} + +// RAII temporary directory for disk-cache tests. Each instance gets a unique +// path under the system temp dir and is removed on destruction. The counter +// guarantees uniqueness even within a single test process. +class TempCacheDir +{ +public: + TempCacheDir() + { + static std::atomic sCounter{ 0 }; + int n = sCounter.fetch_add(1); + m_path = std::filesystem::temp_directory_path() / ("agave_cache_test_" + std::to_string(n)); + std::error_code ec; + std::filesystem::remove_all(m_path, ec); + std::filesystem::create_directories(m_path, ec); + } + ~TempCacheDir() + { + std::error_code ec; + std::filesystem::remove_all(m_path, ec); + } + TempCacheDir(const TempCacheDir&) = delete; + TempCacheDir& operator=(const TempCacheDir&) = delete; + + std::string str() const { return m_path.string(); } + std::filesystem::path path() const { return m_path; } + +private: + std::filesystem::path m_path; +}; + +CacheConfig +diskConfig(const std::string& cacheDir, std::uint64_t maxRamBytes, std::uint64_t maxDiskBytes) +{ + CacheConfig cfg; + cfg.enabled = true; + cfg.enableDisk = true; + cfg.maxRamBytes = maxRamBytes; + cfg.maxDiskBytes = maxDiskBytes; + cfg.cacheDir = cacheDir; + return cfg; +} + +// Count subdirectories under `dir` (used to verify entry-dir counts after +// store/evict/clear operations). +int +countSubdirs(const std::filesystem::path& dir) +{ + int n = 0; + std::error_code ec; + for (auto it = std::filesystem::directory_iterator(dir, ec); !ec && it != std::filesystem::directory_iterator(); + it.increment(ec)) { + if (it->is_directory()) { + ++n; + } + } + return n; +} + } // namespace TEST_CASE("CacheManager respects RAM limit and evicts LRU entries", "[cache]") @@ -281,3 +358,195 @@ TEST_CASE("CacheManager respects RAM limit and evicts LRU entries", "[cache]") REQUIRE(CacheManager::instance().findImage(makeSpec("b")) == nullptr); } } + +TEST_CASE("CacheManager disk tier round-trips images and respects the disk cap", "[cache][disk]") +{ + // 4x4x4x1 uint16 image -> 128 raw bytes. On-disk zarr representation is a + // few KB once chunk + metadata files are accounted for, so a 1 MB cap is + // comfortably oversized for the round-trip and clear-cache tests, and we + // size the cap explicitly down to a couple of images for the eviction + // test. Total disk usage per test is well under a megabyte. + const std::uint64_t oneImage = imageBytes(4, 4, 4, 1); + + SECTION("Initializing disk cache writes the AGAVE marker file") + { + resetCache(); + TempCacheDir tmp; + CacheManager::instance().setConfig(diskConfig(tmp.str(), oneImage * 4, 1ULL * 1024 * 1024)); + + REQUIRE(std::filesystem::exists(tmp.path() / ".agave-cache-dir")); + } + + SECTION("Round-trip: store, drop RAM, find reloads from disk with bit-identical data") + { + resetCache(); + TempCacheDir tmp; + CacheManager::instance().setConfig(diskConfig(tmp.str(), oneImage * 4, 1ULL * 1024 * 1024)); + + auto img = makeImageWithPattern(4, 4, 4, 1); + auto spec = makeSpec("disk_roundtrip"); + CacheManager::instance().storeImage(spec, img); + + // Drop RAM cache; disk cache survives. + CacheManager::instance().clear(); + CacheManager::instance().resetStats(); + + auto found = CacheManager::instance().findImage(spec); + REQUIRE(found != nullptr); + // The reloaded image is a fresh instance, not the same shared_ptr. + REQUIRE(found.get() != img.get()); + + auto stats = CacheManager::instance().getStats(); + REQUIRE(stats.diskHits == 1); + REQUIRE(stats.ramHits == 0); + + REQUIRE(found->sizeX() == 4); + REQUIRE(found->sizeY() == 4); + REQUIRE(found->sizeZ() == 4); + REQUIRE(found->sizeC() == 1); + + const std::uint64_t bytes = imageBytes(4, 4, 4, 1); + REQUIRE(std::memcmp(img->ptr(), found->ptr(), bytes) == 0); + } + + SECTION("An image larger than maxDiskBytes is not written to disk") + { + resetCache(); + TempCacheDir tmp; + // Cap is below a single image, so storeToDisk must refuse outright. + CacheManager::instance().setConfig(diskConfig(tmp.str(), oneImage * 4, oneImage / 2)); + + auto spec = makeSpec("too_big_for_disk"); + CacheManager::instance().storeImage(spec, makeImage(4, 4, 4, 1)); + + // No entry subdirectory should have been created. + REQUIRE(countSubdirs(tmp.path()) == 0); + + // Drop RAM to force a disk-or-miss lookup; with no entry on disk we + // should get a miss. + CacheManager::instance().clear(); + REQUIRE(CacheManager::instance().findImage(spec) == nullptr); + } + + SECTION("Disk eviction removes the oldest entry to stay under the cap") + { + resetCache(); + TempCacheDir tmp; + // Cap large enough for two entries' raw byte estimate but not three. + CacheManager::instance().setConfig(diskConfig(tmp.str(), oneImage * 4, oneImage * 2)); + + CacheManager::instance().storeImage(makeSpec("disk_a"), makeImage(4, 4, 4, 1)); + // Sleep briefly so lastAccess timestamps are distinct on fast disks. + std::this_thread::sleep_for(std::chrono::milliseconds(5)); + CacheManager::instance().storeImage(makeSpec("disk_b"), makeImage(4, 4, 4, 1)); + std::this_thread::sleep_for(std::chrono::milliseconds(5)); + CacheManager::instance().storeImage(makeSpec("disk_c"), makeImage(4, 4, 4, 1)); + + // Drop RAM so finds have to go through the disk tier. + CacheManager::instance().clear(); + + REQUIRE(CacheManager::instance().findImage(makeSpec("disk_a")) == nullptr); + REQUIRE(CacheManager::instance().findImage(makeSpec("disk_b")) != nullptr); + REQUIRE(CacheManager::instance().findImage(makeSpec("disk_c")) != nullptr); + + // After eviction there should be at most two entry subdirectories on + // disk (the marker file is not a directory). + REQUIRE(countSubdirs(tmp.path()) <= 2); + } + + SECTION("clearDiskCache removes entry subdirectories but keeps the marker") + { + resetCache(); + TempCacheDir tmp; + CacheManager::instance().setConfig(diskConfig(tmp.str(), oneImage * 4, 1ULL * 1024 * 1024)); + + CacheManager::instance().storeImage(makeSpec("clear_a"), makeImage(4, 4, 4, 1)); + CacheManager::instance().storeImage(makeSpec("clear_b"), makeImage(4, 4, 4, 1)); + REQUIRE(countSubdirs(tmp.path()) >= 2); + + CacheManager::instance().clearDiskCache(); + + REQUIRE(countSubdirs(tmp.path()) == 0); + REQUIRE(std::filesystem::exists(tmp.path() / ".agave-cache-dir")); + } + + SECTION("clearDiskCache refuses to touch a directory without the AGAVE marker") + { + resetCache(); + TempCacheDir tmp; + CacheManager::instance().setConfig(diskConfig(tmp.str(), oneImage * 4, 1ULL * 1024 * 1024)); + + CacheManager::instance().storeImage(makeSpec("guarded_a"), makeImage(4, 4, 4, 1)); + int subdirsBefore = countSubdirs(tmp.path()); + REQUIRE(subdirsBefore >= 1); + + // Simulate a directory that doesn't belong to AGAVE by removing the + // marker file. clearDiskCache must refuse. + std::error_code ec; + std::filesystem::remove(tmp.path() / ".agave-cache-dir", ec); + REQUIRE_FALSE(ec); + + CacheManager::instance().clearDiskCache(); + + REQUIRE(countSubdirs(tmp.path()) == subdirsBefore); + } + + SECTION("Re-pointing setConfig at a previously-used cache dir rebuilds the disk index") + { + resetCache(); + TempCacheDir tmp; + CacheManager::instance().setConfig(diskConfig(tmp.str(), oneImage * 4, 1ULL * 1024 * 1024)); + + CacheManager::instance().storeImage(makeSpec("persistent"), makeImage(4, 4, 4, 1)); + + // Simulate a session restart: switch the cache dir somewhere else + // (forcing the manager to drop its in-memory disk bookkeeping), then + // point it back at the original dir. + TempCacheDir other; + CacheManager::instance().setConfig(diskConfig(other.str(), oneImage * 4, 1ULL * 1024 * 1024)); + CacheManager::instance().clear(); + + CacheManager::instance().setConfig(diskConfig(tmp.str(), oneImage * 4, 1ULL * 1024 * 1024)); + CacheManager::instance().resetStats(); + + auto found = CacheManager::instance().findImage(makeSpec("persistent")); + REQUIRE(found != nullptr); + REQUIRE(CacheManager::instance().getStats().diskHits == 1); + } +} + +TEST_CASE("CacheManager invalidates entries when the source file mtime changes", "[cache][mtime]") +{ + resetCache(); + + // Use a real file on disk so the cache key picks up its mtime via + // std::filesystem::last_write_time. We bump the file's mtime explicitly + // (rather than sleeping and rewriting) so the test is deterministic on + // filesystems with coarse mtime resolution. + static std::atomic sCounter{ 0 }; + std::filesystem::path srcFile = std::filesystem::temp_directory_path() / + ("agave_cache_mtime_test_" + std::to_string(sCounter.fetch_add(1)) + ".bin"); + { + std::ofstream out(srcFile.string()); + out << "initial content"; + } + + CacheManager::instance().setConfig(ramOnlyConfig(imageBytes(4, 4, 4, 1) * 4)); + + LoadSpec spec; + spec.filepath = srcFile.string(); + CacheManager::instance().storeImage(spec, makeImage(4, 4, 4, 1)); + REQUIRE(CacheManager::instance().findImage(spec) != nullptr); + + // Bump the file's last_write_time well into the future; subsequent + // makeKey calls now produce a different key and must miss. + std::error_code ec; + auto futureTime = std::filesystem::last_write_time(srcFile, ec) + std::chrono::seconds(60); + REQUIRE_FALSE(ec); + std::filesystem::last_write_time(srcFile, futureTime, ec); + REQUIRE_FALSE(ec); + + REQUIRE(CacheManager::instance().findImage(spec) == nullptr); + + std::filesystem::remove(srcFile, ec); +} From 715b5dec8b06412f048fe5782d1abc87ae5600b2 Mon Sep 17 00:00:00 2001 From: dmt Date: Sun, 24 May 2026 11:14:29 -0700 Subject: [PATCH 14/31] Document the stale-disk-lastAccess edge case for RAM-resident entries MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RAM hits short-circuit before the disk tier is consulted, so the disk lastAccess (and its on-disk meta.json) is never refreshed on a RAM hit. This is fine within a session — when an entry falls out of RAM, the next access goes through loadFromDisk which bumps the disk lastAccess right then. The edge case is at session boundaries: an entry that stays RAM-resident for the whole session has stale disk lastAccess, so the next session's disk LRU may treat it as older than it really was and evict it from disk on cold start. Spelling out the tradeoff plus the fix recipe in a NOTE/TODO so the next person doesn't have to rederive it. Co-Authored-By: Claude Opus 4.7 --- renderlib/CacheManager.cpp | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/renderlib/CacheManager.cpp b/renderlib/CacheManager.cpp index bb1eec5f3..aa75195cc 100644 --- a/renderlib/CacheManager.cpp +++ b/renderlib/CacheManager.cpp @@ -203,6 +203,21 @@ CacheManager::findImage(const LoadSpec& loadSpec) m_stats.ramHits++; LOG_DEBUG << "Cache stats: ram_hits=" << m_stats.ramHits << " disk_hits=" << m_stats.diskHits << " misses=" << m_stats.misses << " disk_writes=" << m_stats.diskWrites; + // NOTE: we deliberately do not refresh the matching DiskEntry's + // lastAccess (or its on-disk meta.json) on a RAM hit. The disk LRU + // is only consulted when an entry has fallen out of RAM, and + // loadFromDisk refreshes the disk lastAccess at that point — so + // within a session the disk bookkeeping is fresh whenever it + // actually matters. + // + // TODO: edge case — an entry that stays RAM-resident for an entire + // session never has its disk lastAccess bumped, so at the next + // session start it can look "older" than entries that were only + // served from disk in the previous session, and may be the first + // thing evicted from disk on cold start. If that ever shows up as + // a real cold-start problem, fix by bumping the in-memory + // DiskEntry.lastAccess here (no on-disk write needed) and letting + // the existing flush-on-eviction-or-shutdown path persist it. return it->second.image; } } From e80498ab090e85745e03b26f1a57af66b5b2b800 Mon Sep 17 00:00:00 2001 From: dmt Date: Sun, 24 May 2026 11:21:54 -0700 Subject: [PATCH 15/31] Normalize filepaths in cache keys so equivalent paths share an entry Adds normalizeFilepath() to the CacheManager anonymous namespace and calls it from makeKey for both the stored filepath and the statForKey lookup, so the cache treats: /some/dir/foo.tif /some/dir/./foo.tif /some//dir///foo.tif /some/dir/x/../foo.tif as the same key, and on Windows also folds together: C:/some/dir/foo.tif C:\some\dir\foo.tif c:/SOME/DIR/foo.tif We use std::filesystem::path::lexically_normal (purely textual) rather than weakly_canonical so that bare-name keys used by loadFromArray_4D (e.g. "my_in_memory_array") don't pick up CWD-relative resolution and change between calls if the process directory moves. Remote URLs (http/s3/gs) are passed through unchanged because lexically_normal would mangle "://" into "//". On Windows we lowercase the result since NTFS/FAT are conventionally case-insensitive (accepting incorrect hits on the rare case-sensitive setups like \wsl$ paths or per-directory case-sensitivity flags). Adds 8 sections to test_cacheManager.cpp covering the equivalence classes above plus a negative test that genuinely distinct paths still produce distinct keys, with Windows-only sections behind #ifdef _WIN32. Co-Authored-By: Claude Opus 4.7 --- renderlib/CacheManager.cpp | 39 +++++++++++- test/test_cacheManager.cpp | 122 +++++++++++++++++++++++++++++++++++++ 2 files changed, 159 insertions(+), 2 deletions(-) diff --git a/renderlib/CacheManager.cpp b/renderlib/CacheManager.cpp index aa75195cc..909e52d9a 100644 --- a/renderlib/CacheManager.cpp +++ b/renderlib/CacheManager.cpp @@ -12,6 +12,7 @@ #include "json/json.hpp" #include +#include #include #include #include @@ -56,6 +57,38 @@ isRemotePath(const std::string& path) return path.rfind("http", 0) == 0 || path.rfind("s3:", 0) == 0 || path.rfind("gs:", 0) == 0; } +// Normalize a filepath into a canonical form for cache key generation. Goals: +// - "/some/dir/./foo", "/some//dir//foo", and "/some/dir/x/../foo" all +// produce the same key (lexically_normal collapses these). +// - On Windows, "C:/foo", "C:\foo", and "c:\foo" all produce the same key +// (path treats both separators; lowercase normalizes case). +// +// We deliberately use lexically_normal (purely textual) rather than +// weakly_canonical, because the latter resolves relative paths against the +// process CWD — which would make bare names like "my_in_memory_array" passed +// to loadFromArray_4D produce different keys when CWD changes. We also pass +// remote URLs through unchanged, since lexically_normal would mangle the +// "://" portion into "//". +std::string +normalizeFilepath(const std::string& path) +{ + if (path.empty() || isRemotePath(path)) { + return path; + } + std::filesystem::path p(path); + std::string normalized = p.lexically_normal().generic_string(); +#ifdef _WIN32 + // NTFS and FAT are conventionally case-insensitive. (Case-sensitive + // Windows configurations — per-directory case sensitivity flag, ReFS, + // WSL paths reached via \\wsl$ — will see incorrect cache hits between + // case-only-different paths. Accept this for the common case.) + std::transform(normalized.begin(), normalized.end(), normalized.begin(), [](unsigned char c) { + return static_cast(std::tolower(c)); + }); +#endif + return normalized; +} + // Returns (mtime_ns, file_size). Either or both may be 0 if the path is // remote, missing, or otherwise unreadable. For directories (zarr) file_size // is 0; mtime is the directory's last_write_time, which most filesystems @@ -376,7 +409,7 @@ CacheKey CacheManager::makeKey(const LoadSpec& loadSpec) const { CacheKey key; - key.filepath = loadSpec.filepath; + key.filepath = normalizeFilepath(loadSpec.filepath); key.subpath = loadSpec.subpath; key.scene = loadSpec.scene; key.time = loadSpec.time; @@ -388,7 +421,9 @@ CacheManager::makeKey(const LoadSpec& loadSpec) const key.minz = loadSpec.minz; key.maxz = loadSpec.maxz; key.isImageSequence = loadSpec.isImageSequence; - auto stat = statForKey(loadSpec.filepath); + // Use the normalized filepath for stat() too so equivalent paths produce + // identical fileMtimeNs / fileSize. + auto stat = statForKey(key.filepath); key.fileMtimeNs = stat.first; key.fileSize = stat.second; return key; diff --git a/test/test_cacheManager.cpp b/test/test_cacheManager.cpp index 31f18d71b..b924fc211 100644 --- a/test/test_cacheManager.cpp +++ b/test/test_cacheManager.cpp @@ -550,3 +550,125 @@ TEST_CASE("CacheManager invalidates entries when the source file mtime changes", std::filesystem::remove(srcFile, ec); } + +TEST_CASE("CacheManager normalizes equivalent filepaths to the same key", "[cache][normalize]") +{ + // These tests use synthetic paths that don't exist on disk; statForKey + // returns (0, 0) for them, so the cache key depends only on the + // (normalized) filepath string and the other LoadSpec fields. + const std::uint64_t oneImage = imageBytes(4, 4, 4, 1); + + SECTION("'./' segment is normalized away") + { + resetCache(); + CacheManager::instance().setConfig(ramOnlyConfig(oneImage * 4)); + + LoadSpec stored; + stored.filepath = "/some/dir/foo.tif"; + CacheManager::instance().storeImage(stored, makeImage(4, 4, 4, 1)); + + LoadSpec lookup; + lookup.filepath = "/some/dir/./foo.tif"; + REQUIRE(CacheManager::instance().findImage(lookup) != nullptr); + } + + SECTION("'..' segment is normalized away") + { + resetCache(); + CacheManager::instance().setConfig(ramOnlyConfig(oneImage * 4)); + + LoadSpec stored; + stored.filepath = "/some/dir/foo.tif"; + CacheManager::instance().storeImage(stored, makeImage(4, 4, 4, 1)); + + LoadSpec lookup; + lookup.filepath = "/some/dir/subdir/../foo.tif"; + REQUIRE(CacheManager::instance().findImage(lookup) != nullptr); + } + + SECTION("Duplicate path separators are collapsed") + { + resetCache(); + CacheManager::instance().setConfig(ramOnlyConfig(oneImage * 4)); + + LoadSpec stored; + stored.filepath = "/some/dir/foo.tif"; + CacheManager::instance().storeImage(stored, makeImage(4, 4, 4, 1)); + + LoadSpec lookup; + lookup.filepath = "/some//dir///foo.tif"; + REQUIRE(CacheManager::instance().findImage(lookup) != nullptr); + } + + SECTION("Bare names (no slashes) pass through unchanged") + { + resetCache(); + CacheManager::instance().setConfig(ramOnlyConfig(oneImage * 4)); + + LoadSpec stored; + stored.filepath = "in_memory_array_42"; + CacheManager::instance().storeImage(stored, makeImage(4, 4, 4, 1)); + + LoadSpec lookup; + lookup.filepath = "in_memory_array_42"; + REQUIRE(CacheManager::instance().findImage(lookup) != nullptr); + } + + SECTION("Remote URLs are passed through without normalization") + { + resetCache(); + CacheManager::instance().setConfig(ramOnlyConfig(oneImage * 4)); + + LoadSpec stored; + stored.filepath = "http://example.com/path/to/data.zarr"; + CacheManager::instance().storeImage(stored, makeImage(4, 4, 4, 1)); + + LoadSpec lookup; + lookup.filepath = "http://example.com/path/to/data.zarr"; + REQUIRE(CacheManager::instance().findImage(lookup) != nullptr); + } + + SECTION("Distinct paths still produce distinct keys (no false hits)") + { + resetCache(); + CacheManager::instance().setConfig(ramOnlyConfig(oneImage * 4)); + + LoadSpec stored; + stored.filepath = "/some/dir/foo.tif"; + CacheManager::instance().storeImage(stored, makeImage(4, 4, 4, 1)); + + LoadSpec lookup; + lookup.filepath = "/some/dir/bar.tif"; + REQUIRE(CacheManager::instance().findImage(lookup) == nullptr); + } + +#ifdef _WIN32 + SECTION("Forward and back slashes are equivalent on Windows") + { + resetCache(); + CacheManager::instance().setConfig(ramOnlyConfig(oneImage * 4)); + + LoadSpec stored; + stored.filepath = "C:/some/dir/foo.tif"; + CacheManager::instance().storeImage(stored, makeImage(4, 4, 4, 1)); + + LoadSpec lookup; + lookup.filepath = "C:\\some\\dir\\foo.tif"; + REQUIRE(CacheManager::instance().findImage(lookup) != nullptr); + } + + SECTION("Case differences are equivalent on Windows") + { + resetCache(); + CacheManager::instance().setConfig(ramOnlyConfig(oneImage * 4)); + + LoadSpec stored; + stored.filepath = "C:/Some/Dir/Foo.tif"; + CacheManager::instance().storeImage(stored, makeImage(4, 4, 4, 1)); + + LoadSpec lookup; + lookup.filepath = "c:/some/dir/foo.tif"; + REQUIRE(CacheManager::instance().findImage(lookup) != nullptr); + } +#endif +} From b0a52715d02499a0efe65c1a437d8365a1783cf0 Mon Sep 17 00:00:00 2001 From: dmt Date: Sun, 24 May 2026 11:24:36 -0700 Subject: [PATCH 16/31] Add UNC path test for cache key normalization MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Locks in the expected Windows behavior for UNC paths: - \Server\Share\Path\Foo.tif, //server/share/path/foo.tif, and \server\share\path\subdir\..\foo.tif all collapse to one key (slash style + case fold + '..' collapse). - A different share name (\server\othershare\...) still produces a distinct key, so we're not over-collapsing. The normalization path (std::filesystem::path::lexically_normal + generic_string + lowercase on Windows) preserves UNC roots — the leading \\ becomes // after generic_string, which Windows APIs accept as equivalent. Co-Authored-By: Claude Opus 4.7 --- test/test_cacheManager.cpp | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/test/test_cacheManager.cpp b/test/test_cacheManager.cpp index b924fc211..8f4abdc31 100644 --- a/test/test_cacheManager.cpp +++ b/test/test_cacheManager.cpp @@ -643,6 +643,31 @@ TEST_CASE("CacheManager normalizes equivalent filepaths to the same key", "[cach } #ifdef _WIN32 + SECTION("UNC paths normalize across slash style and case on Windows") + { + resetCache(); + CacheManager::instance().setConfig(ramOnlyConfig(oneImage * 4)); + + LoadSpec stored; + stored.filepath = "\\\\Server\\Share\\Path\\Foo.tif"; + CacheManager::instance().storeImage(stored, makeImage(4, 4, 4, 1)); + + // Same UNC path, lowercase, mixed slashes. + LoadSpec lookupSlash; + lookupSlash.filepath = "//server/share/path/foo.tif"; + REQUIRE(CacheManager::instance().findImage(lookupSlash) != nullptr); + + // Same UNC path, with a '..' segment in the relative tail. + LoadSpec lookupDotDot; + lookupDotDot.filepath = "\\\\server\\share\\path\\subdir\\..\\foo.tif"; + REQUIRE(CacheManager::instance().findImage(lookupDotDot) != nullptr); + + // Different UNC share — must NOT collide. + LoadSpec otherShare; + otherShare.filepath = "\\\\server\\othershare\\path\\foo.tif"; + REQUIRE(CacheManager::instance().findImage(otherShare) == nullptr); + } + SECTION("Forward and back slashes are equivalent on Windows") { resetCache(); From a3e34de6ad0e0ba4d4dc9480a70ae4c186052916 Mon Sep 17 00:00:00 2001 From: dmt Date: Sun, 24 May 2026 16:01:57 -0700 Subject: [PATCH 17/31] Rename CacheManager::clear -> clearMemoryCache for symmetry with clearDiskCache MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous "clear()" cleared only the in-memory tier, not the disk tier — surprising given that a sibling clearDiskCache() exists. Rename to make the scope explicit at the call site, document both methods' scopes on the header, and update all six test callers plus the one SECTION title. Co-Authored-By: Claude Opus 4.7 --- renderlib/CacheManager.cpp | 2 +- renderlib/CacheManager.h | 5 ++++- test/test_cacheManager.cpp | 14 +++++++------- 3 files changed, 12 insertions(+), 9 deletions(-) diff --git a/renderlib/CacheManager.cpp b/renderlib/CacheManager.cpp index 909e52d9a..45474b03c 100644 --- a/renderlib/CacheManager.cpp +++ b/renderlib/CacheManager.cpp @@ -301,7 +301,7 @@ CacheManager::storeImage(const LoadSpec& loadSpec, const std::shared_ptr lock(m_mutex); m_entries.clear(); diff --git a/renderlib/CacheManager.h b/renderlib/CacheManager.h index cf5aad239..ce2692534 100644 --- a/renderlib/CacheManager.h +++ b/renderlib/CacheManager.h @@ -62,7 +62,10 @@ class CacheManager std::shared_ptr findImage(const LoadSpec& loadSpec); void storeImage(const LoadSpec& loadSpec, const std::shared_ptr& image); - void clear(); + // Drop all entries from the in-memory cache. Disk cache is untouched. + void clearMemoryCache(); + // Drop all entries from the disk cache (refuses if the cache directory is + // missing the AGAVE marker file). Memory cache is untouched. void clearDiskCache(); CacheStats getStats() const; diff --git a/test/test_cacheManager.cpp b/test/test_cacheManager.cpp index 8f4abdc31..2e53688fc 100644 --- a/test/test_cacheManager.cpp +++ b/test/test_cacheManager.cpp @@ -51,7 +51,7 @@ imageBytes(uint32_t x, uint32_t y, uint32_t z, uint32_t c) void resetCache() { - CacheManager::instance().clear(); + CacheManager::instance().clearMemoryCache(); CacheManager::instance().resetStats(); } @@ -345,14 +345,14 @@ TEST_CASE("CacheManager respects RAM limit and evicts LRU entries", "[cache]") REQUIRE(CacheManager::instance().findImage(makeSpec("c")) == nullptr); } - SECTION("clear() empties the cache") + SECTION("clearMemoryCache() empties the cache") { resetCache(); CacheManager::instance().setConfig(ramOnlyConfig(oneImage * 4)); CacheManager::instance().storeImage(makeSpec("a"), makeImage(4, 4, 4, 1)); CacheManager::instance().storeImage(makeSpec("b"), makeImage(4, 4, 4, 1)); - CacheManager::instance().clear(); + CacheManager::instance().clearMemoryCache(); REQUIRE(CacheManager::instance().findImage(makeSpec("a")) == nullptr); REQUIRE(CacheManager::instance().findImage(makeSpec("b")) == nullptr); @@ -388,7 +388,7 @@ TEST_CASE("CacheManager disk tier round-trips images and respects the disk cap", CacheManager::instance().storeImage(spec, img); // Drop RAM cache; disk cache survives. - CacheManager::instance().clear(); + CacheManager::instance().clearMemoryCache(); CacheManager::instance().resetStats(); auto found = CacheManager::instance().findImage(spec); @@ -424,7 +424,7 @@ TEST_CASE("CacheManager disk tier round-trips images and respects the disk cap", // Drop RAM to force a disk-or-miss lookup; with no entry on disk we // should get a miss. - CacheManager::instance().clear(); + CacheManager::instance().clearMemoryCache(); REQUIRE(CacheManager::instance().findImage(spec) == nullptr); } @@ -443,7 +443,7 @@ TEST_CASE("CacheManager disk tier round-trips images and respects the disk cap", CacheManager::instance().storeImage(makeSpec("disk_c"), makeImage(4, 4, 4, 1)); // Drop RAM so finds have to go through the disk tier. - CacheManager::instance().clear(); + CacheManager::instance().clearMemoryCache(); REQUIRE(CacheManager::instance().findImage(makeSpec("disk_a")) == nullptr); REQUIRE(CacheManager::instance().findImage(makeSpec("disk_b")) != nullptr); @@ -504,7 +504,7 @@ TEST_CASE("CacheManager disk tier round-trips images and respects the disk cap", // point it back at the original dir. TempCacheDir other; CacheManager::instance().setConfig(diskConfig(other.str(), oneImage * 4, 1ULL * 1024 * 1024)); - CacheManager::instance().clear(); + CacheManager::instance().clearMemoryCache(); CacheManager::instance().setConfig(diskConfig(tmp.str(), oneImage * 4, 1ULL * 1024 * 1024)); CacheManager::instance().resetStats(); From e35f84d96f89c34bf85119d6bd47a9a17b50b20a Mon Sep 17 00:00:00 2001 From: toloudis Date: Tue, 26 May 2026 17:51:02 -0700 Subject: [PATCH 18/31] ensure that bpp loaded from cache is 16bpp --- renderlib/CacheManager.cpp | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/renderlib/CacheManager.cpp b/renderlib/CacheManager.cpp index 45474b03c..ace2f6d05 100644 --- a/renderlib/CacheManager.cpp +++ b/renderlib/CacheManager.cpp @@ -555,6 +555,11 @@ CacheManager::loadFromDisk(const CacheKey& key, const CacheConfig& config) std::uint32_t sizeZ = meta["sizeZ"].get(); std::uint32_t sizeC = meta["sizeC"].get(); std::uint32_t bpp = meta.value("bpp", static_cast(ImageXYZC::IN_MEMORY_BPP)); + if (bpp != static_cast(ImageXYZC::IN_MEMORY_BPP)) { + LOG_ERROR << "Disk cache load: unsupported bpp " << bpp << " in " << metaPath.string() << " (expected " + << ImageXYZC::IN_MEMORY_BPP << "); skipping cache entry"; + return nullptr; + } float sx = meta.value("physicalSizeX", 1.0f); float sy = meta.value("physicalSizeY", 1.0f); float sz = meta.value("physicalSizeZ", 1.0f); @@ -630,8 +635,8 @@ CacheManager::storeToDisk(const CacheKey& key, const std::shared_ptr& if (bytes > config.maxDiskBytes) { // A single image larger than the entire disk cap; refuse rather than // evict everything and overshoot. - LOG_WARNING << "Disk cache: skipping store of " << bytes - << " byte image — larger than disk cap " << config.maxDiskBytes; + LOG_WARNING << "Disk cache: skipping store of " << bytes << " byte image — larger than disk cap " + << config.maxDiskBytes; return; } From bbd86bce819d8e6e7f890c2d60197fcac77b3792 Mon Sep 17 00:00:00 2001 From: toloudis Date: Tue, 26 May 2026 18:11:52 -0700 Subject: [PATCH 19/31] allow the timeline widget to keep reusing its cached reader --- agave_app/TimelineDockWidget.cpp | 4 +++- renderlib/io/FileReader.cpp | 19 ++++++++++++------- renderlib/io/FileReader.h | 6 +++++- 3 files changed, 20 insertions(+), 9 deletions(-) diff --git a/agave_app/TimelineDockWidget.cpp b/agave_app/TimelineDockWidget.cpp index 8cee28d7d..8ad1112b0 100644 --- a/agave_app/TimelineDockWidget.cpp +++ b/agave_app/TimelineDockWidget.cpp @@ -83,7 +83,9 @@ QTimelineWidget::OnTimeChanged(int newTime) loadSpec.time = newTime; QApplication::setOverrideCursor(QCursor(Qt::WaitCursor)); - std::shared_ptr image = FileReader::loadAndCache(loadSpec); + // Reuse the reader created at file-open time so we skip re-opening the file + // and re-parsing metadata on every time-step. + std::shared_ptr image = FileReader::loadAndCache(loadSpec, m_reader); QApplication::restoreOverrideCursor(); if (!image) { // TODO FIXME if we fail to set the new time, then reset the GUI to previous time diff --git a/renderlib/io/FileReader.cpp b/renderlib/io/FileReader.cpp index 77bf1d97d..e0c2ceab9 100644 --- a/renderlib/io/FileReader.cpp +++ b/renderlib/io/FileReader.cpp @@ -64,23 +64,28 @@ FileReader::getReader(const std::string& filepath, bool isImageSequence) } std::shared_ptr -FileReader::loadAndCache(const LoadSpec& loadSpec) +FileReader::loadAndCache(const LoadSpec& loadSpec, std::shared_ptr reader) { auto cached = CacheManager::instance().findImage(loadSpec); if (cached) { return cached; } - VolumeDimensions dims; - std::shared_ptr image; - std::string filepath = loadSpec.filepath; + const std::string& filepath = loadSpec.filepath; - std::unique_ptr reader(FileReader::getReader(filepath, loadSpec.isImageSequence)); + // Fall back to constructing a new reader if the caller didn't supply one. + // A reused reader can skip re-opening the file and re-parsing metadata (notably + // valuable for time-series stepping on OME-Zarr/TIFF/CZI). + std::shared_ptr ownedReader; if (!reader) { - LOG_ERROR << "Could not find a reader for file " << filepath; - return nullptr; + ownedReader.reset(FileReader::getReader(filepath, loadSpec.isImageSequence)); + if (!ownedReader) { + LOG_ERROR << "Could not find a reader for file " << filepath; + return nullptr; + } + reader = ownedReader; } image = reader->loadFromFile(loadSpec); diff --git a/renderlib/io/FileReader.h b/renderlib/io/FileReader.h index 26c20dfcd..ec41c6bc8 100644 --- a/renderlib/io/FileReader.h +++ b/renderlib/io/FileReader.h @@ -18,7 +18,11 @@ class FileReader static IFileReader* getReader(const std::string& filepath, bool isImageSequence = false); - static std::shared_ptr loadAndCache(const LoadSpec& loadSpec); + // If `reader` is provided, it is reused to load the image (skipping the cost of + // re-opening the file and re-parsing metadata). If null, a new reader is constructed + // via getReader(). The result is stored in the CacheManager on success either way. + static std::shared_ptr loadAndCache(const LoadSpec& loadSpec, + std::shared_ptr reader = nullptr); static std::shared_ptr loadFromArray_4D(uint8_t* dataArray, std::vector shape, From 654bbe075e1fdc396b9c76b493f4f723529dedcb Mon Sep 17 00:00:00 2001 From: toloudis Date: Tue, 26 May 2026 18:17:14 -0700 Subject: [PATCH 20/31] reuse existing reader --- agave_app/agaveGui.cpp | 14 +++++++------- renderlib/command.cpp | 4 ++-- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/agave_app/agaveGui.cpp b/agave_app/agaveGui.cpp index eb7489c42..eef058d64 100644 --- a/agave_app/agaveGui.cpp +++ b/agave_app/agaveGui.cpp @@ -113,12 +113,12 @@ agaveGui::agaveGui(QWidget* parent) // may differ from the widget's current text if the user edited the path // without clicking Apply. QString cacheDir = QString::fromStdString(CacheManager::instance().getConfig().cacheDir); - QMessageBox::StandardButton reply = QMessageBox::question( - this, - tr("Clear disk cache"), - tr("Delete all cached volume data in:\n%1\n\nThis cannot be undone.").arg(cacheDir), - QMessageBox::Yes | QMessageBox::No, - QMessageBox::No); + QMessageBox::StandardButton reply = + QMessageBox::question(this, + tr("Clear disk cache"), + tr("Delete all cached volume data in:\n%1\n\nThis cannot be undone.").arg(cacheDir), + QMessageBox::Yes | QMessageBox::No, + QMessageBox::No); if (reply == QMessageBox::Yes) { CacheManager::instance().clearDiskCache(); } @@ -861,7 +861,7 @@ agaveGui::open(const std::string& file, const Serialize::ViewerState* vs, bool i // We can update the render and gui progressively as chunks are loaded. // Also, this would allow renders to be cancelled during loading. QApplication::setOverrideCursor(QCursor(Qt::WaitCursor)); - std::shared_ptr image = FileReader::loadAndCache(loadSpec); + std::shared_ptr image = FileReader::loadAndCache(loadSpec, reader); QApplication::restoreOverrideCursor(); if (!image) { LOG_DEBUG << "Failed to open " << file; diff --git a/renderlib/command.cpp b/renderlib/command.cpp index 0db0fc5af..788f3d585 100644 --- a/renderlib/command.cpp +++ b/renderlib/command.cpp @@ -663,7 +663,7 @@ LoadDataCommand::execute(ExecutionContext* c) c->m_loadSpec.maxz = m_data.m_zmax; // TODO can we load time sequences of separate files here? - std::unique_ptr reader(FileReader::getReader(m_data.m_path)); + std::shared_ptr reader(FileReader::getReader(m_data.m_path)); if (!reader) { LOG_ERROR << "Could not find a reader for file " << m_data.m_path; return; @@ -671,7 +671,7 @@ LoadDataCommand::execute(ExecutionContext* c) VolumeDimensions dims = reader->loadDimensions(m_data.m_path, m_data.m_scene); - std::shared_ptr image = FileReader::loadAndCache(c->m_loadSpec); + std::shared_ptr image = FileReader::loadAndCache(c->m_loadSpec, reader); if (!image) { return; } From a0962576152c77bf4d1156d8afc72c74aea053d6 Mon Sep 17 00:00:00 2001 From: toloudis Date: Tue, 26 May 2026 18:25:47 -0700 Subject: [PATCH 21/31] DRY: only call makeKey once Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- renderlib/CacheManager.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/renderlib/CacheManager.cpp b/renderlib/CacheManager.cpp index ace2f6d05..98a918aa7 100644 --- a/renderlib/CacheManager.cpp +++ b/renderlib/CacheManager.cpp @@ -293,11 +293,13 @@ CacheManager::storeImage(const LoadSpec& loadSpec, const std::shared_ptr 0) { - storeToDisk(makeKey(loadSpec), image, configCopy); + storeToDisk(key, image, configCopy); } - storeImageInMemory(makeKey(loadSpec), image); + storeImageInMemory(key, image); } void From cca35505f83aa4abbf28b880b7a7c157b0624f77 Mon Sep 17 00:00:00 2001 From: toloudis Date: Tue, 26 May 2026 18:30:30 -0700 Subject: [PATCH 22/31] move the cache dir validity check earlier --- renderlib/CacheManager.cpp | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/renderlib/CacheManager.cpp b/renderlib/CacheManager.cpp index ace2f6d05..1b0ca756f 100644 --- a/renderlib/CacheManager.cpp +++ b/renderlib/CacheManager.cpp @@ -317,6 +317,13 @@ CacheManager::clearDiskCache() { std::lock_guard lock(m_mutex); cacheDir = m_config.cacheDir; + + if (!isAgaveCacheDir(cacheDir)) { + LOG_WARNING << "Refusing to clear disk cache: directory missing AGAVE cache marker file (" << kCacheMarkerFilename + << "): " << cacheDir; + return; + } + knownEntryPaths.reserve(m_diskEntries.size()); for (const auto& kv : m_diskEntries) { knownEntryPaths.push_back(kv.second.path); @@ -329,12 +336,6 @@ CacheManager::clearDiskCache() return; } - if (!isAgaveCacheDir(cacheDir)) { - LOG_WARNING << "Refusing to clear disk cache: directory missing AGAVE cache marker file (" << kCacheMarkerFilename - << "): " << cacheDir; - return; - } - // Remove the per-entry subdirectories we know about. for (const auto& path : knownEntryPaths) { std::error_code ec; From 86304d59ded63b7896f7cf991440867ca573c64f Mon Sep 17 00:00:00 2001 From: toloudis Date: Tue, 26 May 2026 18:42:45 -0700 Subject: [PATCH 23/31] refactor a function --- agave_app/CacheSettings.cpp | 27 +-------------------------- agave_app/CacheSettings.h | 3 --- renderlib/CacheManager.cpp | 36 ++++++++++++++++++++++++++++++++++++ renderlib/CacheManager.h | 5 +++++ 4 files changed, 42 insertions(+), 29 deletions(-) diff --git a/agave_app/CacheSettings.cpp b/agave_app/CacheSettings.cpp index 0522b6f5c..cbc981c7c 100644 --- a/agave_app/CacheSettings.cpp +++ b/agave_app/CacheSettings.cpp @@ -150,7 +150,7 @@ CacheSettings::applyToRenderlib(const CacheSettingsData& data) const { ::CacheConfig config = toRenderlibConfig(*this, data); if (config.enableDisk && !config.cacheDir.empty()) { - if (!canWriteCacheDir(config.cacheDir)) { + if (!CacheManager::canWriteCacheDir(config.cacheDir)) { LOG_WARNING << "Cache disk disabled: cache directory not writable: " << config.cacheDir; config.enableDisk = false; config.maxDiskBytes = 0; @@ -165,28 +165,3 @@ CacheSettings::applyToRenderlib(const CacheSettingsData& data) const << " disk_enabled=" << (applied.enableDisk ? 1 : 0) << " disk_bytes=" << applied.maxDiskBytes << " cache_dir=" << applied.cacheDir; } - -bool -CacheSettings::canWriteCacheDir(const std::string& path) const -{ - if (path.empty()) { - return false; - } - - QString qPath = QString::fromStdString(path); - QDir dir(qPath); - if (!dir.exists()) { - if (!dir.mkpath(".")) { - return false; - } - } - - QString testPath = dir.filePath(".agave_cache_write_test"); - QFile testFile(testPath); - if (!testFile.open(QIODevice::WriteOnly | QIODevice::Truncate)) { - return false; - } - testFile.write("test"); - testFile.close(); - return testFile.remove(); -} diff --git a/agave_app/CacheSettings.h b/agave_app/CacheSettings.h index bfac1329d..90b03c7b0 100644 --- a/agave_app/CacheSettings.h +++ b/agave_app/CacheSettings.h @@ -24,7 +24,4 @@ class CacheSettings CacheSettingsData defaultSettings() const; std::string configPath() const; - -private: - bool canWriteCacheDir(const std::string& path) const; }; diff --git a/renderlib/CacheManager.cpp b/renderlib/CacheManager.cpp index e1c06be1c..cb6170c30 100644 --- a/renderlib/CacheManager.cpp +++ b/renderlib/CacheManager.cpp @@ -174,6 +174,42 @@ CacheManager::instance() return manager; } +bool +CacheManager::canWriteCacheDir(const std::string& path) +{ + if (path.empty()) { + return false; + } + + std::error_code ec; + std::filesystem::path dir(path); + if (!std::filesystem::exists(dir, ec)) { + if (!std::filesystem::create_directories(dir, ec) || ec) { + LOG_WARNING << "canWriteCacheDir: failed to create " << path << ": " << ec.message(); + return false; + } + } else if (!std::filesystem::is_directory(dir, ec) || ec) { + LOG_WARNING << "canWriteCacheDir: not a directory: " << path; + return false; + } + + std::filesystem::path testPath = dir / ".agave_cache_write_test"; + { + std::ofstream testFile(testPath, std::ios::binary | std::ios::trunc); + if (!testFile.is_open()) { + return false; + } + testFile << "test"; + if (!testFile.good()) { + testFile.close(); + std::filesystem::remove(testPath, ec); + return false; + } + } + std::filesystem::remove(testPath, ec); + return !ec; +} + void CacheManager::setConfig(const CacheConfig& config) { diff --git a/renderlib/CacheManager.h b/renderlib/CacheManager.h index ce2692534..ccc380469 100644 --- a/renderlib/CacheManager.h +++ b/renderlib/CacheManager.h @@ -57,6 +57,11 @@ class CacheManager static CacheManager& instance(); + // Verify that `path` is (or can be made) a writable directory. Creates the + // directory if it does not exist, then probes it by writing and deleting a + // small marker file. Returns false on any failure. + static bool canWriteCacheDir(const std::string& path); + void setConfig(const CacheConfig& config); CacheConfig getConfig() const; From 0b4f9b5725d9f04a4892fa6bc11fa37550422289 Mon Sep 17 00:00:00 2001 From: toloudis Date: Mon, 1 Jun 2026 16:05:13 -0700 Subject: [PATCH 24/31] don't allow changing the cache settings during rendering from Render Dialog --- agave_app/agaveGui.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/agave_app/agaveGui.cpp b/agave_app/agaveGui.cpp index eef058d64..0b169ddc3 100644 --- a/agave_app/agaveGui.cpp +++ b/agave_app/agaveGui.cpp @@ -629,6 +629,7 @@ agaveGui::onRenderAction() m_glView->doneCurrent(); m_glView->setEnabled(false); m_glView->setUpdatesEnabled(false); + m_cacheSettingsDockWidget->setEnabled(false); if (m_captureSettings.width == 0 && m_captureSettings.height == 0) { m_captureSettings.width = m_glView->width(); m_captureSettings.height = m_glView->height(); @@ -663,6 +664,7 @@ agaveGui::onRenderAction() m_renderSettings.m_DirtyFlags.SetFlag(LightsDirty); m_renderSettings.m_DirtyFlags.SetFlag(RenderParamsDirty); m_renderSettings.m_DirtyFlags.SetFlag(TransferFunctionDirty); + m_cacheSettingsDockWidget->setEnabled(true); m_glView->setEnabled(true); m_glView->resizeGL(m_glView->width(), m_glView->height()); m_glView->setUpdatesEnabled(true); From 7a2605a86b2b3fa708f924cf14f8d2327e15af01 Mon Sep 17 00:00:00 2001 From: toloudis Date: Mon, 1 Jun 2026 16:11:22 -0700 Subject: [PATCH 25/31] prefer scoped_lock --- renderlib/CacheManager.cpp | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/renderlib/CacheManager.cpp b/renderlib/CacheManager.cpp index cb6170c30..3244304e9 100644 --- a/renderlib/CacheManager.cpp +++ b/renderlib/CacheManager.cpp @@ -216,7 +216,7 @@ CacheManager::setConfig(const CacheConfig& config) CacheConfig configCopy; bool rebuildDiskIndex = false; { - std::lock_guard lock(m_mutex); + std::scoped_lock lock(m_mutex); m_config = config; if (!m_config.enabled) { m_entries.clear(); @@ -246,14 +246,14 @@ CacheManager::setConfig(const CacheConfig& config) evictDiskIfNeeded(configCopy, 0); } - std::lock_guard lock(m_mutex); + std::scoped_lock lock(m_mutex); evictIfNeededLocked(0); } CacheConfig CacheManager::getConfig() const { - std::lock_guard lock(m_mutex); + std::scoped_lock lock(m_mutex); return m_config; } @@ -263,7 +263,7 @@ CacheManager::findImage(const LoadSpec& loadSpec) CacheConfig configCopy; CacheKey key = makeKey(loadSpec); { - std::lock_guard lock(m_mutex); + std::scoped_lock lock(m_mutex); configCopy = m_config; if (m_config.enabled && m_config.maxRamBytes > 0) { auto it = m_entries.find(key); @@ -296,7 +296,7 @@ CacheManager::findImage(const LoadSpec& loadSpec) auto diskImage = loadFromDisk(key, configCopy); if (diskImage) { { - std::lock_guard lock(m_mutex); + std::scoped_lock lock(m_mutex); m_stats.diskHits++; LOG_DEBUG << "Cache stats: ram_hits=" << m_stats.ramHits << " disk_hits=" << m_stats.diskHits << " misses=" << m_stats.misses << " disk_writes=" << m_stats.diskWrites; @@ -307,7 +307,7 @@ CacheManager::findImage(const LoadSpec& loadSpec) } { - std::lock_guard lock(m_mutex); + std::scoped_lock lock(m_mutex); m_stats.misses++; LOG_DEBUG << "Cache stats: ram_hits=" << m_stats.ramHits << " disk_hits=" << m_stats.diskHits << " misses=" << m_stats.misses << " disk_writes=" << m_stats.diskWrites; @@ -325,7 +325,7 @@ CacheManager::storeImage(const LoadSpec& loadSpec, const std::shared_ptr lock(m_mutex); + std::scoped_lock lock(m_mutex); configCopy = m_config; } @@ -341,7 +341,7 @@ CacheManager::storeImage(const LoadSpec& loadSpec, const std::shared_ptr lock(m_mutex); + std::scoped_lock lock(m_mutex); m_entries.clear(); m_lruKeys.clear(); m_currentRamBytes = 0; @@ -353,7 +353,7 @@ CacheManager::clearDiskCache() std::string cacheDir; std::vector knownEntryPaths; { - std::lock_guard lock(m_mutex); + std::scoped_lock lock(m_mutex); cacheDir = m_config.cacheDir; if (!isAgaveCacheDir(cacheDir)) { @@ -433,14 +433,14 @@ CacheManager::isAgaveCacheDir(const std::string& path) const CacheManager::CacheStats CacheManager::getStats() const { - std::lock_guard lock(m_mutex); + std::scoped_lock lock(m_mutex); return m_stats; } void CacheManager::resetStats() { - std::lock_guard lock(m_mutex); + std::scoped_lock lock(m_mutex); m_stats = CacheStats{}; } @@ -531,7 +531,7 @@ CacheManager::evictIfNeededLocked(std::uint64_t incomingBytes) void CacheManager::storeImageInMemory(const CacheKey& key, const std::shared_ptr& image) { - std::lock_guard lock(m_mutex); + std::scoped_lock lock(m_mutex); if (!m_config.enabled || m_config.maxRamBytes == 0) { return; } @@ -650,7 +650,7 @@ CacheManager::loadFromDisk(const CacheKey& key, const CacheConfig& config) } { - std::lock_guard lock(m_mutex); + std::scoped_lock lock(m_mutex); auto it = m_diskEntries.find(diskCacheId(key)); if (it != m_diskEntries.end()) { it->second.lastAccess = meta["lastAccess"].get(); @@ -758,7 +758,7 @@ CacheManager::storeToDisk(const CacheKey& key, const std::shared_ptr& } { - std::lock_guard lock(m_mutex); + std::scoped_lock lock(m_mutex); m_stats.diskWrites++; DiskEntry entry; entry.path = entryPath.string(); @@ -830,7 +830,7 @@ CacheManager::loadDiskIndex(const CacheConfig& config) } } - std::lock_guard lock(m_mutex); + std::scoped_lock lock(m_mutex); m_diskEntries = std::move(entries); m_currentDiskBytes = totalBytes; } @@ -846,7 +846,7 @@ CacheManager::evictDiskIfNeeded(const CacheConfig& config, std::uint64_t incomin // re-populate an entry we're about to delete on disk (which would // otherwise let us nuke the fresh file). Each per-entry remove_all is a // single small zarr directory, so keeping the lock held is acceptable. - std::lock_guard lock(m_mutex); + std::scoped_lock lock(m_mutex); if ((m_currentDiskBytes + incomingBytes) <= config.maxDiskBytes) { return; From 70e79b1a32d6a8dfd8d66abd09ae282925b2614b Mon Sep 17 00:00:00 2001 From: toloudis Date: Mon, 1 Jun 2026 16:56:04 -0700 Subject: [PATCH 26/31] handle error --- renderlib/CacheManager.cpp | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/renderlib/CacheManager.cpp b/renderlib/CacheManager.cpp index 3244304e9..321f9ea1e 100644 --- a/renderlib/CacheManager.cpp +++ b/renderlib/CacheManager.cpp @@ -795,7 +795,12 @@ CacheManager::loadDiskIndex(const CacheConfig& config) std::unordered_map entries; std::uint64_t totalBytes = 0; - for (const auto& dirEntry : std::filesystem::directory_iterator(root)) { + std::error_code iterEc; + for (const auto& dirEntry : std::filesystem::directory_iterator(root, iterEc)) { + if (iterEc) { + LOG_WARNING << "loadDiskIndex: error reading cache directory " << root.string() << ": " << iterEc.message(); + break; + } if (!dirEntry.is_directory()) { continue; } From 0ea24c4f9d90062fb8a66cf7934255943e2b5536 Mon Sep 17 00:00:00 2001 From: toloudis Date: Mon, 1 Jun 2026 16:56:15 -0700 Subject: [PATCH 27/31] better macos instructions --- AGENTS.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index dcc3d4bbb..56a8f8b95 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -34,8 +34,8 @@ aqt install-qt --outputdir ~/Qt mac desktop 6.9.3 -m qtwebsockets qtimageformats export Qt6_DIR=~/Qt/6.9.3/macos mkdir build && cd build -cmake .. -make +cmake .. -G Ninja +cmake --build . ``` ### Windows From f25a1f551fed90a434bb4559136a47d134266074 Mon Sep 17 00:00:00 2001 From: toloudis Date: Tue, 2 Jun 2026 17:58:11 -0700 Subject: [PATCH 28/31] don't let users select cache directory --- agave_app/CacheSettings.cpp | 12 +++++------- agave_app/CacheSettingsWidget.cpp | 29 +++++++---------------------- agave_app/CacheSettingsWidget.h | 8 ++------ 3 files changed, 14 insertions(+), 35 deletions(-) diff --git a/agave_app/CacheSettings.cpp b/agave_app/CacheSettings.cpp index cbc981c7c..286b7c04a 100644 --- a/agave_app/CacheSettings.cpp +++ b/agave_app/CacheSettings.cpp @@ -16,9 +16,7 @@ ::CacheConfig toRenderlibConfig(const CacheSettings& settings, const CacheSettingsData& data) { CacheSettingsData normalized = data; - if (normalized.cacheDir.empty()) { - normalized.cacheDir = settings.defaultSettings().cacheDir; - } + normalized.cacheDir = settings.defaultSettings().cacheDir; ::CacheConfig config; config.enabled = normalized.enabled; @@ -115,13 +113,14 @@ CacheSettings::load() if (doc.contains("maxDiskBytes")) { data.maxDiskBytes = doc["maxDiskBytes"].get(); } - if (doc.contains("cacheDir")) { - data.cacheDir = doc["cacheDir"].get(); - } } catch (...) { return defaultSettings(); } + // Cache files are not user data. Always use AGAVE's platform-selected cache + // directory and ignore custom paths from older settings files. + data.cacheDir = defaultSettings().cacheDir; + return data; } @@ -133,7 +132,6 @@ CacheSettings::save(const CacheSettingsData& data) const doc["enableDisk"] = data.enableDisk; doc["maxRamBytes"] = data.maxRamBytes; doc["maxDiskBytes"] = data.maxDiskBytes; - doc["cacheDir"] = data.cacheDir; QString path = QString::fromStdString(configPath()); QFile file(path); diff --git a/agave_app/CacheSettingsWidget.cpp b/agave_app/CacheSettingsWidget.cpp index e824b86ba..ffa532b53 100644 --- a/agave_app/CacheSettingsWidget.cpp +++ b/agave_app/CacheSettingsWidget.cpp @@ -1,9 +1,6 @@ #include "CacheSettingsWidget.h" -#include #include -#include -#include CacheSettingsWidget::CacheSettingsWidget(QWidget* parent) : QWidget(parent) @@ -23,13 +20,10 @@ CacheSettingsWidget::CacheSettingsWidget(QWidget* parent) m_diskLimitGB->setSuffix(tr(" GB")); m_diskLimitGB->setSingleStep(10); - m_cacheDirEdit = new QLineEdit(this); - m_browseButton = new QPushButton(tr("Browse"), this); - connect(m_browseButton, &QPushButton::clicked, this, &CacheSettingsWidget::browseForCacheDir); - - auto* dirLayout = new QHBoxLayout(); - dirLayout->addWidget(m_cacheDirEdit); - dirLayout->addWidget(m_browseButton); + m_cacheDirLabel = new QLabel(this); + m_cacheDirLabel->setText(QString::fromStdString(CacheSettings().defaultSettings().cacheDir)); + m_cacheDirLabel->setTextInteractionFlags(Qt::TextSelectableByMouse); + m_cacheDirLabel->setWordWrap(true); m_applyButton = new QPushButton(tr("Apply"), this); m_clearDiskButton = new QPushButton(tr("Clear disk cache"), this); @@ -38,7 +32,7 @@ CacheSettingsWidget::CacheSettingsWidget(QWidget* parent) layout->addRow(m_enableDisk); layout->addRow(tr("RAM limit"), m_ramLimitMB); layout->addRow(tr("Disk limit"), m_diskLimitGB); - layout->addRow(tr("Cache directory"), dirLayout); + layout->addRow(tr("Cache directory"), m_cacheDirLabel); layout->addRow(QString(), m_applyButton); layout->addRow(QString(), m_clearDiskButton); setLayout(layout); @@ -51,7 +45,7 @@ CacheSettingsWidget::setSettings(const CacheSettingsData& data) m_enableDisk->setChecked(data.enableDisk); m_ramLimitMB->setValue(static_cast(data.maxRamBytes / (1024ULL * 1024ULL))); m_diskLimitGB->setValue(static_cast(data.maxDiskBytes / (1024ULL * 1024ULL * 1024ULL))); - m_cacheDirEdit->setText(QString::fromStdString(data.cacheDir)); + m_cacheDirLabel->setText(QString::fromStdString(data.cacheDir)); } CacheSettingsData @@ -62,15 +56,6 @@ CacheSettingsWidget::getSettings() const data.enableDisk = m_enableDisk->isChecked(); data.maxRamBytes = static_cast(m_ramLimitMB->value()) * 1024ULL * 1024ULL; data.maxDiskBytes = static_cast(m_diskLimitGB->value()) * 1024ULL * 1024ULL * 1024ULL; - data.cacheDir = m_cacheDirEdit->text().toStdString(); + data.cacheDir = CacheSettings().defaultSettings().cacheDir; return data; } - -void -CacheSettingsWidget::browseForCacheDir() -{ - QString dir = QFileDialog::getExistingDirectory(this, tr("Select cache directory")); - if (!dir.isEmpty()) { - m_cacheDirEdit->setText(dir); - } -} diff --git a/agave_app/CacheSettingsWidget.h b/agave_app/CacheSettingsWidget.h index a5434a1fc..2da5cb36c 100644 --- a/agave_app/CacheSettingsWidget.h +++ b/agave_app/CacheSettingsWidget.h @@ -3,7 +3,7 @@ #include "CacheSettings.h" #include -#include +#include #include #include #include @@ -21,16 +21,12 @@ class CacheSettingsWidget : public QWidget QPushButton* applyButton() const { return m_applyButton; } QPushButton* clearDiskButton() const { return m_clearDiskButton; } -private slots: - void browseForCacheDir(); - private: QCheckBox* m_enableCache = nullptr; QCheckBox* m_enableDisk = nullptr; QSpinBox* m_ramLimitMB = nullptr; QSpinBox* m_diskLimitGB = nullptr; - QLineEdit* m_cacheDirEdit = nullptr; - QPushButton* m_browseButton = nullptr; + QLabel* m_cacheDirLabel = nullptr; QPushButton* m_applyButton = nullptr; QPushButton* m_clearDiskButton = nullptr; }; From 55ccbd54804f696477f5d78b1d59ca7a1f063c80 Mon Sep 17 00:00:00 2001 From: toloudis Date: Tue, 2 Jun 2026 19:36:50 -0700 Subject: [PATCH 29/31] keep cache dir fixed for duration of app. it is not user-selectable --- agave_app/CacheSettings.cpp | 65 +++++++---------- agave_app/CacheSettings.h | 1 - agave_app/CacheSettingsWidget.cpp | 7 +- agave_app/agaveGui.cpp | 6 +- agave_app/main.cpp | 24 +++++++ renderlib/CacheConfig.h | 7 +- renderlib/CacheManager.cpp | 114 ++++++++++++++++++++++++------ renderlib/CacheManager.h | 45 ++++++++++-- test/test_cacheManager.cpp | 79 +++++++++++++++++---- 9 files changed, 256 insertions(+), 92 deletions(-) diff --git a/agave_app/CacheSettings.cpp b/agave_app/CacheSettings.cpp index 286b7c04a..7fca8dabd 100644 --- a/agave_app/CacheSettings.cpp +++ b/agave_app/CacheSettings.cpp @@ -13,36 +13,34 @@ namespace { ::CacheConfig -toRenderlibConfig(const CacheSettings& settings, const CacheSettingsData& data) +toRenderlibConfig(const CacheSettingsData& data) { - CacheSettingsData normalized = data; - normalized.cacheDir = settings.defaultSettings().cacheDir; - ::CacheConfig config; - config.enabled = normalized.enabled; - config.enableDisk = normalized.enableDisk; - config.cacheDir = normalized.cacheDir; + config.enabled = data.enabled; + config.enableDisk = data.enableDisk; std::uint64_t availableMem = SystemInfo::availableMemoryBytes(); if (availableMem > 0) { - config.maxRamBytes = std::min(normalized.maxRamBytes, availableMem); - if (config.maxRamBytes < normalized.maxRamBytes) { - LOG_WARNING << "Cache RAM limit reduced from requested " << normalized.maxRamBytes << " to " << config.maxRamBytes + config.maxRamBytes = std::min(data.maxRamBytes, availableMem); + if (config.maxRamBytes < data.maxRamBytes) { + LOG_WARNING << "Cache RAM limit reduced from requested " << data.maxRamBytes << " to " << config.maxRamBytes << " bytes to fit available memory."; } } else { - config.maxRamBytes = normalized.maxRamBytes; + config.maxRamBytes = data.maxRamBytes; } - std::uint64_t availableDisk = SystemInfo::availableDiskBytes(normalized.cacheDir); + // The cache root is owned and writability-checked by CacheManager; clamp the + // disk limit against whatever filesystem it actually lives on. + std::uint64_t availableDisk = SystemInfo::availableDiskBytes(CacheManager::instance().getCacheDirectory()); if (availableDisk > 0) { - config.maxDiskBytes = std::min(normalized.maxDiskBytes, availableDisk); - if (config.maxDiskBytes < normalized.maxDiskBytes && normalized.enableDisk) { - LOG_WARNING << "Cache disk limit reduced from requested " << normalized.maxDiskBytes << " to " - << config.maxDiskBytes << " bytes to fit available disk space."; + config.maxDiskBytes = std::min(data.maxDiskBytes, availableDisk); + if (config.maxDiskBytes < data.maxDiskBytes && data.enableDisk) { + LOG_WARNING << "Cache disk limit reduced from requested " << data.maxDiskBytes << " to " << config.maxDiskBytes + << " bytes to fit available disk space."; } } else { - config.maxDiskBytes = normalized.maxDiskBytes; + config.maxDiskBytes = data.maxDiskBytes; } if (!config.enableDisk) { @@ -59,16 +57,8 @@ CacheSettings::CacheSettings() {} CacheSettingsData CacheSettings::defaultSettings() const { - CacheSettingsData data; - QString baseDir = QStandardPaths::writableLocation(QStandardPaths::CacheLocation); - if (baseDir.isEmpty()) { - baseDir = QStandardPaths::writableLocation(QStandardPaths::AppDataLocation); - } - if (baseDir.isEmpty()) { - baseDir = QDir::currentPath(); - } - data.cacheDir = QDir(baseDir).filePath("agave-cache").toStdString(); - return data; + // Tunable defaults come from CacheSettingsData's in-class initializers. + return CacheSettingsData(); } std::string @@ -117,10 +107,6 @@ CacheSettings::load() return defaultSettings(); } - // Cache files are not user data. Always use AGAVE's platform-selected cache - // directory and ignore custom paths from older settings files. - data.cacheDir = defaultSettings().cacheDir; - return data; } @@ -146,20 +132,17 @@ CacheSettings::save(const CacheSettingsData& data) const void CacheSettings::applyToRenderlib(const CacheSettingsData& data) const { - ::CacheConfig config = toRenderlibConfig(*this, data); - if (config.enableDisk && !config.cacheDir.empty()) { - if (!CacheManager::canWriteCacheDir(config.cacheDir)) { - LOG_WARNING << "Cache disk disabled: cache directory not writable: " << config.cacheDir; - config.enableDisk = false; - config.maxDiskBytes = 0; - } - } + // The cache directory (and its writability) is settled once at startup in + // CacheManager::initialize(); if it wasn't writable the manager left its root + // unset, so a disk-enabled config here is simply honored as RAM-only. We only + // push the runtime tunables. + ::CacheConfig config = toRenderlibConfig(data); LOG_INFO << "Cache config: enabled=" << (config.enabled ? 1 : 0) << " ram_bytes=" << config.maxRamBytes << " disk_enabled=" << (config.enableDisk ? 1 : 0) << " disk_bytes=" << config.maxDiskBytes - << " cache_dir=" << config.cacheDir; + << " cache_dir=" << CacheManager::instance().getCacheDirectory(); CacheManager::instance().setConfig(config); auto applied = CacheManager::instance().getConfig(); LOG_INFO << "Cache config applied: enabled=" << (applied.enabled ? 1 : 0) << " ram_bytes=" << applied.maxRamBytes << " disk_enabled=" << (applied.enableDisk ? 1 : 0) << " disk_bytes=" << applied.maxDiskBytes - << " cache_dir=" << applied.cacheDir; + << " cache_dir=" << CacheManager::instance().getCacheDirectory(); } diff --git a/agave_app/CacheSettings.h b/agave_app/CacheSettings.h index 90b03c7b0..579cab174 100644 --- a/agave_app/CacheSettings.h +++ b/agave_app/CacheSettings.h @@ -9,7 +9,6 @@ struct CacheSettingsData bool enableDisk = true; std::uint64_t maxRamBytes = 4ULL * 1024ULL * 1024ULL * 1024ULL; std::uint64_t maxDiskBytes = 100ULL * 1024ULL * 1024ULL * 1024ULL; - std::string cacheDir; }; class CacheSettings diff --git a/agave_app/CacheSettingsWidget.cpp b/agave_app/CacheSettingsWidget.cpp index ffa532b53..89d49ed38 100644 --- a/agave_app/CacheSettingsWidget.cpp +++ b/agave_app/CacheSettingsWidget.cpp @@ -1,5 +1,7 @@ #include "CacheSettingsWidget.h" +#include "renderlib/CacheManager.h" + #include CacheSettingsWidget::CacheSettingsWidget(QWidget* parent) @@ -21,7 +23,8 @@ CacheSettingsWidget::CacheSettingsWidget(QWidget* parent) m_diskLimitGB->setSingleStep(10); m_cacheDirLabel = new QLabel(this); - m_cacheDirLabel->setText(QString::fromStdString(CacheSettings().defaultSettings().cacheDir)); + // The cache directory is fixed (registered at startup); display it read-only. + m_cacheDirLabel->setText(QString::fromStdString(CacheManager::instance().getCacheDirectory())); m_cacheDirLabel->setTextInteractionFlags(Qt::TextSelectableByMouse); m_cacheDirLabel->setWordWrap(true); @@ -45,7 +48,6 @@ CacheSettingsWidget::setSettings(const CacheSettingsData& data) m_enableDisk->setChecked(data.enableDisk); m_ramLimitMB->setValue(static_cast(data.maxRamBytes / (1024ULL * 1024ULL))); m_diskLimitGB->setValue(static_cast(data.maxDiskBytes / (1024ULL * 1024ULL * 1024ULL))); - m_cacheDirLabel->setText(QString::fromStdString(data.cacheDir)); } CacheSettingsData @@ -56,6 +58,5 @@ CacheSettingsWidget::getSettings() const data.enableDisk = m_enableDisk->isChecked(); data.maxRamBytes = static_cast(m_ramLimitMB->value()) * 1024ULL * 1024ULL; data.maxDiskBytes = static_cast(m_diskLimitGB->value()) * 1024ULL * 1024ULL * 1024ULL; - data.cacheDir = CacheSettings().defaultSettings().cacheDir; return data; } diff --git a/agave_app/agaveGui.cpp b/agave_app/agaveGui.cpp index 0b169ddc3..5d38c8419 100644 --- a/agave_app/agaveGui.cpp +++ b/agave_app/agaveGui.cpp @@ -109,10 +109,8 @@ agaveGui::agaveGui(QWidget* parent) m_cacheSettings.applyToRenderlib(data); }); connect(m_cacheSettingsDockWidget->widget()->clearDiskButton(), &QPushButton::clicked, this, [this]() { - // Show the path that will actually be cleared (the applied config), which - // may differ from the widget's current text if the user edited the path - // without clicking Apply. - QString cacheDir = QString::fromStdString(CacheManager::instance().getConfig().cacheDir); + // Show the directory that will actually be cleared. + QString cacheDir = QString::fromStdString(CacheManager::instance().getCacheDirectory()); QMessageBox::StandardButton reply = QMessageBox::question(this, tr("Clear disk cache"), diff --git a/agave_app/main.cpp b/agave_app/main.cpp index 5a4d4f674..474b178c1 100644 --- a/agave_app/main.cpp +++ b/agave_app/main.cpp @@ -1,6 +1,7 @@ #include "agaveGui.h" #include "mainwindow.h" +#include "renderlib/CacheManager.h" #include "renderlib/Logging.h" #include "renderlib/io/FileReader.h" #include "renderlib/renderlib.h" @@ -10,6 +11,7 @@ #include #include #include +#include #include #include #include @@ -134,6 +136,22 @@ getUrlToOpen(const QUrl& agaveUrl) return ""; } +// Platform-appropriate cache directory for AGAVE's volume cache. Cache files +// are not user data, so the location is fixed (not user-configurable) and +// resolved once at startup, the same way the assets path is. +std::string +getCacheDirectory() +{ + QString baseDir = QStandardPaths::writableLocation(QStandardPaths::CacheLocation); + if (baseDir.isEmpty()) { + baseDir = QStandardPaths::writableLocation(QStandardPaths::AppDataLocation); + } + if (baseDir.isEmpty()) { + baseDir = QDir::currentPath(); + } + return QDir(baseDir).filePath("agave-cache").toStdString(); +} + class AgaveApplication : public QApplication { public: @@ -254,6 +272,12 @@ main(int argc, char* argv[]) return 0; } + // Register the cache directory once for the lifetime of the process, after + // renderlib has successfully initialized. Resolving the platform path needs + // the Qt application name (set above). Applies to both server and GUI mode; + // caching stays inert until a CacheConfig enables it. + CacheManager::initialize(getCacheDirectory()); + if (isServer) { QString configPath = parser.value(serverConfigOption); ServerParams p = readConfig(configPath); diff --git a/renderlib/CacheConfig.h b/renderlib/CacheConfig.h index b777cfa18..e3f19a5e0 100644 --- a/renderlib/CacheConfig.h +++ b/renderlib/CacheConfig.h @@ -1,13 +1,16 @@ #pragma once #include -#include +// Runtime-tunable cache settings. The disk cache *location* is intentionally +// not part of this struct: it is resolved once at app startup and registered +// via CacheManager::initialize(). Keeping it out of CacheConfig means +// the per-apply settings (enable/limits) can change freely without ever +// re-pointing the cache root. struct CacheConfig { bool enabled = false; bool enableDisk = false; std::uint64_t maxRamBytes = 0; std::uint64_t maxDiskBytes = 0; - std::string cacheDir; }; diff --git a/renderlib/CacheManager.cpp b/renderlib/CacheManager.cpp index 321f9ea1e..53ab9d32d 100644 --- a/renderlib/CacheManager.cpp +++ b/renderlib/CacheManager.cpp @@ -19,6 +19,7 @@ #include #include #include +#include namespace { @@ -210,10 +211,75 @@ CacheManager::canWriteCacheDir(const std::string& path) return !ec; } +void +CacheManager::initialize(const std::string& cacheDir) +{ + CacheManager& mgr = instance(); + { + std::scoped_lock lock(mgr.m_mutex); + if (mgr.m_initialized) { + throw std::logic_error("CacheManager::initialize() called more than once; the cache directory is fixed for the " + "lifetime of the process."); + } + mgr.m_initialized = true; + } + + // Probe writability once, here, rather than on every config-apply: the cache + // root is fixed for the process lifetime. If the directory can't be created + // or written, leave the root unset so the disk tier stays inert regardless of + // what any later CacheConfig requests. + std::string root = cacheDir; + if (!root.empty() && !canWriteCacheDir(root)) { + LOG_WARNING << "Disk cache disabled: cache directory not writable: " << root; + root.clear(); + } + mgr.applyCacheDirectory(root); +} + +void +CacheManager::applyCacheDirectory(const std::string& dir) +{ + CacheConfig configCopy; + std::string cacheDirCopy; + bool rebuildDiskIndex = false; + { + std::scoped_lock lock(m_mutex); + if (m_cacheDir == dir) { + return; + } + m_cacheDir = dir; + // The previous root's bookkeeping no longer applies. Drop it; it will be + // rebuilt below (or by the next setConfig) for the new root. + m_diskEntries.clear(); + m_currentDiskBytes = 0; + m_diskIndexRoot.clear(); + + if (m_config.enabled && m_config.enableDisk && !m_cacheDir.empty()) { + m_diskIndexRoot = m_cacheDir; + rebuildDiskIndex = true; + configCopy = m_config; + cacheDirCopy = m_cacheDir; + } + } + + if (rebuildDiskIndex) { + loadDiskIndex(configCopy, cacheDirCopy); + evictDiskIfNeeded(configCopy, 0); + } +} + +std::string +CacheManager::getCacheDirectory() const +{ + std::scoped_lock lock(m_mutex); + return m_cacheDir; +} + void CacheManager::setConfig(const CacheConfig& config) { CacheConfig configCopy; + std::string cacheDirCopy; bool rebuildDiskIndex = false; { std::scoped_lock lock(m_mutex); @@ -228,21 +294,22 @@ CacheManager::setConfig(const CacheConfig& config) return; } - if (!m_config.enableDisk || m_config.cacheDir.empty()) { + if (!m_config.enableDisk || m_cacheDir.empty()) { m_diskEntries.clear(); m_currentDiskBytes = 0; m_diskIndexRoot.clear(); - } else if (m_diskIndexRoot != m_config.cacheDir) { + } else if (m_diskIndexRoot != m_cacheDir) { m_diskEntries.clear(); m_currentDiskBytes = 0; - m_diskIndexRoot = m_config.cacheDir; + m_diskIndexRoot = m_cacheDir; rebuildDiskIndex = true; configCopy = m_config; + cacheDirCopy = m_cacheDir; } } if (rebuildDiskIndex) { - loadDiskIndex(configCopy); + loadDiskIndex(configCopy, cacheDirCopy); evictDiskIfNeeded(configCopy, 0); } @@ -261,10 +328,12 @@ std::shared_ptr CacheManager::findImage(const LoadSpec& loadSpec) { CacheConfig configCopy; + std::string cacheDirCopy; CacheKey key = makeKey(loadSpec); { std::scoped_lock lock(m_mutex); configCopy = m_config; + cacheDirCopy = m_cacheDir; if (m_config.enabled && m_config.maxRamBytes > 0) { auto it = m_entries.find(key); if (it != m_entries.end()) { @@ -292,8 +361,8 @@ CacheManager::findImage(const LoadSpec& loadSpec) } } - if (configCopy.enabled && configCopy.enableDisk && configCopy.maxDiskBytes > 0) { - auto diskImage = loadFromDisk(key, configCopy); + if (configCopy.enabled && configCopy.enableDisk && configCopy.maxDiskBytes > 0 && !cacheDirCopy.empty()) { + auto diskImage = loadFromDisk(key, configCopy, cacheDirCopy); if (diskImage) { { std::scoped_lock lock(m_mutex); @@ -324,15 +393,17 @@ CacheManager::storeImage(const LoadSpec& loadSpec, const std::shared_ptr 0) { - storeToDisk(key, image, configCopy); + if (configCopy.enabled && configCopy.enableDisk && configCopy.maxDiskBytes > 0 && !cacheDirCopy.empty()) { + storeToDisk(key, image, configCopy, cacheDirCopy); } storeImageInMemory(key, image); @@ -354,7 +425,7 @@ CacheManager::clearDiskCache() std::vector knownEntryPaths; { std::scoped_lock lock(m_mutex); - cacheDir = m_config.cacheDir; + cacheDir = m_cacheDir; if (!isAgaveCacheDir(cacheDir)) { LOG_WARNING << "Refusing to clear disk cache: directory missing AGAVE cache marker file (" << kCacheMarkerFilename @@ -560,13 +631,13 @@ CacheManager::storeImageInMemory(const CacheKey& key, const std::shared_ptr -CacheManager::loadFromDisk(const CacheKey& key, const CacheConfig& config) +CacheManager::loadFromDisk(const CacheKey& key, const CacheConfig& config, const std::string& cacheDir) { - if (!config.enableDisk || config.cacheDir.empty()) { + if (!config.enableDisk || cacheDir.empty()) { return nullptr; } - std::filesystem::path entryPath = std::filesystem::path(config.cacheDir) / diskCacheId(key); + std::filesystem::path entryPath = std::filesystem::path(cacheDir) / diskCacheId(key); std::filesystem::path metaPath = entryPath / "meta.json"; std::filesystem::path dataPath = entryPath / "data.zarr"; if (!std::filesystem::exists(metaPath) || !std::filesystem::exists(dataPath)) { @@ -661,9 +732,12 @@ CacheManager::loadFromDisk(const CacheKey& key, const CacheConfig& config) } void -CacheManager::storeToDisk(const CacheKey& key, const std::shared_ptr& image, const CacheConfig& config) +CacheManager::storeToDisk(const CacheKey& key, + const std::shared_ptr& image, + const CacheConfig& config, + const std::string& cacheDir) { - if (!image || !config.enableDisk || config.cacheDir.empty()) { + if (!image || !config.enableDisk || cacheDir.empty()) { return; } @@ -683,9 +757,9 @@ CacheManager::storeToDisk(const CacheKey& key, const std::shared_ptr& // disk and don't waste a large write that would have to be undone. evictDiskIfNeeded(config, bytes); - writeCacheMarker(config.cacheDir); + writeCacheMarker(cacheDir); - std::filesystem::path entryPath = std::filesystem::path(config.cacheDir) / diskCacheId(key); + std::filesystem::path entryPath = std::filesystem::path(cacheDir) / diskCacheId(key); std::filesystem::path dataPath = entryPath / "data.zarr"; std::filesystem::path metaPath = entryPath / "meta.json"; std::error_code ec; @@ -778,19 +852,19 @@ CacheManager::storeToDisk(const CacheKey& key, const std::shared_ptr& } void -CacheManager::loadDiskIndex(const CacheConfig& config) +CacheManager::loadDiskIndex(const CacheConfig& config, const std::string& cacheDir) { - if (config.cacheDir.empty() || !config.enableDisk) { + if (cacheDir.empty() || !config.enableDisk) { return; } - std::filesystem::path root(config.cacheDir); + std::filesystem::path root(cacheDir); std::error_code ec; std::filesystem::create_directories(root, ec); if (!std::filesystem::exists(root)) { return; } - writeCacheMarker(config.cacheDir); + writeCacheMarker(cacheDir); std::unordered_map entries; std::uint64_t totalBytes = 0; diff --git a/renderlib/CacheManager.h b/renderlib/CacheManager.h index ccc380469..c78fa2184 100644 --- a/renderlib/CacheManager.h +++ b/renderlib/CacheManager.h @@ -46,6 +46,12 @@ struct CacheKeyHash class CacheManager { + // Test-only access to internals. Lets the test suite re-point the cache + // directory between cases (the singleton is shared across all tests) without + // going through the once-only initialize() guard. Declared here so the + // production API exposes no test hooks. + friend struct CacheManagerTestAccess; + public: struct CacheStats { @@ -57,10 +63,15 @@ class CacheManager static CacheManager& instance(); - // Verify that `path` is (or can be made) a writable directory. Creates the - // directory if it does not exist, then probes it by writing and deleting a - // small marker file. Returns false on any failure. - static bool canWriteCacheDir(const std::string& path); + // Register the on-disk cache root. Must be called exactly once, at app + // startup, before the cache is used. The platform-appropriate path is + // resolved by the GUI layer (renderlib has no Qt and so cannot resolve + // QStandardPaths itself) and injected here. The cache directory is fixed for + // the lifetime of the process and is never derived from per-apply + // CacheConfig. Calling initialize() more than once is a programming error + // and throws std::logic_error. + static void initialize(const std::string& cacheDir); + std::string getCacheDirectory() const; void setConfig(const CacheConfig& config); CacheConfig getConfig() const; @@ -79,6 +90,17 @@ class CacheManager private: CacheManager() = default; + // Verify that `path` is (or can be made) a writable directory. Creates the + // directory if it does not exist, then probes it by writing and deleting a + // small marker file. Returns false on any failure. Probed once, at + // initialize() time, since the cache root is fixed for the process lifetime. + static bool canWriteCacheDir(const std::string& path); + + // Records the cache root and (re)builds the disk index for it when the disk + // tier is active. The single production entry point is initialize(); this + // worker is also reused by the test seam to re-point across test cases. + void applyCacheDirectory(const std::string& dir); + CacheKey makeKey(const LoadSpec& loadSpec) const; std::string keyToString(const CacheKey& key) const; std::string diskCacheId(const CacheKey& key) const; @@ -88,9 +110,12 @@ class CacheManager void evictIfNeededLocked(std::uint64_t incomingBytes); void storeImageInMemory(const CacheKey& key, const std::shared_ptr& image); - std::shared_ptr loadFromDisk(const CacheKey& key, const CacheConfig& config); - void storeToDisk(const CacheKey& key, const std::shared_ptr& image, const CacheConfig& config); - void loadDiskIndex(const CacheConfig& config); + std::shared_ptr loadFromDisk(const CacheKey& key, const CacheConfig& config, const std::string& cacheDir); + void storeToDisk(const CacheKey& key, + const std::shared_ptr& image, + const CacheConfig& config, + const std::string& cacheDir); + void loadDiskIndex(const CacheConfig& config, const std::string& cacheDir); void evictDiskIfNeeded(const CacheConfig& config, std::uint64_t incomingBytes); std::uint64_t directorySizeBytes(const std::string& path) const; // Writes a marker file to a directory we manage as our own disk cache root. @@ -101,6 +126,12 @@ class CacheManager mutable std::mutex m_mutex; CacheConfig m_config; + // Set true by initialize(); guards against a second initialize() call. + bool m_initialized = false; + // The disk cache root, registered once via initialize(). Distinct from + // m_diskIndexRoot, which tracks the root the in-memory index was last built + // against (used to decide when a rebuild is needed). + std::string m_cacheDir; std::uint64_t m_currentRamBytes = 0; std::list m_lruKeys; diff --git a/test/test_cacheManager.cpp b/test/test_cacheManager.cpp index 2e53688fc..c00e0a5f2 100644 --- a/test/test_cacheManager.cpp +++ b/test/test_cacheManager.cpp @@ -15,6 +15,16 @@ #include #include +// Grants the test suite access to CacheManager internals (declared a friend in +// CacheManager.h). The production API is a one-shot CacheManager::initialize(); +// tests share a single process-wide singleton and need to re-point the cache +// directory for per-case isolation, so they call the underlying worker +// directly through this seam rather than initialize(). +struct CacheManagerTestAccess +{ + static void setCacheDirectory(const std::string& dir) { CacheManager::instance().applyCacheDirectory(dir); } +}; + namespace { // Bytes per pixel that CacheManager uses when estimating an image's RAM cost. @@ -110,16 +120,19 @@ class TempCacheDir std::filesystem::path m_path; }; -CacheConfig -diskConfig(const std::string& cacheDir, std::uint64_t maxRamBytes, std::uint64_t maxDiskBytes) +// Register the disk cache root and apply a disk-enabled config, mirroring the +// real startup flow (setCacheDirectory once, then setConfig). The cache +// directory is no longer part of CacheConfig. +void +configureDiskCache(const std::string& cacheDir, std::uint64_t maxRamBytes, std::uint64_t maxDiskBytes) { + CacheManagerTestAccess::setCacheDirectory(cacheDir); CacheConfig cfg; cfg.enabled = true; cfg.enableDisk = true; cfg.maxRamBytes = maxRamBytes; cfg.maxDiskBytes = maxDiskBytes; - cfg.cacheDir = cacheDir; - return cfg; + CacheManager::instance().setConfig(cfg); } // Count subdirectories under `dir` (used to verify entry-dir counts after @@ -372,7 +385,7 @@ TEST_CASE("CacheManager disk tier round-trips images and respects the disk cap", { resetCache(); TempCacheDir tmp; - CacheManager::instance().setConfig(diskConfig(tmp.str(), oneImage * 4, 1ULL * 1024 * 1024)); + configureDiskCache(tmp.str(), oneImage * 4, 1ULL * 1024 * 1024); REQUIRE(std::filesystem::exists(tmp.path() / ".agave-cache-dir")); } @@ -381,7 +394,7 @@ TEST_CASE("CacheManager disk tier round-trips images and respects the disk cap", { resetCache(); TempCacheDir tmp; - CacheManager::instance().setConfig(diskConfig(tmp.str(), oneImage * 4, 1ULL * 1024 * 1024)); + configureDiskCache(tmp.str(), oneImage * 4, 1ULL * 1024 * 1024); auto img = makeImageWithPattern(4, 4, 4, 1); auto spec = makeSpec("disk_roundtrip"); @@ -414,7 +427,7 @@ TEST_CASE("CacheManager disk tier round-trips images and respects the disk cap", resetCache(); TempCacheDir tmp; // Cap is below a single image, so storeToDisk must refuse outright. - CacheManager::instance().setConfig(diskConfig(tmp.str(), oneImage * 4, oneImage / 2)); + configureDiskCache(tmp.str(), oneImage * 4, oneImage / 2); auto spec = makeSpec("too_big_for_disk"); CacheManager::instance().storeImage(spec, makeImage(4, 4, 4, 1)); @@ -433,7 +446,7 @@ TEST_CASE("CacheManager disk tier round-trips images and respects the disk cap", resetCache(); TempCacheDir tmp; // Cap large enough for two entries' raw byte estimate but not three. - CacheManager::instance().setConfig(diskConfig(tmp.str(), oneImage * 4, oneImage * 2)); + configureDiskCache(tmp.str(), oneImage * 4, oneImage * 2); CacheManager::instance().storeImage(makeSpec("disk_a"), makeImage(4, 4, 4, 1)); // Sleep briefly so lastAccess timestamps are distinct on fast disks. @@ -458,7 +471,7 @@ TEST_CASE("CacheManager disk tier round-trips images and respects the disk cap", { resetCache(); TempCacheDir tmp; - CacheManager::instance().setConfig(diskConfig(tmp.str(), oneImage * 4, 1ULL * 1024 * 1024)); + configureDiskCache(tmp.str(), oneImage * 4, 1ULL * 1024 * 1024); CacheManager::instance().storeImage(makeSpec("clear_a"), makeImage(4, 4, 4, 1)); CacheManager::instance().storeImage(makeSpec("clear_b"), makeImage(4, 4, 4, 1)); @@ -474,7 +487,7 @@ TEST_CASE("CacheManager disk tier round-trips images and respects the disk cap", { resetCache(); TempCacheDir tmp; - CacheManager::instance().setConfig(diskConfig(tmp.str(), oneImage * 4, 1ULL * 1024 * 1024)); + configureDiskCache(tmp.str(), oneImage * 4, 1ULL * 1024 * 1024); CacheManager::instance().storeImage(makeSpec("guarded_a"), makeImage(4, 4, 4, 1)); int subdirsBefore = countSubdirs(tmp.path()); @@ -491,11 +504,11 @@ TEST_CASE("CacheManager disk tier round-trips images and respects the disk cap", REQUIRE(countSubdirs(tmp.path()) == subdirsBefore); } - SECTION("Re-pointing setConfig at a previously-used cache dir rebuilds the disk index") + SECTION("Re-pointing setCacheDirectory at a previously-used cache dir rebuilds the disk index") { resetCache(); TempCacheDir tmp; - CacheManager::instance().setConfig(diskConfig(tmp.str(), oneImage * 4, 1ULL * 1024 * 1024)); + configureDiskCache(tmp.str(), oneImage * 4, 1ULL * 1024 * 1024); CacheManager::instance().storeImage(makeSpec("persistent"), makeImage(4, 4, 4, 1)); @@ -503,10 +516,10 @@ TEST_CASE("CacheManager disk tier round-trips images and respects the disk cap", // (forcing the manager to drop its in-memory disk bookkeeping), then // point it back at the original dir. TempCacheDir other; - CacheManager::instance().setConfig(diskConfig(other.str(), oneImage * 4, 1ULL * 1024 * 1024)); + configureDiskCache(other.str(), oneImage * 4, 1ULL * 1024 * 1024); CacheManager::instance().clearMemoryCache(); - CacheManager::instance().setConfig(diskConfig(tmp.str(), oneImage * 4, 1ULL * 1024 * 1024)); + configureDiskCache(tmp.str(), oneImage * 4, 1ULL * 1024 * 1024); CacheManager::instance().resetStats(); auto found = CacheManager::instance().findImage(makeSpec("persistent")); @@ -515,6 +528,44 @@ TEST_CASE("CacheManager disk tier round-trips images and respects the disk cap", } } +TEST_CASE("CacheManager cache directory is registered separately from config", "[cache][disk]") +{ + const std::uint64_t oneImage = imageBytes(4, 4, 4, 1); + + SECTION("getCacheDirectory reflects the registered path") + { + resetCache(); + TempCacheDir tmp; + CacheManagerTestAccess::setCacheDirectory(tmp.str()); + REQUIRE(CacheManager::instance().getCacheDirectory() == tmp.str()); + } + + SECTION("With no cache directory registered, the disk tier is inert") + { + resetCache(); + // Clear any directory a previous test may have left registered on the + // singleton; the disk tier should then behave as RAM-only. + CacheManagerTestAccess::setCacheDirectory(""); + + CacheConfig cfg; + cfg.enabled = true; + cfg.enableDisk = true; + cfg.maxRamBytes = oneImage * 4; + cfg.maxDiskBytes = 1ULL * 1024 * 1024; + CacheManager::instance().setConfig(cfg); + + auto spec = makeSpec("no_dir"); + CacheManager::instance().storeImage(spec, makeImage(4, 4, 4, 1)); + + // RAM still serves it within the session... + REQUIRE(CacheManager::instance().findImage(spec) != nullptr); + + // ...but nothing was persisted to disk, so once RAM is dropped it misses. + CacheManager::instance().clearMemoryCache(); + REQUIRE(CacheManager::instance().findImage(spec) == nullptr); + } +} + TEST_CASE("CacheManager invalidates entries when the source file mtime changes", "[cache][mtime]") { resetCache(); From f67632b7c31794f7828af4611e315a7204a5a154 Mon Sep 17 00:00:00 2001 From: toloudis Date: Tue, 2 Jun 2026 20:00:27 -0700 Subject: [PATCH 30/31] allow cachemanager to be constructed --- agave_app/CacheSettings.cpp | 4 +- renderlib/CacheManager.cpp | 73 +++---- renderlib/CacheManager.h | 45 ++-- test/test_cacheManager.cpp | 418 +++++++++++++++--------------------- 4 files changed, 222 insertions(+), 318 deletions(-) diff --git a/agave_app/CacheSettings.cpp b/agave_app/CacheSettings.cpp index 7fca8dabd..29a6a1b39 100644 --- a/agave_app/CacheSettings.cpp +++ b/agave_app/CacheSettings.cpp @@ -52,13 +52,13 @@ toRenderlibConfig(const CacheSettingsData& data) } // namespace -CacheSettings::CacheSettings() {} +CacheSettings::CacheSettings() = default; CacheSettingsData CacheSettings::defaultSettings() const { // Tunable defaults come from CacheSettingsData's in-class initializers. - return CacheSettingsData(); + return {}; } std::string diff --git a/renderlib/CacheManager.cpp b/renderlib/CacheManager.cpp index 53ab9d32d..7bdd2eca2 100644 --- a/renderlib/CacheManager.cpp +++ b/renderlib/CacheManager.cpp @@ -18,6 +18,7 @@ #include #include #include +#include #include #include @@ -168,11 +169,20 @@ CacheKeyHash::operator()(const CacheKey& key) const return seed; } -CacheManager& -CacheManager::instance() +namespace { +// Storage for the process-wide singleton. A unique_ptr (rather than a Meyers +// static) lets initialize() inject the cache directory at construction time. +std::unique_ptr& +singletonSlot() +{ + static std::unique_ptr slot; + return slot; +} +} // namespace + +CacheManager::CacheManager(std::string cacheDir) + : m_cacheDir(std::move(cacheDir)) { - static CacheManager manager; - return manager; } bool @@ -214,58 +224,33 @@ CacheManager::canWriteCacheDir(const std::string& path) void CacheManager::initialize(const std::string& cacheDir) { - CacheManager& mgr = instance(); - { - std::scoped_lock lock(mgr.m_mutex); - if (mgr.m_initialized) { - throw std::logic_error("CacheManager::initialize() called more than once; the cache directory is fixed for the " - "lifetime of the process."); - } - mgr.m_initialized = true; + if (singletonSlot()) { + throw std::logic_error("CacheManager::initialize() called more than once; the cache directory is fixed for the " + "lifetime of the process."); } // Probe writability once, here, rather than on every config-apply: the cache - // root is fixed for the process lifetime. If the directory can't be created - // or written, leave the root unset so the disk tier stays inert regardless of - // what any later CacheConfig requests. + // root is fixed for the lifetime of the process. If the directory can't be + // created or written, leave the root unset so the disk tier stays inert + // regardless of what any later CacheConfig requests. std::string root = cacheDir; if (!root.empty() && !canWriteCacheDir(root)) { LOG_WARNING << "Disk cache disabled: cache directory not writable: " << root; root.clear(); } - mgr.applyCacheDirectory(root); + singletonSlot() = std::make_unique(root); } -void -CacheManager::applyCacheDirectory(const std::string& dir) +CacheManager& +CacheManager::instance() { - CacheConfig configCopy; - std::string cacheDirCopy; - bool rebuildDiskIndex = false; - { - std::scoped_lock lock(m_mutex); - if (m_cacheDir == dir) { - return; - } - m_cacheDir = dir; - // The previous root's bookkeeping no longer applies. Drop it; it will be - // rebuilt below (or by the next setConfig) for the new root. - m_diskEntries.clear(); - m_currentDiskBytes = 0; - m_diskIndexRoot.clear(); - - if (m_config.enabled && m_config.enableDisk && !m_cacheDir.empty()) { - m_diskIndexRoot = m_cacheDir; - rebuildDiskIndex = true; - configCopy = m_config; - cacheDirCopy = m_cacheDir; - } - } - - if (rebuildDiskIndex) { - loadDiskIndex(configCopy, cacheDirCopy); - evictDiskIfNeeded(configCopy, 0); + auto& slot = singletonSlot(); + if (!slot) { + // initialize() was never called (e.g. a context that does no caching); + // fall back to a RAM-only, disk-inert manager. + slot = std::make_unique(std::string{}); } + return *slot; } std::string diff --git a/renderlib/CacheManager.h b/renderlib/CacheManager.h index c78fa2184..6ed78a8db 100644 --- a/renderlib/CacheManager.h +++ b/renderlib/CacheManager.h @@ -46,12 +46,6 @@ struct CacheKeyHash class CacheManager { - // Test-only access to internals. Lets the test suite re-point the cache - // directory between cases (the singleton is shared across all tests) without - // going through the once-only initialize() guard. Declared here so the - // production API exposes no test hooks. - friend struct CacheManagerTestAccess; - public: struct CacheStats { @@ -61,16 +55,22 @@ class CacheManager std::uint64_t diskWrites = 0; }; - static CacheManager& instance(); - - // Register the on-disk cache root. Must be called exactly once, at app - // startup, before the cache is used. The platform-appropriate path is + // Construct a cache rooted at `cacheDir`. An empty `cacheDir` disables the + // disk tier (the cache is RAM-only). The directory is fixed for the lifetime + // of the instance and is never derived from per-apply CacheConfig. Production + // code uses the process-wide singleton (initialize() + instance()); this + // constructor is public so tests can create isolated, throwaway caches with + // their own directories. + explicit CacheManager(std::string cacheDir = {}); + + // The process-wide singleton. initialize() creates it rooted at `cacheDir` + // and must be called exactly once, at app startup, before the cache is used; + // a second call throws std::logic_error. The platform-appropriate path is // resolved by the GUI layer (renderlib has no Qt and so cannot resolve - // QStandardPaths itself) and injected here. The cache directory is fixed for - // the lifetime of the process and is never derived from per-apply - // CacheConfig. Calling initialize() more than once is a programming error - // and throws std::logic_error. + // QStandardPaths itself) and injected here. If initialize() is never called, + // instance() lazily yields a RAM-only (disk-inert) manager. static void initialize(const std::string& cacheDir); + static CacheManager& instance(); std::string getCacheDirectory() const; void setConfig(const CacheConfig& config); @@ -88,19 +88,12 @@ class CacheManager void resetStats(); private: - CacheManager() = default; - // Verify that `path` is (or can be made) a writable directory. Creates the // directory if it does not exist, then probes it by writing and deleting a // small marker file. Returns false on any failure. Probed once, at // initialize() time, since the cache root is fixed for the process lifetime. static bool canWriteCacheDir(const std::string& path); - // Records the cache root and (re)builds the disk index for it when the disk - // tier is active. The single production entry point is initialize(); this - // worker is also reused by the test seam to re-point across test cases. - void applyCacheDirectory(const std::string& dir); - CacheKey makeKey(const LoadSpec& loadSpec) const; std::string keyToString(const CacheKey& key) const; std::string diskCacheId(const CacheKey& key) const; @@ -126,12 +119,10 @@ class CacheManager mutable std::mutex m_mutex; CacheConfig m_config; - // Set true by initialize(); guards against a second initialize() call. - bool m_initialized = false; - // The disk cache root, registered once via initialize(). Distinct from - // m_diskIndexRoot, which tracks the root the in-memory index was last built - // against (used to decide when a rebuild is needed). - std::string m_cacheDir; + // The disk cache root, fixed at construction. Distinct from m_diskIndexRoot, + // which tracks the root the in-memory index was last built against (used to + // decide when a rebuild is needed). + const std::string m_cacheDir; std::uint64_t m_currentRamBytes = 0; std::list m_lruKeys; diff --git a/test/test_cacheManager.cpp b/test/test_cacheManager.cpp index c00e0a5f2..ff8c550c5 100644 --- a/test/test_cacheManager.cpp +++ b/test/test_cacheManager.cpp @@ -15,15 +15,10 @@ #include #include -// Grants the test suite access to CacheManager internals (declared a friend in -// CacheManager.h). The production API is a one-shot CacheManager::initialize(); -// tests share a single process-wide singleton and need to re-point the cache -// directory for per-case isolation, so they call the underlying worker -// directly through this seam rather than initialize(). -struct CacheManagerTestAccess -{ - static void setCacheDirectory(const std::string& dir) { CacheManager::instance().applyCacheDirectory(dir); } -}; +// These tests construct their own CacheManager instances (each with its own +// cache directory) rather than touching the process-wide singleton. That keeps +// every case fully isolated and means we never need to reach into CacheManager +// internals or reset shared state. namespace { @@ -57,14 +52,6 @@ imageBytes(uint32_t x, uint32_t y, uint32_t z, uint32_t c) return static_cast(x) * y * z * c * kBytesPerPixel; } -// Reset the singleton CacheManager to a known state for each test. -void -resetCache() -{ - CacheManager::instance().clearMemoryCache(); - CacheManager::instance().resetStats(); -} - CacheConfig ramOnlyConfig(std::uint64_t maxRamBytes) { @@ -76,6 +63,17 @@ ramOnlyConfig(std::uint64_t maxRamBytes) return cfg; } +CacheConfig +diskConfig(std::uint64_t maxRamBytes, std::uint64_t maxDiskBytes) +{ + CacheConfig cfg; + cfg.enabled = true; + cfg.enableDisk = true; + cfg.maxRamBytes = maxRamBytes; + cfg.maxDiskBytes = maxDiskBytes; + return cfg; +} + // An image whose pixel bytes match a known pattern (data[i] = i & 0xFF), used // to verify the disk round-trip didn't corrupt the data. std::shared_ptr @@ -120,21 +118,6 @@ class TempCacheDir std::filesystem::path m_path; }; -// Register the disk cache root and apply a disk-enabled config, mirroring the -// real startup flow (setCacheDirectory once, then setConfig). The cache -// directory is no longer part of CacheConfig. -void -configureDiskCache(const std::string& cacheDir, std::uint64_t maxRamBytes, std::uint64_t maxDiskBytes) -{ - CacheManagerTestAccess::setCacheDirectory(cacheDir); - CacheConfig cfg; - cfg.enabled = true; - cfg.enableDisk = true; - cfg.maxRamBytes = maxRamBytes; - cfg.maxDiskBytes = maxDiskBytes; - CacheManager::instance().setConfig(cfg); -} - // Count subdirectories under `dir` (used to verify entry-dir counts after // store/evict/clear operations). int @@ -158,154 +141,148 @@ TEST_CASE("CacheManager respects RAM limit and evicts LRU entries", "[cache]") // Each 4x4x4x1 image is 4*4*4*1*2 = 128 bytes. const std::uint64_t oneImage = imageBytes(4, 4, 4, 1); + // Fresh, RAM-only cache for each section (Catch2 re-runs the body per leaf). + CacheManager cache; + SECTION("Store and retrieve a single image (RAM hit)") { - resetCache(); - CacheManager::instance().setConfig(ramOnlyConfig(oneImage * 4)); + cache.setConfig(ramOnlyConfig(oneImage * 4)); auto img = makeImage(4, 4, 4, 1); auto spec = makeSpec("a"); - CacheManager::instance().storeImage(spec, img); - auto retrieved = CacheManager::instance().findImage(spec); + cache.storeImage(spec, img); + auto retrieved = cache.findImage(spec); REQUIRE(retrieved != nullptr); REQUIRE(retrieved.get() == img.get()); - auto stats = CacheManager::instance().getStats(); + auto stats = cache.getStats(); REQUIRE(stats.ramHits == 1); REQUIRE(stats.misses == 0); } SECTION("Miss returns null and increments miss counter") { - resetCache(); - CacheManager::instance().setConfig(ramOnlyConfig(oneImage * 4)); + cache.setConfig(ramOnlyConfig(oneImage * 4)); - auto retrieved = CacheManager::instance().findImage(makeSpec("missing")); + auto retrieved = cache.findImage(makeSpec("missing")); REQUIRE(retrieved == nullptr); - REQUIRE(CacheManager::instance().getStats().misses == 1); + REQUIRE(cache.getStats().misses == 1); } SECTION("Cache stays under maxRamBytes when limit is reached") { - resetCache(); // Limit is exactly 2 images. Storing a 3rd must evict. - CacheManager::instance().setConfig(ramOnlyConfig(oneImage * 2)); + cache.setConfig(ramOnlyConfig(oneImage * 2)); - CacheManager::instance().storeImage(makeSpec("a"), makeImage(4, 4, 4, 1)); - CacheManager::instance().storeImage(makeSpec("b"), makeImage(4, 4, 4, 1)); - CacheManager::instance().storeImage(makeSpec("c"), makeImage(4, 4, 4, 1)); + cache.storeImage(makeSpec("a"), makeImage(4, 4, 4, 1)); + cache.storeImage(makeSpec("b"), makeImage(4, 4, 4, 1)); + cache.storeImage(makeSpec("c"), makeImage(4, 4, 4, 1)); // "a" was the least recently used and must be evicted. - REQUIRE(CacheManager::instance().findImage(makeSpec("a")) == nullptr); - REQUIRE(CacheManager::instance().findImage(makeSpec("b")) != nullptr); - REQUIRE(CacheManager::instance().findImage(makeSpec("c")) != nullptr); + REQUIRE(cache.findImage(makeSpec("a")) == nullptr); + REQUIRE(cache.findImage(makeSpec("b")) != nullptr); + REQUIRE(cache.findImage(makeSpec("c")) != nullptr); } SECTION("LRU ordering is updated on access (touched entry survives eviction)") { - resetCache(); - CacheManager::instance().setConfig(ramOnlyConfig(oneImage * 2)); + cache.setConfig(ramOnlyConfig(oneImage * 2)); - CacheManager::instance().storeImage(makeSpec("a"), makeImage(4, 4, 4, 1)); - CacheManager::instance().storeImage(makeSpec("b"), makeImage(4, 4, 4, 1)); + cache.storeImage(makeSpec("a"), makeImage(4, 4, 4, 1)); + cache.storeImage(makeSpec("b"), makeImage(4, 4, 4, 1)); // Touch "a" so that "b" becomes least recently used. - REQUIRE(CacheManager::instance().findImage(makeSpec("a")) != nullptr); + REQUIRE(cache.findImage(makeSpec("a")) != nullptr); - CacheManager::instance().storeImage(makeSpec("c"), makeImage(4, 4, 4, 1)); + cache.storeImage(makeSpec("c"), makeImage(4, 4, 4, 1)); - REQUIRE(CacheManager::instance().findImage(makeSpec("a")) != nullptr); - REQUIRE(CacheManager::instance().findImage(makeSpec("b")) == nullptr); - REQUIRE(CacheManager::instance().findImage(makeSpec("c")) != nullptr); + REQUIRE(cache.findImage(makeSpec("a")) != nullptr); + REQUIRE(cache.findImage(makeSpec("b")) == nullptr); + REQUIRE(cache.findImage(makeSpec("c")) != nullptr); } SECTION("Storing many images keeps total RAM usage bounded") { - resetCache(); const std::uint64_t cap = oneImage * 3; - CacheManager::instance().setConfig(ramOnlyConfig(cap)); + cache.setConfig(ramOnlyConfig(cap)); for (int i = 0; i < 20; ++i) { - CacheManager::instance().storeImage(makeSpec("file_" + std::to_string(i)), makeImage(4, 4, 4, 1)); + cache.storeImage(makeSpec("file_" + std::to_string(i)), makeImage(4, 4, 4, 1)); } // Count how many of the 20 inserts are still resident. With a cap of 3 // images, no more than 3 can be present at once. - CacheManager::instance().resetStats(); + cache.resetStats(); int present = 0; for (int i = 0; i < 20; ++i) { - if (CacheManager::instance().findImage(makeSpec("file_" + std::to_string(i)))) { + if (cache.findImage(makeSpec("file_" + std::to_string(i)))) { present++; } } REQUIRE(present <= 3); // The most recently inserted entries should still be in the cache. REQUIRE(present >= 1); - REQUIRE(CacheManager::instance().findImage(makeSpec("file_19")) != nullptr); + REQUIRE(cache.findImage(makeSpec("file_19")) != nullptr); } SECTION("Image larger than maxRamBytes is not stored") { - resetCache(); // Cap at less than the size of one image. - CacheManager::instance().setConfig(ramOnlyConfig(oneImage / 2)); + cache.setConfig(ramOnlyConfig(oneImage / 2)); - CacheManager::instance().storeImage(makeSpec("too_big"), makeImage(4, 4, 4, 1)); - REQUIRE(CacheManager::instance().findImage(makeSpec("too_big")) == nullptr); + cache.storeImage(makeSpec("too_big"), makeImage(4, 4, 4, 1)); + REQUIRE(cache.findImage(makeSpec("too_big")) == nullptr); } SECTION("Re-storing the same key replaces the entry without exceeding the limit") { - resetCache(); - CacheManager::instance().setConfig(ramOnlyConfig(oneImage * 2)); + cache.setConfig(ramOnlyConfig(oneImage * 2)); auto first = makeImage(4, 4, 4, 1); auto second = makeImage(4, 4, 4, 1); auto spec = makeSpec("a"); - CacheManager::instance().storeImage(spec, first); - CacheManager::instance().storeImage(spec, second); + cache.storeImage(spec, first); + cache.storeImage(spec, second); // Fill the cache up to the limit with another entry. If the duplicate // key had double-counted bytes, this would evict "a". - CacheManager::instance().storeImage(makeSpec("b"), makeImage(4, 4, 4, 1)); + cache.storeImage(makeSpec("b"), makeImage(4, 4, 4, 1)); - auto retrieved = CacheManager::instance().findImage(spec); + auto retrieved = cache.findImage(spec); REQUIRE(retrieved != nullptr); REQUIRE(retrieved.get() == second.get()); - REQUIRE(CacheManager::instance().findImage(makeSpec("b")) != nullptr); + REQUIRE(cache.findImage(makeSpec("b")) != nullptr); } SECTION("Disabling the cache clears all entries") { - resetCache(); - CacheManager::instance().setConfig(ramOnlyConfig(oneImage * 4)); - CacheManager::instance().storeImage(makeSpec("a"), makeImage(4, 4, 4, 1)); - REQUIRE(CacheManager::instance().findImage(makeSpec("a")) != nullptr); + cache.setConfig(ramOnlyConfig(oneImage * 4)); + cache.storeImage(makeSpec("a"), makeImage(4, 4, 4, 1)); + REQUIRE(cache.findImage(makeSpec("a")) != nullptr); CacheConfig disabled; disabled.enabled = false; - CacheManager::instance().setConfig(disabled); + cache.setConfig(disabled); - REQUIRE(CacheManager::instance().findImage(makeSpec("a")) == nullptr); + REQUIRE(cache.findImage(makeSpec("a")) == nullptr); } SECTION("Shrinking the limit evicts existing entries") { - resetCache(); - CacheManager::instance().setConfig(ramOnlyConfig(oneImage * 4)); - CacheManager::instance().storeImage(makeSpec("a"), makeImage(4, 4, 4, 1)); - CacheManager::instance().storeImage(makeSpec("b"), makeImage(4, 4, 4, 1)); - CacheManager::instance().storeImage(makeSpec("c"), makeImage(4, 4, 4, 1)); + cache.setConfig(ramOnlyConfig(oneImage * 4)); + cache.storeImage(makeSpec("a"), makeImage(4, 4, 4, 1)); + cache.storeImage(makeSpec("b"), makeImage(4, 4, 4, 1)); + cache.storeImage(makeSpec("c"), makeImage(4, 4, 4, 1)); // Reconfigure to a smaller limit; oldest entries must be evicted. - CacheManager::instance().setConfig(ramOnlyConfig(oneImage)); + cache.setConfig(ramOnlyConfig(oneImage)); int present = 0; for (const char* name : { "a", "b", "c" }) { - if (CacheManager::instance().findImage(makeSpec(name))) { + if (cache.findImage(makeSpec(name))) { present++; } } @@ -314,61 +291,58 @@ TEST_CASE("CacheManager respects RAM limit and evicts LRU entries", "[cache]") SECTION("Reducing cache size immediately evicts LRU entries to fit") { - resetCache(); // Fill the cache exactly to its 4-image cap. - CacheManager::instance().setConfig(ramOnlyConfig(oneImage * 4)); - CacheManager::instance().storeImage(makeSpec("a"), makeImage(4, 4, 4, 1)); - CacheManager::instance().storeImage(makeSpec("b"), makeImage(4, 4, 4, 1)); - CacheManager::instance().storeImage(makeSpec("c"), makeImage(4, 4, 4, 1)); - CacheManager::instance().storeImage(makeSpec("d"), makeImage(4, 4, 4, 1)); + cache.setConfig(ramOnlyConfig(oneImage * 4)); + cache.storeImage(makeSpec("a"), makeImage(4, 4, 4, 1)); + cache.storeImage(makeSpec("b"), makeImage(4, 4, 4, 1)); + cache.storeImage(makeSpec("c"), makeImage(4, 4, 4, 1)); + cache.storeImage(makeSpec("d"), makeImage(4, 4, 4, 1)); // All four are resident. LRU order (most recent first) is: d, c, b, a. // Reset stats so the find() calls below count cleanly. - CacheManager::instance().resetStats(); + cache.resetStats(); // User shrinks the cache to hold only 2 images. Eviction must occur // synchronously inside setConfig() — not lazily on the next store. - CacheManager::instance().setConfig(ramOnlyConfig(oneImage * 2)); + cache.setConfig(ramOnlyConfig(oneImage * 2)); // The two least recently used entries ("a" and "b") must be gone, and // the two most recently used ("c" and "d") must still be present. - REQUIRE(CacheManager::instance().findImage(makeSpec("a")) == nullptr); - REQUIRE(CacheManager::instance().findImage(makeSpec("b")) == nullptr); - REQUIRE(CacheManager::instance().findImage(makeSpec("c")) != nullptr); - REQUIRE(CacheManager::instance().findImage(makeSpec("d")) != nullptr); + REQUIRE(cache.findImage(makeSpec("a")) == nullptr); + REQUIRE(cache.findImage(makeSpec("b")) == nullptr); + REQUIRE(cache.findImage(makeSpec("c")) != nullptr); + REQUIRE(cache.findImage(makeSpec("d")) != nullptr); - auto stats = CacheManager::instance().getStats(); + auto stats = cache.getStats(); REQUIRE(stats.misses == 2); REQUIRE(stats.ramHits == 2); } SECTION("Reducing cache size below a single image evicts everything") { - resetCache(); - CacheManager::instance().setConfig(ramOnlyConfig(oneImage * 3)); - CacheManager::instance().storeImage(makeSpec("a"), makeImage(4, 4, 4, 1)); - CacheManager::instance().storeImage(makeSpec("b"), makeImage(4, 4, 4, 1)); - CacheManager::instance().storeImage(makeSpec("c"), makeImage(4, 4, 4, 1)); + cache.setConfig(ramOnlyConfig(oneImage * 3)); + cache.storeImage(makeSpec("a"), makeImage(4, 4, 4, 1)); + cache.storeImage(makeSpec("b"), makeImage(4, 4, 4, 1)); + cache.storeImage(makeSpec("c"), makeImage(4, 4, 4, 1)); // Shrink below the size of a single entry — everything must go. - CacheManager::instance().setConfig(ramOnlyConfig(oneImage / 2)); + cache.setConfig(ramOnlyConfig(oneImage / 2)); - REQUIRE(CacheManager::instance().findImage(makeSpec("a")) == nullptr); - REQUIRE(CacheManager::instance().findImage(makeSpec("b")) == nullptr); - REQUIRE(CacheManager::instance().findImage(makeSpec("c")) == nullptr); + REQUIRE(cache.findImage(makeSpec("a")) == nullptr); + REQUIRE(cache.findImage(makeSpec("b")) == nullptr); + REQUIRE(cache.findImage(makeSpec("c")) == nullptr); } SECTION("clearMemoryCache() empties the cache") { - resetCache(); - CacheManager::instance().setConfig(ramOnlyConfig(oneImage * 4)); - CacheManager::instance().storeImage(makeSpec("a"), makeImage(4, 4, 4, 1)); - CacheManager::instance().storeImage(makeSpec("b"), makeImage(4, 4, 4, 1)); + cache.setConfig(ramOnlyConfig(oneImage * 4)); + cache.storeImage(makeSpec("a"), makeImage(4, 4, 4, 1)); + cache.storeImage(makeSpec("b"), makeImage(4, 4, 4, 1)); - CacheManager::instance().clearMemoryCache(); + cache.clearMemoryCache(); - REQUIRE(CacheManager::instance().findImage(makeSpec("a")) == nullptr); - REQUIRE(CacheManager::instance().findImage(makeSpec("b")) == nullptr); + REQUIRE(cache.findImage(makeSpec("a")) == nullptr); + REQUIRE(cache.findImage(makeSpec("b")) == nullptr); } } @@ -381,35 +355,35 @@ TEST_CASE("CacheManager disk tier round-trips images and respects the disk cap", // test. Total disk usage per test is well under a megabyte. const std::uint64_t oneImage = imageBytes(4, 4, 4, 1); - SECTION("Initializing disk cache writes the AGAVE marker file") + // Fresh temp dir + cache rooted at it for each section. + TempCacheDir tmp; + CacheManager cache(tmp.str()); + + SECTION("Enabling the disk tier writes the AGAVE marker file") { - resetCache(); - TempCacheDir tmp; - configureDiskCache(tmp.str(), oneImage * 4, 1ULL * 1024 * 1024); + cache.setConfig(diskConfig(oneImage * 4, 1ULL * 1024 * 1024)); REQUIRE(std::filesystem::exists(tmp.path() / ".agave-cache-dir")); } SECTION("Round-trip: store, drop RAM, find reloads from disk with bit-identical data") { - resetCache(); - TempCacheDir tmp; - configureDiskCache(tmp.str(), oneImage * 4, 1ULL * 1024 * 1024); + cache.setConfig(diskConfig(oneImage * 4, 1ULL * 1024 * 1024)); auto img = makeImageWithPattern(4, 4, 4, 1); auto spec = makeSpec("disk_roundtrip"); - CacheManager::instance().storeImage(spec, img); + cache.storeImage(spec, img); // Drop RAM cache; disk cache survives. - CacheManager::instance().clearMemoryCache(); - CacheManager::instance().resetStats(); + cache.clearMemoryCache(); + cache.resetStats(); - auto found = CacheManager::instance().findImage(spec); + auto found = cache.findImage(spec); REQUIRE(found != nullptr); // The reloaded image is a fresh instance, not the same shared_ptr. REQUIRE(found.get() != img.get()); - auto stats = CacheManager::instance().getStats(); + auto stats = cache.getStats(); REQUIRE(stats.diskHits == 1); REQUIRE(stats.ramHits == 0); @@ -424,43 +398,39 @@ TEST_CASE("CacheManager disk tier round-trips images and respects the disk cap", SECTION("An image larger than maxDiskBytes is not written to disk") { - resetCache(); - TempCacheDir tmp; // Cap is below a single image, so storeToDisk must refuse outright. - configureDiskCache(tmp.str(), oneImage * 4, oneImage / 2); + cache.setConfig(diskConfig(oneImage * 4, oneImage / 2)); auto spec = makeSpec("too_big_for_disk"); - CacheManager::instance().storeImage(spec, makeImage(4, 4, 4, 1)); + cache.storeImage(spec, makeImage(4, 4, 4, 1)); // No entry subdirectory should have been created. REQUIRE(countSubdirs(tmp.path()) == 0); // Drop RAM to force a disk-or-miss lookup; with no entry on disk we // should get a miss. - CacheManager::instance().clearMemoryCache(); - REQUIRE(CacheManager::instance().findImage(spec) == nullptr); + cache.clearMemoryCache(); + REQUIRE(cache.findImage(spec) == nullptr); } SECTION("Disk eviction removes the oldest entry to stay under the cap") { - resetCache(); - TempCacheDir tmp; // Cap large enough for two entries' raw byte estimate but not three. - configureDiskCache(tmp.str(), oneImage * 4, oneImage * 2); + cache.setConfig(diskConfig(oneImage * 4, oneImage * 2)); - CacheManager::instance().storeImage(makeSpec("disk_a"), makeImage(4, 4, 4, 1)); + cache.storeImage(makeSpec("disk_a"), makeImage(4, 4, 4, 1)); // Sleep briefly so lastAccess timestamps are distinct on fast disks. std::this_thread::sleep_for(std::chrono::milliseconds(5)); - CacheManager::instance().storeImage(makeSpec("disk_b"), makeImage(4, 4, 4, 1)); + cache.storeImage(makeSpec("disk_b"), makeImage(4, 4, 4, 1)); std::this_thread::sleep_for(std::chrono::milliseconds(5)); - CacheManager::instance().storeImage(makeSpec("disk_c"), makeImage(4, 4, 4, 1)); + cache.storeImage(makeSpec("disk_c"), makeImage(4, 4, 4, 1)); // Drop RAM so finds have to go through the disk tier. - CacheManager::instance().clearMemoryCache(); + cache.clearMemoryCache(); - REQUIRE(CacheManager::instance().findImage(makeSpec("disk_a")) == nullptr); - REQUIRE(CacheManager::instance().findImage(makeSpec("disk_b")) != nullptr); - REQUIRE(CacheManager::instance().findImage(makeSpec("disk_c")) != nullptr); + REQUIRE(cache.findImage(makeSpec("disk_a")) == nullptr); + REQUIRE(cache.findImage(makeSpec("disk_b")) != nullptr); + REQUIRE(cache.findImage(makeSpec("disk_c")) != nullptr); // After eviction there should be at most two entry subdirectories on // disk (the marker file is not a directory). @@ -469,15 +439,13 @@ TEST_CASE("CacheManager disk tier round-trips images and respects the disk cap", SECTION("clearDiskCache removes entry subdirectories but keeps the marker") { - resetCache(); - TempCacheDir tmp; - configureDiskCache(tmp.str(), oneImage * 4, 1ULL * 1024 * 1024); + cache.setConfig(diskConfig(oneImage * 4, 1ULL * 1024 * 1024)); - CacheManager::instance().storeImage(makeSpec("clear_a"), makeImage(4, 4, 4, 1)); - CacheManager::instance().storeImage(makeSpec("clear_b"), makeImage(4, 4, 4, 1)); + cache.storeImage(makeSpec("clear_a"), makeImage(4, 4, 4, 1)); + cache.storeImage(makeSpec("clear_b"), makeImage(4, 4, 4, 1)); REQUIRE(countSubdirs(tmp.path()) >= 2); - CacheManager::instance().clearDiskCache(); + cache.clearDiskCache(); REQUIRE(countSubdirs(tmp.path()) == 0); REQUIRE(std::filesystem::exists(tmp.path() / ".agave-cache-dir")); @@ -485,11 +453,9 @@ TEST_CASE("CacheManager disk tier round-trips images and respects the disk cap", SECTION("clearDiskCache refuses to touch a directory without the AGAVE marker") { - resetCache(); - TempCacheDir tmp; - configureDiskCache(tmp.str(), oneImage * 4, 1ULL * 1024 * 1024); + cache.setConfig(diskConfig(oneImage * 4, 1ULL * 1024 * 1024)); - CacheManager::instance().storeImage(makeSpec("guarded_a"), makeImage(4, 4, 4, 1)); + cache.storeImage(makeSpec("guarded_a"), makeImage(4, 4, 4, 1)); int subdirsBefore = countSubdirs(tmp.path()); REQUIRE(subdirsBefore >= 1); @@ -499,76 +465,62 @@ TEST_CASE("CacheManager disk tier round-trips images and respects the disk cap", std::filesystem::remove(tmp.path() / ".agave-cache-dir", ec); REQUIRE_FALSE(ec); - CacheManager::instance().clearDiskCache(); + cache.clearDiskCache(); REQUIRE(countSubdirs(tmp.path()) == subdirsBefore); } - SECTION("Re-pointing setCacheDirectory at a previously-used cache dir rebuilds the disk index") + SECTION("A fresh cache instance rebuilds the disk index from an existing cache dir") { - resetCache(); - TempCacheDir tmp; - configureDiskCache(tmp.str(), oneImage * 4, 1ULL * 1024 * 1024); - - CacheManager::instance().storeImage(makeSpec("persistent"), makeImage(4, 4, 4, 1)); + cache.setConfig(diskConfig(oneImage * 4, 1ULL * 1024 * 1024)); + cache.storeImage(makeSpec("persistent"), makeImage(4, 4, 4, 1)); - // Simulate a session restart: switch the cache dir somewhere else - // (forcing the manager to drop its in-memory disk bookkeeping), then - // point it back at the original dir. - TempCacheDir other; - configureDiskCache(other.str(), oneImage * 4, 1ULL * 1024 * 1024); - CacheManager::instance().clearMemoryCache(); + // Simulate a session restart: a brand-new CacheManager rooted at the same + // directory must rebuild its disk index on first use and serve the + // persisted entry from disk. + CacheManager restarted(tmp.str()); + restarted.setConfig(diskConfig(oneImage * 4, 1ULL * 1024 * 1024)); - configureDiskCache(tmp.str(), oneImage * 4, 1ULL * 1024 * 1024); - CacheManager::instance().resetStats(); - - auto found = CacheManager::instance().findImage(makeSpec("persistent")); + auto found = restarted.findImage(makeSpec("persistent")); REQUIRE(found != nullptr); - REQUIRE(CacheManager::instance().getStats().diskHits == 1); + REQUIRE(restarted.getStats().diskHits == 1); } } -TEST_CASE("CacheManager cache directory is registered separately from config", "[cache][disk]") +TEST_CASE("CacheManager cache directory is fixed at construction", "[cache][disk]") { const std::uint64_t oneImage = imageBytes(4, 4, 4, 1); - SECTION("getCacheDirectory reflects the registered path") + SECTION("getCacheDirectory reflects the constructed path") { - resetCache(); TempCacheDir tmp; - CacheManagerTestAccess::setCacheDirectory(tmp.str()); - REQUIRE(CacheManager::instance().getCacheDirectory() == tmp.str()); + CacheManager cache(tmp.str()); + REQUIRE(cache.getCacheDirectory() == tmp.str()); } - SECTION("With no cache directory registered, the disk tier is inert") + SECTION("A cache constructed with no directory is RAM-only") { - resetCache(); - // Clear any directory a previous test may have left registered on the - // singleton; the disk tier should then behave as RAM-only. - CacheManagerTestAccess::setCacheDirectory(""); + CacheManager cache; // empty cache directory + REQUIRE(cache.getCacheDirectory().empty()); - CacheConfig cfg; - cfg.enabled = true; - cfg.enableDisk = true; - cfg.maxRamBytes = oneImage * 4; - cfg.maxDiskBytes = 1ULL * 1024 * 1024; - CacheManager::instance().setConfig(cfg); + // Even with a disk-enabled config, an empty root keeps the disk tier inert. + cache.setConfig(diskConfig(oneImage * 4, 1ULL * 1024 * 1024)); auto spec = makeSpec("no_dir"); - CacheManager::instance().storeImage(spec, makeImage(4, 4, 4, 1)); + cache.storeImage(spec, makeImage(4, 4, 4, 1)); // RAM still serves it within the session... - REQUIRE(CacheManager::instance().findImage(spec) != nullptr); + REQUIRE(cache.findImage(spec) != nullptr); // ...but nothing was persisted to disk, so once RAM is dropped it misses. - CacheManager::instance().clearMemoryCache(); - REQUIRE(CacheManager::instance().findImage(spec) == nullptr); + cache.clearMemoryCache(); + REQUIRE(cache.findImage(spec) == nullptr); } } TEST_CASE("CacheManager invalidates entries when the source file mtime changes", "[cache][mtime]") { - resetCache(); + CacheManager cache; // Use a real file on disk so the cache key picks up its mtime via // std::filesystem::last_write_time. We bump the file's mtime explicitly @@ -582,12 +534,12 @@ TEST_CASE("CacheManager invalidates entries when the source file mtime changes", out << "initial content"; } - CacheManager::instance().setConfig(ramOnlyConfig(imageBytes(4, 4, 4, 1) * 4)); + cache.setConfig(ramOnlyConfig(imageBytes(4, 4, 4, 1) * 4)); LoadSpec spec; spec.filepath = srcFile.string(); - CacheManager::instance().storeImage(spec, makeImage(4, 4, 4, 1)); - REQUIRE(CacheManager::instance().findImage(spec) != nullptr); + cache.storeImage(spec, makeImage(4, 4, 4, 1)); + REQUIRE(cache.findImage(spec) != nullptr); // Bump the file's last_write_time well into the future; subsequent // makeKey calls now produce a different key and must miss. @@ -597,7 +549,7 @@ TEST_CASE("CacheManager invalidates entries when the source file mtime changes", std::filesystem::last_write_time(srcFile, futureTime, ec); REQUIRE_FALSE(ec); - REQUIRE(CacheManager::instance().findImage(spec) == nullptr); + REQUIRE(cache.findImage(spec) == nullptr); std::filesystem::remove(srcFile, ec); } @@ -609,142 +561,118 @@ TEST_CASE("CacheManager normalizes equivalent filepaths to the same key", "[cach // (normalized) filepath string and the other LoadSpec fields. const std::uint64_t oneImage = imageBytes(4, 4, 4, 1); + CacheManager cache; + cache.setConfig(ramOnlyConfig(oneImage * 4)); + SECTION("'./' segment is normalized away") { - resetCache(); - CacheManager::instance().setConfig(ramOnlyConfig(oneImage * 4)); - LoadSpec stored; stored.filepath = "/some/dir/foo.tif"; - CacheManager::instance().storeImage(stored, makeImage(4, 4, 4, 1)); + cache.storeImage(stored, makeImage(4, 4, 4, 1)); LoadSpec lookup; lookup.filepath = "/some/dir/./foo.tif"; - REQUIRE(CacheManager::instance().findImage(lookup) != nullptr); + REQUIRE(cache.findImage(lookup) != nullptr); } SECTION("'..' segment is normalized away") { - resetCache(); - CacheManager::instance().setConfig(ramOnlyConfig(oneImage * 4)); - LoadSpec stored; stored.filepath = "/some/dir/foo.tif"; - CacheManager::instance().storeImage(stored, makeImage(4, 4, 4, 1)); + cache.storeImage(stored, makeImage(4, 4, 4, 1)); LoadSpec lookup; lookup.filepath = "/some/dir/subdir/../foo.tif"; - REQUIRE(CacheManager::instance().findImage(lookup) != nullptr); + REQUIRE(cache.findImage(lookup) != nullptr); } SECTION("Duplicate path separators are collapsed") { - resetCache(); - CacheManager::instance().setConfig(ramOnlyConfig(oneImage * 4)); - LoadSpec stored; stored.filepath = "/some/dir/foo.tif"; - CacheManager::instance().storeImage(stored, makeImage(4, 4, 4, 1)); + cache.storeImage(stored, makeImage(4, 4, 4, 1)); LoadSpec lookup; lookup.filepath = "/some//dir///foo.tif"; - REQUIRE(CacheManager::instance().findImage(lookup) != nullptr); + REQUIRE(cache.findImage(lookup) != nullptr); } SECTION("Bare names (no slashes) pass through unchanged") { - resetCache(); - CacheManager::instance().setConfig(ramOnlyConfig(oneImage * 4)); - LoadSpec stored; stored.filepath = "in_memory_array_42"; - CacheManager::instance().storeImage(stored, makeImage(4, 4, 4, 1)); + cache.storeImage(stored, makeImage(4, 4, 4, 1)); LoadSpec lookup; lookup.filepath = "in_memory_array_42"; - REQUIRE(CacheManager::instance().findImage(lookup) != nullptr); + REQUIRE(cache.findImage(lookup) != nullptr); } SECTION("Remote URLs are passed through without normalization") { - resetCache(); - CacheManager::instance().setConfig(ramOnlyConfig(oneImage * 4)); - LoadSpec stored; stored.filepath = "http://example.com/path/to/data.zarr"; - CacheManager::instance().storeImage(stored, makeImage(4, 4, 4, 1)); + cache.storeImage(stored, makeImage(4, 4, 4, 1)); LoadSpec lookup; lookup.filepath = "http://example.com/path/to/data.zarr"; - REQUIRE(CacheManager::instance().findImage(lookup) != nullptr); + REQUIRE(cache.findImage(lookup) != nullptr); } SECTION("Distinct paths still produce distinct keys (no false hits)") { - resetCache(); - CacheManager::instance().setConfig(ramOnlyConfig(oneImage * 4)); - LoadSpec stored; stored.filepath = "/some/dir/foo.tif"; - CacheManager::instance().storeImage(stored, makeImage(4, 4, 4, 1)); + cache.storeImage(stored, makeImage(4, 4, 4, 1)); LoadSpec lookup; lookup.filepath = "/some/dir/bar.tif"; - REQUIRE(CacheManager::instance().findImage(lookup) == nullptr); + REQUIRE(cache.findImage(lookup) == nullptr); } #ifdef _WIN32 SECTION("UNC paths normalize across slash style and case on Windows") { - resetCache(); - CacheManager::instance().setConfig(ramOnlyConfig(oneImage * 4)); - LoadSpec stored; stored.filepath = "\\\\Server\\Share\\Path\\Foo.tif"; - CacheManager::instance().storeImage(stored, makeImage(4, 4, 4, 1)); + cache.storeImage(stored, makeImage(4, 4, 4, 1)); // Same UNC path, lowercase, mixed slashes. LoadSpec lookupSlash; lookupSlash.filepath = "//server/share/path/foo.tif"; - REQUIRE(CacheManager::instance().findImage(lookupSlash) != nullptr); + REQUIRE(cache.findImage(lookupSlash) != nullptr); // Same UNC path, with a '..' segment in the relative tail. LoadSpec lookupDotDot; lookupDotDot.filepath = "\\\\server\\share\\path\\subdir\\..\\foo.tif"; - REQUIRE(CacheManager::instance().findImage(lookupDotDot) != nullptr); + REQUIRE(cache.findImage(lookupDotDot) != nullptr); // Different UNC share — must NOT collide. LoadSpec otherShare; otherShare.filepath = "\\\\server\\othershare\\path\\foo.tif"; - REQUIRE(CacheManager::instance().findImage(otherShare) == nullptr); + REQUIRE(cache.findImage(otherShare) == nullptr); } SECTION("Forward and back slashes are equivalent on Windows") { - resetCache(); - CacheManager::instance().setConfig(ramOnlyConfig(oneImage * 4)); - LoadSpec stored; stored.filepath = "C:/some/dir/foo.tif"; - CacheManager::instance().storeImage(stored, makeImage(4, 4, 4, 1)); + cache.storeImage(stored, makeImage(4, 4, 4, 1)); LoadSpec lookup; lookup.filepath = "C:\\some\\dir\\foo.tif"; - REQUIRE(CacheManager::instance().findImage(lookup) != nullptr); + REQUIRE(cache.findImage(lookup) != nullptr); } SECTION("Case differences are equivalent on Windows") { - resetCache(); - CacheManager::instance().setConfig(ramOnlyConfig(oneImage * 4)); - LoadSpec stored; stored.filepath = "C:/Some/Dir/Foo.tif"; - CacheManager::instance().storeImage(stored, makeImage(4, 4, 4, 1)); + cache.storeImage(stored, makeImage(4, 4, 4, 1)); LoadSpec lookup; lookup.filepath = "c:/some/dir/foo.tif"; - REQUIRE(CacheManager::instance().findImage(lookup) != nullptr); + REQUIRE(cache.findImage(lookup) != nullptr); } #endif } From f6764d537f4743a36bc49a0f7c10c67b514cd6ad Mon Sep 17 00:00:00 2001 From: toloudis Date: Tue, 2 Jun 2026 20:01:41 -0700 Subject: [PATCH 31/31] lint fixes --- renderlib/CacheManager.cpp | 6 +++--- renderlib/CacheManager.h | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/renderlib/CacheManager.cpp b/renderlib/CacheManager.cpp index 7bdd2eca2..e8721e1d0 100644 --- a/renderlib/CacheManager.cpp +++ b/renderlib/CacheManager.cpp @@ -152,7 +152,7 @@ CacheKeyHash::operator()(const CacheKey& key) const std::size_t seed = 0; hashCombine(seed, std::hash{}(key.filepath)); hashCombine(seed, std::hash{}(key.subpath)); - hashCombine(seed, std::hash{}(key.scene)); + hashCombine(seed, std::hash{}(key.scene)); hashCombine(seed, std::hash{}(key.time)); hashCombine(seed, std::hash{}(key.minx)); hashCombine(seed, std::hash{}(key.maxx)); @@ -673,7 +673,7 @@ CacheManager::loadFromDisk(const CacheKey& key, const CacheConfig& config, const tensorstore::Open({ { "driver", "zarr3" }, { "kvstore", { { "driver", "file" }, { "path", dataPath.string() } } } }, tensorstore::OpenMode::open, tensorstore::ReadWriteMode::read); - auto result = openFuture.result(); + const auto& result = openFuture.result(); if (!result.ok()) { return nullptr; } @@ -773,7 +773,7 @@ CacheManager::storeToDisk(const CacheKey& key, { "kvstore", { { "driver", "file" }, { "path", dataPath.string() } } }, { "schema", schema } }, tensorstore::OpenMode::create | tensorstore::OpenMode::open); - auto result = openFuture.result(); + const auto& result = openFuture.result(); if (!result.ok()) { std::error_code rmEc; std::filesystem::remove_all(entryPath, rmEc); diff --git a/renderlib/CacheManager.h b/renderlib/CacheManager.h index 6ed78a8db..1cbd48f70 100644 --- a/renderlib/CacheManager.h +++ b/renderlib/CacheManager.h @@ -17,7 +17,7 @@ struct CacheKey { std::string filepath; std::string subpath; - int scene = 0; + std::uint32_t scene = 0; std::uint32_t time = 0; std::vector channels; std::uint32_t minx = 0;