fix(hipdnn): winner-cache follow-up fixes (ALMIOPEN-2505) - #11493
Draft
SamuelReeder wants to merge 3 commits into
Draft
fix(hipdnn): winner-cache follow-up fixes (ALMIOPEN-2505)#11493SamuelReeder wants to merge 3 commits into
SamuelReeder wants to merge 3 commits into
Conversation
Five correctness fixes, three Windows fixes, the write-path adopt-gate change plus one lock-hold fix, six test/CI fixes, and one doc wording fix, per the ALMIOPEN-2451/PR#11101 review follow-ups. - LineStore.hpp: retain descriptors on the two unsafe close paths in openOrFindLineStoreEntry()/findExistingLineStoreEntry(); retry fcntl(F_SETLKW) on EINTR; add FILE_SHARE_DELETE to both registry CreateFileW calls; stop leaking NOMINMAX/WIN32_LEAN_AND_MEAN past <windows.h>. - CacheRoot.hpp: decline an unresolved '~'/'%USERPROFILE%' instead of creating a literal '~' cache directory. - PathSanitizer.hpp: correct the collision-bound doc comment. - WinnerCacheFile.hpp: per-line format-version stamp; bounded integer reads for warp_size/multi_processor_count. - WinnerCache.hpp/KernelIngestorStateManager.hpp/GenericPlanBuilder.hpp: replace the ranking-comparison adopt gate with an explicit WinnerWriteCause (fresh-miss presence check vs. coverage-rebenchmark append); release the shard lock before logging an append failure. - Test/CI: clear HIPDNN_DISABLE_CACHE in the cross-process ctest env; ship the three DiskCacheCrossProcess_* entries in the install tree; new LineStore nested-exclusive-decline and fault-injection tests; remove wall-clock dependence from the LineStore process tests; port the cross-process lock tests to Windows; dedupe ContentCarryingTestGraph.hpp in favor of flatbuffers_sdk's copy. - docs/Environment.md: record the two accepted design deviations (shard path shape, sanitizeForPath() on the engine component) and the Windows delete-safety caveat pending CI observation. Verified in-container: ingestor-ON and -OFF builds clean (clang-tidy gated); hipdnn_data_sdk_tests and hipdnn_plugin_sdk_tests suites pass, including TestLineStore.* repeated 20x with zero flakes; DiskCacheCrossProcess_* pass in both the build tree and an installed tree, including under an ambient truthy HIPDNN_DISABLE_CACHE; HOME-unset CacheRoot regression test passes and creates no literal '~' directory.
✅ All Checks Passed — Ready for Review
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
Pre-commit check failed⛔ pre-commit failed Please run locally:
This repo uses |
…Helper - Reformat the 6 files pre-commit flagged (clang-format-18, project style). - LineStoreLockHelper: set stdout to binary mode on Windows so the CRT does not rewrite the helper's \n to \r\n, which made the parent test read back "arm\r" instead of "arm" on Windows CI.
|
🎉 All checks passed! This PR is ready for review. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #11493 +/- ##
===========================================
+ Coverage 69.35% 69.37% +0.01%
===========================================
Files 2810 2811 +1
Lines 464609 464651 +42
Branches 68557 68558 +1
===========================================
+ Hits 322227 322323 +96
+ Misses 118866 118820 -46
+ Partials 23516 23508 -8
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
- Fix stale writeBackToShard()/recordWinner() doc comments that still described the deleted rankedIdsEqual() gate instead of the cause-based adopt/append rule (B1). - Resolve the Windows cache-root-deletion doc placeholder now that TestLineStore.AShardsParentDirectoryCanBeRemovedWhileTheRegistryHoldsItOpen has passed on Windows CI: deletion is safe on both platforms (B2). - Make ChildProcess in TestLineStore.cpp move-only and self-reaping (RAII kill+reap+close), so an ASSERT_* between spawning a helper and the normal awaitChild()/awaitProbe() reap no longer orphans a child spinning on the barrier file for the rest of the CI job (I1/I2). Bound LineStoreLockHelper's barrier wait with a 60s timeout as a second, independent backstop. - Apply the NOMINMAX/WIN32_LEAN_AND_MEAN define/undef-after-<windows.h> containment to PlatformUtils.windows.hpp, matching LineStore.hpp; this header was the one still leaking both macros to every downstream Windows translation unit (I3). - Strengthen ARecordUnderAnUnusableGraphKeyIsNotStored to record a valid key first, so the test distinguishes rejecting the unusable key from recordWinner() doing nothing at all (I4). - Strengthen the probe-mode arm checks in TestLineStore.cpp to assert the marker text, not just presence. - Make WinnerWriteCause a required recordWinner() parameter (no more FRESH_MISS default), with every call site now passing it explicitly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implement the ALMIOPEN-2505 follow-up fixes from the ALMIOPEN-2451 review and PR #11101. The changes harden LineStore behavior, make winner-cache writes deterministic under races, improve malformed-record handling, and complete the related test and install-tree coverage.
JIRA ID : ALMIOPEN-2505
Related PR: #11101
Risk Assessment
Risk level: 3. The change affects the shared LineStore header, winner-cache persistence, and CMake test installation. The ingestor remains disabled by default, and the changed behavior is covered by focused data SDK and plugin SDK tests. Windows-specific behavior still requires Windows CI confirmation.
ASIC Coverage
The changes are architecture-independent host-side persistence, locking, test, and build infrastructure changes. They do not alter kernel selection, provider applicability, dispatch, or device behavior. Standard PR CI is sufficient; Windows CI is required for the Windows handle and process-test changes. A full multi-arch sweep is not required.
Testing Summary
~directory creation.HIPDNN_DISABLE_CACHE.Testing Checklist
cmake --build build- Status: Passedcmake --build build-off --target hipdnn_data_sdk_tests- Status: Passed/usr/bin/ctest --test-dir build/projects/hipdnn -R hipdnn_data_sdk_tests --output-on-failure- Status: Passedhipdnn_data_sdk_tests --gtest_filter='TestLineStore.*' --gtest_repeat=20- Status: Passedhipdnn_plugin_sdk_tests --gtest_filter='TestIngestorWinnerCache*:TestIngestorGenericPlanBuilder*'- Status: Passedctest --test-dir build/projects/hipdnn -R DiskCacheCrossProcess --output-on-failure- Status: Passedctest --test-dir stage/bin/hipdnn -R DiskCacheCrossProcess --output-on-failure- Status: Passedhipdnn_data_sdk_tests --gtest_filter='TestCacheRoot.*'- Status: Passedpre-commit- required repository check - Status: PendingTechnical Changes
fcntllock, retry interrupted lock acquisition, add Windows delete sharing, and contain Windows macro definitions.~cache path.