[cuda.compute]: Fix windows race related to get nvrtc type name - #10049
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughChangesWindows builds now serialize NVRTC synchronization
Suggested reviewers: Comment |
| // policy). Serialize every call through one process-wide mutex; the `inline` | ||
| // variable is a single instance shared across all `cccl_type_name_from_nvrtc<T>` | ||
| // instantiations and translation units. | ||
| inline std::mutex nvrtc_type_name_mutex; |
There was a problem hiding this comment.
Any reason not to just make this a static inside the function? Putting it at bare global scope is also potentially dangerous because std::mutex can throw on construction and opens us up to the static initialization ordering fiasco
There was a problem hiding this comment.
cccl_type_name_from_nvrtc is a template, so a static inside it would be instantiated once per T. This is a problem because e.g. device_reduce_policy and device_transform_policy would get separate mutexes and wouldn't serialize against each other. Since the race is cross-type (a transform build and a reduce build both calling nvrtcGetTypeName, which funnels into the process-global, single-threaded DbgHelp UnDecorateSymbolName), we need one shared instance.
std::mutex can throw on construction and opens us up to the static initialization ordering fiasco
std::mutex's default ctor is constexpr + noexcept, so a namespace-scope instance is constant-initialized rather than dynamically initialized, so it's not subject to the static-init-order fiasco and can't throw at construction (nothing references it during static init anyway).
Would you prefer something like
inline std::mutex& nvrtc_type_name_mutex()
{
static std::mutex m;
return m;
}
There was a problem hiding this comment.
I have no concerns here. A global mutex variable is fine I think.
There was a problem hiding this comment.
Ah yes missed the fact it was templated. And it is strange they made the constructor constexpr (I didn't know this), I don't believe any other member in mutex is constexpr so it is functionally useless for it to be constexpr...
🥳 CI Workflow Results🟩 Finished in 1h 21m: Pass: 100%/20 | Total: 5h 43m | Max: 49m 12s | Hits: 82%/2043See results here. |
3 participants
This was discovered in CI in #9887