Trying to improve performance for another project by adding some useful caching, etc. - #175
Trying to improve performance for another project by adding some useful caching, etc.#175amrosado wants to merge 4 commits into
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #175 +/- ##
==========================================
+ Coverage 67.91% 68.13% +0.22%
==========================================
Files 99 99
Lines 13467 13718 +251
==========================================
+ Hits 9146 9347 +201
- Misses 4321 4371 +50
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@ptahmose Can you allow the testing workflow to go through so I can see what is missing/needed? |
|
Circling back on this, can you please help me with getting the pull request tested/done? |
|
Thank you for your contribution. Before we can proceed with the review and potential integration of this contribution, we need a signed Contributor License Agreement (CLA). Please have a look at our contribution guidelines here: https://github.com/ZEISS/libczi/blob/main/CONTRIBUTING.md A scanned copy or even a clear photo of the signed document is perfectly fine. Once we have the signed CLA, we can continue with the review process. Please note that the technical evaluation of the proposed changes will likely take some additional time, as we need to analyze the impact and implications in more detail. Having said this - at first glance... The fact that libCZI would itself maintain and manage concurrency and threads is not really inline with my ideas how to introduce concurrency. I'd opt for keeping libCZI itself threading-agnostic, and concurrency being externally injected - if this makes sense. Concretely - an interface something like the "TaskExecutor" (but thread-API agnostic) being passed to the libCZI-functions - or here is an example. But - that's just my first idea here. |
There was a problem hiding this comment.
Pull request overview
This PR introduces several performance-oriented changes across libCZI’s tile accessors and repository internals, primarily targeting faster ROI reads, avoiding redundant metadata work, improving subblock-cache behavior under concurrency, and enabling limited parallel subblock decode/read workflows.
Changes:
- Add “single-flight” cache loading (
GetOrCreate) and switch the built-in subblock cache to an O(1) LRU eviction scheme. - Add an indexed subset-enumeration path in the CZI subblock directory and expose an extended subset enumeration interface (
EnumSubsetEx) with optional scene filtering. - Optimize tile accessors by (a) directly returning a decoded bitmap for the “unique exact layer-0 tile” case and (b) adding optional concurrent subblock reads in the scaling accessor.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| Src/libCZI/TaskExecutor.h | Adds an internal task-executor API intended to avoid per-job thread creation. |
| Src/libCZI/TaskExecutor.cpp | Implements the internal worker pool. |
| Src/libCZI/subblock_cache.h | Updates cache structures for LRU + in-flight load coordination; adds GetOrCreate. |
| Src/libCZI/subblock_cache.cpp | Implements O(1) LRU + single-flight loading; updates statistics/pruning behavior. |
| Src/libCZI/SingleChannelTileAccessor.h | Extends subblock enumeration helpers to accept a scene filter. |
| Src/libCZI/SingleChannelTileAccessor.cpp | Adds “exact single layer-0 tile” fast path; uses optimized subset enumeration when available. |
| Src/libCZI/SingleChannelScalingTileAccessor.h | Adds scene-filter plumbing and optional prefetched data for scaling blits; updates scene determination signature. |
| Src/libCZI/SingleChannelScalingTileAccessor.cpp | Adds optional concurrent subblock reads and scene-filter-aware optimized enumeration. |
| Src/libCZI/SingleChannelAccessorBase.cpp | Switches cache usage to GetOrCreate and avoids caching uncompressed data when configured. |
| Src/libCZI/libCZI_Compositor.h | Adds default GetOrCreate to cache interface; adds scaling accessor options (maxConcurrentSubBlockReads, sceneIndex). |
| Src/libCZI/CziSubBlockDirectory.h | Adds data structures and APIs for indexed subset enumeration and scene/channel helpers. |
| Src/libCZI/CziSubBlockDirectory.cpp | Builds and uses a subset index to accelerate subset enumeration; adds helper lookups. |
| Src/libCZI/CZIReader.h | Introduces ISubBlockRepositorySubsetEx and implements it on CCZIReader. |
| Src/libCZI/CZIReader.cpp | Routes subset enumeration through the indexed directory when possible; adds scene-bbox and channel helper usage. |
| Src/libCZI_UnitTests/test_SubBlockCache.cpp | Adds a concurrency test ensuring cache misses are loaded exactly once. |
| Src/libCZI_UnitTests/test_CziSubBlockDirectory.cpp | Adds tests for indexed subset enumeration correctness and helper APIs. |
| Src/libCZI_UnitTests/test_Accessors.cpp | Exercises new scaling concurrency option and adds tests for the tile accessor fast path behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| #include <condition_variable> | ||
| #include <cstddef> | ||
| #include <functional> | ||
| #include <future> |
| #include "BitmapOperations.h" | ||
| #include "BitmapOperationsBitonal.h" | ||
| #include "Site.h" | ||
| #include <future> |
| std::vector<int> scenesInvolved = options.sceneIndex != (std::numeric_limits<int>::min)() ? | ||
| std::vector<int>{ options.sceneIndex } : | ||
| this->DetermineInvolvedScenes( | ||
| roi, effectiveSceneFilter, options.sceneIndex); |
| if (!sub_block_repository->TryGetSubBlockInfo(sub_block_index, &result.subBlockInfo)) | ||
| { | ||
| stringstream ss; | ||
| ss << "SubBlockInfo not found in repository for subblock index " << sub_block_index << "."; | ||
| throw logic_error(ss.str()); | ||
| } |
| reads.emplace_back(std::async(std::launch::async, [this, &options, subBlockIndex] | ||
| { | ||
| return CSingleChannelAccessorBase::GetSubBlockDataIncludingMaskForSubBlockIndex( | ||
| this->sbBlkRepository, | ||
| options.subBlockCache, | ||
| subBlockIndex, | ||
| options.onlyUseSubBlockCacheForCompressedData, | ||
| options.maskAware); | ||
| })); |
Fill out and Adjust this Template
Description
Summary of the change(s) and which issue(s) is/are fixed:
In trying to setup libczi based tile source for large image I noticed some issues with performance of my eager iterator which I use to use multiprocessing to retrieve image information. In analyzing the C++ code there were some changes that could help with avoiding having to do expensive operations with metadata to support faster region of interest reads and allow parallel subblock decoding as well as consolidating some expensive computational operations and preventing expensive memory copies.
Not sure if your group accepts contributions or how active this project is, but let me know if you want further things done for a pull request.