…1146) Implements #1066, addressing @mkepa-nv's review note on #1034: > This interleaved usage of `index_hash` and `key` looks error prone. Maybe in a future MPR it would be better to use `key` for `lock()`? ## The problem The cuslide2 tile path built a key from `(ifd_hash, tile_index)` and then, on an adjacent line, computed a separate `index_hash` from those same two values by hand: ```cpp // Hash for per-tile locking const uint64_t index_hash = ifd_hash ^ (static_cast<uint64_t>(tile_index) | (static_cast<uint64_t>(tile_index) << 32)); // --- Cache lookup (RAII lock guard for exception safety) --- auto key = image_cache.create_key(ifd_hash, tile_index); ``` The same tile was described two different ways with nothing tying them together, so changing one would silently not reach the other. Worth being explicit about why that is worse than ordinary duplication: the hash only selects which mutex of the pool guards an entry, so a divergence would not fail loudly or immediately. `lock()` and `unlock()` would simply operate on different mutexes, and the tile would be decoded and inserted with no lock actually held. That is a latent data race, not a crash. ## The change `ImageCacheKey::lock_hash()` derives the value from the key, and cuslide2 passes `key->lock_hash()` to its lock guard. The formula is exactly the one the IFD readers computed inline, so every tile still maps onto the mutex it mapped to before — **no behavioural change**. ## Scope Deliberately limited to cuslide2, the file the review comment was on. Two other places derive a lock hash by hand and are left alone: - **Legacy cuslide `ifd.cpp`** keeps its `index_hash` because a `PROF_SCOPED_RANGE` profiling marker consumes it earlier in the function, before any key exists. - **`NvJpegProcessor`** derives its lock hash with `splitmix64(index)` in both `request()` and `wait_for_processing()`. Both are self-consistent today, so converting them would change which mutex a tile maps to without any functional gain, on a code path I cannot test here. `lock_hash()` is available to them whenever that code is next touched with tests to back the change. ## Tests `cpp/tests/test_image_cache.cpp` covers the invariants callers actually depend on rather than restating the implementation: - the hash is a pure function of the key - separately constructed equal keys agree — this is the real-world case, where the lookup and the later insert each build their own key - the value still matches the previous inline formula, pinning the no-behaviour-change claim - consecutive tile indices land in distinct pool slots, which is the access pattern tile assembly produces - one tile index in different IFDs does not share a slot ## Verification `-fsyntax-only` against the project's own `compile_commands.json` for `image_cache.cpp`, `ifd.cpp` and the new test, which compiles under the test target's `-Werror -Wall -Wextra`. All clean. I also executed the test assertions standalone, since compiling a test proves nothing about whether it passes: ``` repeated calls agree PASS equal keys agree PASS matches inline IFD formula PASS 64 consecutive indices -> 64 distinct slots PASS same index, different IFD -> different hash PASS pool=8 -> all distinct PASS pool=16 -> all distinct PASS pool=32 -> all distinct PASS pool=128 -> all distinct PASS pool=256 -> all distinct PASS ``` The distinct-slot property holds across pool sizes 8 through 256, so the assertion is not overfit to the single size the test uses. ## Unrelated issue noticed nearby, not fixed here While tracing the key's hashing I found that `PerProcessImageCache`'s equality comparator ignores `file_hash`: https://github.com/rapidsai/cucim/blob/main/cpp/src/cache/image_cache_per_process.cpp#L25-L29 ```cpp bool equal_to<std::shared_ptr<cucim::cache::ImageCacheKey>>::operator()( const std::shared_ptr<cucim::cache::ImageCacheKey>& lhs, const std::shared_ptr<cucim::cache::ImageCacheKey>& rhs) const { return lhs->location_hash == rhs->location_hash; } ``` `std::hash` for the same key does combine both fields, so two keys differing only in `file_hash` normally land in different buckets and the comparator is never asked. But bucket assignment is `hash % bucket_count`, so a collision is possible, and when it happens this returns a tile belonging to a different image or IFD. `SharedMemoryImageCache`'s comparator does compare both fields, which suggests this one is an oversight rather than intentional. Left out of this PR since it is a separate defect with its own behavioural risk. Happy to file it as its own issue if you agree it is one. Authors: - https://github.com/cdinea Approvers: - Gigon Bae (https://github.com/gigony) - https://github.com/jakirkham URL: #1146

[FEA]Add lock_hash() method to ImageCacheKey #1066

Description

Activity

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions