Repository navigation
ALMIOPEN-2598: Reuse KernDb connections via cached-database mechanism - #11933
Merged
Merged
Conversation
✅ All Checks Passed — Ready for Review
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🎉 All checks passed! This PR is ready for review. |
SQLiteBase::GetCached had a 2-arg signature (path, is_system) while MultiFileDb::GetDbInstance probed for a 3-arg form (DbKinds, path, bool). The SFINAE mismatch caused the rank<1> caching overload to silently fail, so every LoadBinary/SaveBinary call opened and closed a fresh SQLite connection (sqlite3_open, schema creation, PRAGMA table_info, sqlite3_close). - Change GetCached to accept (DbKinds, path, is_system), matching the RamDb/ReadonlyRamDb pattern so rank<1> now wins - Switch internal storage to unique_ptr<Derived> to avoid requiring Derived to be movable - Add per-instance mutex (instance_mutex) to protect concurrent operations on shared cached instances - Wrap all public entry points (FindRecord, StoreRecord, RemoveRecord, Remove, Update, Load) with lock_guard before delegating to Unsafe methods - Add tests for connection reuse, path isolation, and multi-threaded safety Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
iq136boy
force-pushed
the
users/dfeng/reuse-kerndb-cached-connections
branch
from
September 11, 2026 16:58
d8ea6b5 to
ece2e19
Compare
Contributor
Author
|
The The error is in an external This file doesn't exist in the All actual MIOpen builds and GPU tests pass (gfx90a, gfx942, gfx110X on both Linux and Windows). |
…kerndb-cached-connections
DarylHawkinsAMD
requested changes
Oct 5, 2026
- Narrow lock scope: move locking from SQLiteBase wrappers into *Unsafe methods so KernDb::FindRecordUnsafe locks only the SQLite query while bz2 decompression and MD5 run concurrently across threads - Use std::recursive_mutex for safe re-entry (LoadUnsafe→FindRecordUnsafe) - Key cache on (path, is_system) to prevent read-only/read-write conflicts - Add EvictCached() so tests can release connections before TempFile cleanup - Fix std::vector<bool> data race → std::vector<int> - Add static_assert to catch future SFINAE probe drift at compile time - Rewrite thread safety test with concurrent StoreRecord + FindRecord - Update PR description to mention SQLitePerfDb benefits Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
DarylHawkinsAMD
approved these changes
Oct 6, 2026
Contributor
TheRock Submodule Bump ActivityNewest first
|
mattwill-amd
pushed a commit
that referenced
this pull request
Oct 14, 2026
JIRA ID: ALMIOPEN-2913 ## Motivation Only 5 of the 30 commits that landed against `projects/miopen` since the 3.6.2 version bump (`2d20a8f775e`, #12211) were reflected in `CHANGELOG.md`. The remaining 25 were reviewed and the 15 externally-visible ones documented, so the ROCm 10.2.0 section accurately reflects what shipped. 14 entries added (two of the fixes share one entry), plus a duplicate-heading fix. ### Added * [Conv] NHWC (channel-last) support in the GEMM convolution solvers, with new im2col/col2im kernels (#8693) * [Conv] Cross-architecture AI heuristics for immediate mode (`MIOPEN_FIND_MODE=2`), extending data-driven selection to Navi (#9680) * [Conv] Missing 3x3 stride-1 pad-1 tile-28 CK depthwise backward-data instance — ALMIOPEN-2423 (#11271) ### Changed * [Conv] `ConvHipConv` held out of the AI perf-config picker until its model is retrained — ALMIOPEN-2494 (#13337) * [Conv] CK byte-span gate lifted for forward and backward-data, now that CK fixes the 32-bit offset overflow itself — ALMIOPEN-2645 (#12396) ### Optimized * [Conv] `ConvDirectNaiveConv`: per-direction kernel files, float accumulators, spatial tiling, bfloat16 path — ALMIOPEN-2485/2486 (#7529) * Kernel cache reuses cached SQLite connections instead of reopening per load/store — ALMIOPEN-2598 (#11933) ### Resolved Issues * [Conv] Incorrect backward-weights results from the gfx950 direct kernel at `waves_q == 1` (missing cross-lane ordering on LDS staging reuse) — ROCM-31508 (#12406) * [Conv] Up to 8x slower hipconv `direct`/`direct_l1`/`direct_wgrad` under ROCm 10.1's full AMDGCN LTO default; fixed with `-fno-offload-lto` — ROCM-31334 (#12328) * [Conv] gfx942 multi-node training throughput regression from backward GEMM on small-batch 1x1 NHWC shapes — ROCM-31155 (#13232) * [Conv] Invalid CK perf-config entries in the gfx1201 and gfx1100 system databases — ROCM-29026 (#12088, #12117) * [BatchNorm] JIT compilation failures on non-GFX9 targets; Welford DPP assembly restricted to GFX9 — ALMIOPEN-2838 (#12889) * Out-of-bounds write in `miopenLoadSolution` when a serialized solution carries more than three work dimensions — ROCM-31082 (#12242) * `uint64_t` typedef conflict between `miopen_cstdint.hpp` and system `<stdint.h>` on LP64 Linux (#12623) ## Technical Details Single-file change to `projects/miopen/CHANGELOG.md`. The 10.2.0 section had **two** `### Resolved Issues` headings. The two entries under the trailing one (`ConvHipConv` gfx1250 crash/LDS race, and the perf-db reload config loss) were merged into the first, and the trailing heading removed — this accounts for the 2 removed bullets in the diff. Section order is now `Resolved Issues → Added → Changed → Optimized → Removed`, matching the 3.6.1 section. `Optimized` is new to this release section. Already documented, so untouched: #12874 (reduce int32 rejection), #12000 / #13016 / #13213 (hipconv v0.3.1 / v0.3.2 / v0.4.0), #11761 (gfx803 removal). ## Notes Deliberately excluded as not externally visible (10 commits): * **hipDNN convolution forwarding (#12276)** — `MIOPEN_ENABLE_HIPDNN_WRAPPER` defaults to `OFF` (`projects/miopen/CMakeLists.txt:162`), so no shipped build compiles the forwarding code and the runtime `MIOPEN_HIPDNN_FORWARDING` flag has nothing to enable. * **Test-only**: #13289 (gfx1250 depthwise test applicability, ALMIOPEN-2841), #11610 (hipDNN parity harness), #12660 (Jenkins status post). * **Internal kernel-math cleanup, generated ISA unchanged**: #13312 (drop direct `__ocml` calls), #13190 (remove broken single-argument `__half` overloads). * **Add/revert pairs, net no change**: #12466 + #13066 (gfx1101 Navi system DBs), #12496 + #12800 (gfx1250 in the dev image arch lists). ## Risk Assessment Documentation-only change to a single Markdown file; no code, build, or runtime impact. **Risk Level: 🟢 Low** ## Test Plan None required — Markdown-only. Verified that the entry topics are not already documented elsewhere in the file, that no content was lost when merging the duplicate heading, and that every new entry maps to a commit in `2d20a8f775e..origin/develop -- projects/miopen`. ## Test Result 16 bullets added, 2 relocated — 14 net-new entries covering 15 commits. Duplicate heading removed; section ordering matches 3.6.1. Co-authored-by: Claude <noreply@anthropic.com>
This was referenced Nov 5, 2026
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
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
SQLiteBase::GetCachedsignature from(path, is_system)to(DbKinds, path, is_system)so therank<1>SFINAE caching overload inMultiFileDb::GetDbInstancematches, eliminating redundantsqlite3_open/sqlite3_closeperLoadBinary/SaveBinarycallunique_ptr<Derived>(matchingRamDbpattern) to avoid requiringDerivedto be movablestd::recursive_mutexwith fine-grained locking in each*Unsafemethod for thread safety on shared cached instances. ForKernDb, the lock covers only the SQLite step so that bz2 decompression and MD5 verification can run concurrently across threads(path, is_system)to prevent read-only/read-write mode conflictsEvictCached()to allow tests to release cached connections before temp files are deleted (required for Windows cleanup)static_assertto catch future SFINAE probe driftSQLitePerfDbalso benefits from this caching whenMIOPEN_USE_SQLITE_PERFDB=On, since the fix is in the sharedSQLiteBasetemplateStrace benchmark results
Measured filesystem syscalls over 50 iterations of
GetDbInstance<KernDb>()+FindRecord()usingstrace -f -e trace=file, comparingdevelop(before) vs this branch (after):openaton KDB file)openatsyscallsnewfstatatsyscallsRoot cause: On
develop,SQLiteBase::GetCachedhad a 2-arg signature(path, is_system)whileGetDbInstanceprobed for a 3-arg form(DbKinds, path, bool). The SFINAE mismatch caused therank<1>caching overload to silently fail, so everyGetDbInstance<KernDb>()call fell through torank<0>— constructing a freshKernDbeach time (sqlite3_open, schema creation, PRAGMAs,sqlite3_close). This is especially costly when the KDB resides on a network filesystem (e.g. NFS), as each open/close is a round-trip.Test plan
check_kern_dbtest passescheck_kern_db_cached_reuse— same path returns same instancecheck_kern_db_cached_distinct_paths— different paths return different instancescheck_kern_db_cached_thread_safety— 8 threads concurrently store and read back per-thread recordsstatic_assertensuresGetDbInstance<KernDb>returns a reference (caching overload)JIRA ID : ALMIOPEN-2598
🤖 Generated with Claude Code