perf(face-swapper): use double-check locking to reduce lock contention#1672
perf(face-swapper): use double-check locking to reduce lock contention#1672laurigates wants to merge 1 commit intohacksider:mainfrom
Conversation
Reviewer's GuideIntroduces a double-checked locking pattern around the global FACE_SWAPPER initialization to reduce lock contention while preserving thread-safe lazy loading, and slightly adjusts CoreML provider config behavior. Sequence diagram for double-checked locking in get_face_swappersequenceDiagram
actor Thread1
actor Thread2
participant FaceSwapperModule
participant InsightFaceModelZoo
participant CoreMLSession
Thread1->>FaceSwapperModule: get_face_swapper()
Thread2->>FaceSwapperModule: get_face_swapper()
rect rgb(235, 245, 255)
FaceSwapperModule-->>Thread1: check FACE_SWAPPER is None
FaceSwapperModule-->>Thread2: check FACE_SWAPPER is None
end
Thread1->>FaceSwapperModule: acquire THREAD_LOCK
Thread2-->>FaceSwapperModule: wait for THREAD_LOCK
FaceSwapperModule-->>FaceSwapperModule: second check FACE_SWAPPER is None
FaceSwapperModule->>InsightFaceModelZoo: get_model(model_path, providers_config)
InsightFaceModelZoo-->>FaceSwapperModule: FACE_SWAPPER instance
alt CoreMLExecutionProvider enabled
FaceSwapperModule->>CoreMLSession: session = FACE_SWAPPER.session
FaceSwapperModule->>CoreMLSession: session.run(warmup_input)
CoreMLSession-->>FaceSwapperModule: warmup complete
else CoreMLExecutionProvider not enabled
FaceSwapperModule-->>FaceSwapperModule: skip CoreML warmup
end
FaceSwapperModule-->>Thread1: release THREAD_LOCK
Thread2->>FaceSwapperModule: acquire THREAD_LOCK
FaceSwapperModule-->>FaceSwapperModule: second check FACE_SWAPPER is not None
FaceSwapperModule-->>Thread2: return FACE_SWAPPER
FaceSwapperModule-->>Thread1: return FACE_SWAPPER
Flow diagram for get_face_swapper initialization, CoreML config, and warmupflowchart TD
A[Call get_face_swapper] --> B{FACE_SWAPPER is None?}
B -- No --> Z[Return existing FACE_SWAPPER]
B -- Yes --> C[Acquire THREAD_LOCK]
C --> D{FACE_SWAPPER is None after lock?}
D -- No --> Y[Release THREAD_LOCK]
Y --> Z
D -- Yes --> E[Select model_name based on execution_providers]
E --> F[Build providers_config]
F --> G[Call insightface.model_zoo.get_model]
G --> H{Model load successful?}
H -- No --> I[update_status Error loading face swapper model]
I --> J[Set FACE_SWAPPER to None]
J --> K[Release THREAD_LOCK]
K --> L[Return None]
H -- Yes --> M[Assign FACE_SWAPPER]
M --> N[update_status Face swapper model loaded successfully]
N --> O{Any CoreMLExecutionProvider in providers_config?}
O -- No --> P[Release THREAD_LOCK]
P --> Z
O -- Yes --> Q[Prepare warmup input from session.get_inputs]
Q --> R[session.run with warmup input]
R --> S[update_status CoreML warmup inference complete]
S --> P
R -->|Exception| T[update_status CoreML warmup skipped non fatal]
T --> P
Z[Return FACE_SWAPPER]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The new double-checked locking block unconditionally sets
model_name = "inswapper_128_fp16.onnx"and removes the previousCUDAExecutionProvidercheck and defaultinswapper_128.onnxfallback, which changes model selection behavior; if that’s not intentional, restore the conditional model choice inside the locked section. - You changed the CoreML provider config from
"RequireStaticShapes": 0to1; if this wasn’t explicitly required, consider whether this might break dynamic shape inputs on some platforms and, if necessary, gate it behind a capability check or a separate configuration flag.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new double-checked locking block unconditionally sets `model_name = "inswapper_128_fp16.onnx"` and removes the previous `CUDAExecutionProvider` check and default `inswapper_128.onnx` fallback, which changes model selection behavior; if that’s not intentional, restore the conditional model choice inside the locked section.
- You changed the CoreML provider config from `"RequireStaticShapes": 0` to `1`; if this wasn’t explicitly required, consider whether this might break dynamic shape inputs on some platforms and, if necessary, gate it behind a capability check or a separate configuration flag.
## Individual Comments
### Comment 1
<location> `modules/processors/frame/face_swapper.py:89` </location>
<code_context>
+ if FACE_SWAPPER is None:
+ with THREAD_LOCK:
+ if FACE_SWAPPER is None:
model_name = "inswapper_128_fp16.onnx"
- model_path = os.path.join(models_dir, model_name)
- update_status(f"Loading face swapper model from: {model_path}", NAME)
</code_context>
<issue_to_address>
**issue (bug_risk):** Model selection now always uses the fp16 variant and no longer respects non-CUDA providers.
With this change, `model_name` is now always set to `"inswapper_128_fp16.onnx"` whenever `FACE_SWAPPER` is initialized, regardless of the available execution providers. This can break or degrade behavior on providers without good fp16 support (e.g., CPU, some CoreML configs). Please restore conditional selection based on `modules.globals.execution_providers`, or gate fp16 behind an explicit capability check.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| if FACE_SWAPPER is None: | ||
| with THREAD_LOCK: | ||
| if FACE_SWAPPER is None: | ||
| model_name = "inswapper_128_fp16.onnx" |
There was a problem hiding this comment.
issue (bug_risk): Model selection now always uses the fp16 variant and no longer respects non-CUDA providers.
With this change, model_name is now always set to "inswapper_128_fp16.onnx" whenever FACE_SWAPPER is initialized, regardless of the available execution providers. This can break or degrade behavior on providers without good fp16 support (e.g., CPU, some CoreML configs). Please restore conditional selection based on modules.globals.execution_providers, or gate fp16 behind an explicit capability check.
Summary
get_face_swapper()— checkFACE_SWAPPER is Nonebefore acquiring lock to reduce contention in multi-threaded live modeRequireStaticShapes=1for CoreML provider (required for compute plan caching; previously was 0)inswapper_128_fp16.onnxunconditionally for all providers (previously fp16 was CUDA-only; fp16 works correctly on all providers and reduces memory usage)Test plan
Generated with Claude Code