Fix concurrent XNNPACK allocator initialization - #32489
Fix concurrent XNNPACK allocator initialization#32489Silu Panda (SiluPanda) wants to merge 1 commit into
Conversation
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
Scott McKay (@skottmckay) could you please review this focused fix for #32461, given your context on the issue and the XNNPACK allocator implementation? The new fresh-process regression failed 12/20 times with the original implementation and passed 100/100 with the fix on macOS arm64 ( Could you enable the appropriate PR validation when eligible, particularly the native macOS/Windows XNNPACK coverage? The CLA bot has requested a signature, which I will leave to the contributor to complete. |
There was a problem hiding this comment.
🟢 Approval recommended
The synchronization correctly protects the full initialization sequence, and the regression test exercises the original race and allocator lifetime.
Pull request overview
Makes process-wide XNNPACK allocator initialization thread-safe and preserves allocator lifetime.
Changes:
- Uses
std::call_oncefor allocator creation, publication, and XNNPACK initialization. - Adds a fresh-process, 32-thread regression test covering identity and lifetime.
File summaries
| File | Description |
|---|---|
onnxruntime/core/providers/xnnpack/xnnpack_execution_provider.cc |
Serializes global allocator initialization. |
onnxruntime/test/providers/xnnpack/xnnpack_basic_test.cc |
Tests concurrent initialization and retained allocator use. |
No actionable findings identified.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
@microsoft-github-policy-service agree |
|
Validated the patch at cdc43f6 on both the original macOS reproducer and the React Native / iOS workload from #32461, with concurrent model loading restored. It fixes the race. macOS arm64 (macOS 26, Apple clang 21, Release,
For reference, the same reproducer crashes 49/100 against the released 1.27.0 binary we ship, so it is a much weaker control against main than your death test. Your test is the one that matters there and its failure rate without the fix matches your numbers. iOS, React Native. Built the iOS static framework from the branch (
One caveat on the device numbers: our unfixed build never crashed from that JS harness either (0/28 today, 0/30 last week), even though scribbling is confirmed active in the process. Without a spin barrier the RN loader threads rarely land in the window, so treat the iOS runs as a functional check of concurrent creation against the fixed library, not as a crash-versus-no-crash comparison. The macOS death test is the negative control. Also checked that moving Two small notes on the iOS build, in case it helps the CI story: with CMake 4 the CoreML path needs Thanks for the quick turnaround on this. |
Description
Make the process-wide XNNPACK allocator initialization thread-safe with
std::call_once. The once callback covers allocator creation, wrapper context publication, andxnn_initialize, so concurrent EPs cannot replace the allocator or rewrite the context while existing kernels use it.Initialization errors still throw inside the callback, allowing a retry. The existing stored-allocator guard is retained so a retry does not replace an allocator XNNPACK may already have retained.
Add a cold-start regression test that re-executes in a fresh subprocess, synchronizes 32 calls to
CreatePreferredAllocators(), and checks allocator identity across EPs. After releasing the per-EP owners, it exercises XNNPACK's retained allocator through workspace allocation/release and verifies subsequent EP creation can still use the same allocator.Motivation and Context
Fixes #32461.
XNNPACK retains its allocator context process-wide. Concurrent first-time session initialization could replace the owning
shared_ptrafter XNNPACK captured its pointer, resulting in a dangling context. The wrapper is also read by existing kernels, so successful initialization must not rewrite it on subsequent EP creation.Validation
Locally built on macOS 26.3.1 arm64 with Apple Clang 17, Release, and XNNPACK enabled; no GPU required.
MallocScribble=1(including 7 SIGSEGV failures).MallocScribble=1.Xnnpack*passed; 12 existing tests remain disabled.git diff --checkpassed.Build configuration:
The pinned-dependency option avoids a locally installed Protobuf/runtime mismatch. When building only the provider-test target, I copied
onnxruntime/test/testdataandsamplesto the build output directory, matching the fixture-copy steps normally attached toonnxruntime_test_all.From
build/MacOS/Release:MallocScribble=1 ./onnxruntime_provider_test \ --gtest_filter=XnnpackEPDeathTest.ConcurrentAllocatorInitialization --gtest_repeat=100 MallocScribble=1 ./onnxruntime_provider_test --gtest_filter='Xnnpack*'The regression directly exercises allocator initialization/lifetime, not the reporter's complete React Native workload. iOS/React Native and native Windows have not been tested locally. The subprocess test is guarded by
GTEST_HAS_DEATH_TESTand is not compiled on iOS/WASM. Initialization-failure retry behavior was reviewed but not fault-injection tested.