GH-50428: [C++] Better mimalloc configuration on macOS#50549
Conversation
|
@github-actions crossbow submit wheelmacos* |
|
|
This comment was marked as outdated.
This comment was marked as outdated.
| -DMI_LOCAL_DYNAMIC_TLS=ON | ||
| # GH-50428: Make sure several mimalloc instances can cohabit in the same process | ||
| # (also https://github.com/microsoft/mimalloc/issues/1327#issuecomment-4964140817) | ||
| -DMI_TLS_MODEL_THREAD_LOCAL=ON |
There was a problem hiding this comment.
Shouldn't this be MI_TLS_MODEL_LOCAL?
https://github.com/microsoft/mimalloc/blob/477c1d71c75a92255b3bdffb6f9dcae0c31d21c8/CMakeLists.txt#L59
Defining MI_TLS_MODEL_THREAD_LOCAL seems to be no-op:
https://github.com/microsoft/mimalloc/blob/477c1d71c75a92255b3bdffb6f9dcae0c31d21c8/CMakeLists.txt#L445-L456
There was a problem hiding this comment.
On the built wheel logs I can see that MI_TLS_MODEL_THREAD_LOCAL don't seem to be used:
CMake Warning:
Manually-specified variables were not used by the project:
BUILD_STATIC_LIBS
BUILD_TESTING
CMAKE_CXX_COMPILER
CMAKE_CXX_FLAGS
CMAKE_CXX_FLAGS_DEBUG
CMAKE_CXX_FLAGS_MISIZEREL
CMAKE_CXX_FLAGS_RELEASE
CMAKE_CXX_FLAGS_RELWITHDEBINFO
CMAKE_C_FLAGS_MISIZEREL
CMAKE_POLICY_VERSION_MINIMUM
MI_TLS_MODEL_THREAD_LOCAL
There was a problem hiding this comment.
Ouch, you're right, I was blindly copying the suggestion from the mimalloc issue.
github-actions
Bot
added
awaiting changes
github-actions
Bot
added
awaiting change review
|
@github-actions crossbow submit wheelmacos* |
This comment was marked as outdated.
This comment was marked as outdated.
|
Silly me: the option I'm adding was only added in mimalloc 3.4.0 🤦 |
I am trying to bump here: But there are several CI failures which I haven't diagnosed yet, do you want me to try this change there? |
|
I've pushed the 3.4.1 change here and we'll see... |
|
@github-actions crossbow submit wheelmacoscp314 |
|
Revision: 0bf57cb Submitted crossbow builds: ursacomputing/crossbow @ actions-ea3f09b433
|
|
@github-actions crossbow submit wheelmacos* |
|
Revision: c5a439d Submitted crossbow builds: ursacomputing/crossbow @ actions-e31f2f43ac
|
|
@github-actions crossbow submit wheelcp314* |
|
Revision: c5a439d Submitted crossbow builds: ursacomputing/crossbow @ actions-bf2306637b
|
|
The combination of the version bump + the new TLS model option seems to work fine. @raulcd |
| set(MIMALLOC_CMAKE_ARGS | ||
| ${MIMALLOC_CMAKE_ARGS} |
There was a problem hiding this comment.
| set(MIMALLOC_CMAKE_ARGS | |
| ${MIMALLOC_CMAKE_ARGS} | |
| list(APPEND MIMALLOC_CMAKE_ARGS |
github-actions
Bot
added
awaiting changes
3 participants
Rationale for this change
On macOS, two independently-linked mimalloc v3 instances built with default TLS settings can end up using the same hard-coded TLS slots and crash due to conflicting expectations. See upstream issue at microsoft/mimalloc#1327
This can manifest when PyArrow is loaded side-by-side with another Python extension module that bundles its own instance of mimalloc.
What changes are included in this PR?
Configure macOS to use C thread-local variables for thread-local storage, avoiding conflicting accesses to hard-coded TLS slots on macOS.
Also, unrelatedly, make sure the default malloc is not overriden by our mimalloc build on macOS.
Are these changes tested?
By existing CI jobs.
Are there any user-facing changes?
No, just a bugfix.