Skip to content

Replace Win32 PAL implementation with minipal implementation #115858

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 42 commits into from
May 29, 2025

Conversation

AaronRobinsonMSFT
Copy link
Member

@AaronRobinsonMSFT AaronRobinsonMSFT commented May 22, 2025

This PR removes the PAL Win32 emulation of CRITICAL_SECTION. The emulation logic is unnecessarily complicated and used both a pthread mutex and condition variable. For CoreCLR purposes a mutex is sufficient.

A new minipal API was introduced, minipal_critsect. This uses the Win32 CRITICAL_SECTION on the Windows platform and pthread_mutex_t on all other platforms. This implementation aligns with a private "critical section" defined for the GC and native AOT. All now use this new API.

The CRITICAL_SECTION associated APIs and tests in the CoreCLR PAL have been removed.

@huoyaoyuan
Copy link
Member

FWIW, I tested std::mutex and mtx_t on Windows. std::mutex is about the same speed of CRITICAL_SECTION, and mtx_t is 2x slower. We are unable to use std::mutex because it's not noexcept.

Remove any non-running tests requiring CRITICAL_SECTION
Remove unused PAL header referencing CRITICAL_SECTION (internal.h)
@AaronRobinsonMSFT
Copy link
Member Author

AaronRobinsonMSFT commented May 22, 2025

FWIW, I tested std::mutex and mtx_t on Windows. std::mutex is about the same speed of CRITICAL_SECTION, and mtx_t is 2x slower. We are unable to use std::mutex because it's not noexcept.

It isn't just because of noexcept. On Windows std::mutex will pull in the concrt, which is a huge dependency.

mtx_t

clang on macOS doesn't support C11 threads. It isn't an option in any case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants