Skip to content

[libc++] Improve performance of std::atomic_flag on Windows #163524

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.

Sign up for GitHub

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 8 commits into from
Nov 13, 2025
Merged

[libc++] Improve performance of std::atomic_flag on Windows #163524

Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
63ee5da
[libc++] Improve performance of std::atomic_flag on Windows
Oct 15, 2025
4515628
[libc++] Fixed naming, added comments.
Oct 15, 2025
49efee9
[libc++] More naming and comment changes as recommended
Oct 21, 2025
899aed7
[libc++] Corrected loading/unloading of API set
Oct 26, 2025
2586c83
[libc++] Removed assumption that NULL HMODULE value is safe to use
Oct 26, 2025
930b0c4
[libc++] Consolidated logic into helper function
Oct 26, 2025
27b395d
[libc++] Switched to assume API set is already loaded as per MS STL
Nov 2, 2025
b6df5a5
[libc++] Fixed build error and warning
Nov 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 44 additions & 0 deletions libcxx/src/atomic.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,10 @@
// OpenBSD has no indirect syscalls
# define _LIBCPP_FUTEX(...) futex(__VA_ARGS__)

#elif defined(_WIN32)

# include <windows.h>

#else // <- Add other operating systems here

// Baseline needs no new headers
Expand Down Expand Up @@ -101,6 +105,46 @@ static void __libcpp_platform_wake_by_address(__cxx_atomic_contention_t const vo
_umtx_op(const_cast<__cxx_atomic_contention_t*>(__ptr), UMTX_OP_WAKE, __notify_one ? 1 : INT_MAX, nullptr, nullptr);
}

#elif defined(_WIN32)

static void
__libcpp_platform_wait_on_address(__cxx_atomic_contention_t const volatile* __ptr, __cxx_contention_t __val) {
// WaitOnAddress was added in Windows 8 (build 9200)
static auto wait_on_address = reinterpret_cast<BOOL(WINAPI*)(volatile void*, PVOID, SIZE_T, DWORD)>(
GetProcAddress(GetModuleHandleW(L"api-ms-win-core-synch-l1-2-0.dll"), "WaitOnAddress"));
if (wait_on_address != nullptr) {
Copy link
Member

@huixie90 huixie90 Oct 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a way (e.g. a macro) to check if the function is available at compile time? we don't do this sort of check at runtime on other systems

Copy link
Contributor

@Alcaro Alcaro Oct 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't know which OS the resulting binary will be run on, so no, no such thing, except by dropping support for windows 7 completely.

Which may be a reasonable move - ms-stl already did - but that would require a project-wide RFC, not just a mundane-looking PR to an obscure module.

Copy link
Contributor

@Andarwinux Andarwinux Oct 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see #163524 (comment)

libc++ seems to have dropped Windows 7 support (for modern toolchains where _WIN32_WINNT > _WIN32_WINNT_WIN8 ), but avoiding the dependency on synchronization.lib does sound very meaningful.

Copy link
Member

@mstorsjo mstorsjo Oct 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

libc++ seems to have dropped Windows 7 support

That's not true. Libc++ does support Windows 7 - but when building, _WIN32_WINNT indicates the lowest version you require at runtime.

So if your toolchain defaults to a higher version, you need to manually define _WIN32_WINNT to the lowest version you want to support. llvm-mingw builds with _WIN32_WINNT set to Windows 7.

So for things like this, like the example in chrono.cpp, we can choose to either load symbols dynamically, if targeting an older version, or link directly, if we're targeting a new enough version. But in this case, the PR author pointed out that it would require linking in another import library if we'd link directly, which both is extra work on the CMake level, and also translates to extra work for users who statically link in libc++. So as the overhead is near-zero with only loading it the first time it is called, the PR author thought it was simplest to always go with the dynamic loading approach, which sounds reasonable to me.

Copy link
Contributor

@Andarwinux Andarwinux Oct 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not true. Libc++ does support Windows 7 - but when building, _WIN32_WINNT indicates the lowest version you require at runtime.

Yes, that's exactly what I meant. Considering that both MinGW and Windows SDK default to _WIN32_WINNT_WIN10.

Copy link
Member

@mstorsjo mstorsjo Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if they currently aren't using the latest LLVM, I don't think that logic should be extended so that they can't update to the latest one, for things they are working on. Whether the next release is ready for shipping right now or not seems beside the point - as long as that branch is worked on, and is built for a Win7 baseline, IMO.

Copy link
Contributor

@Andarwinux Andarwinux Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, considering that version 4.0 will not be released anytime soon, absolutely NO SUPPORT for it from the VideoLAN Team before its release, and that VideoLAN also provides GCC builds, I suspect that there are likely few or no end users who will be affected by LLVM's decision to drop Windows 7 support. In this case, LLVM's downstream users should perhaps reconsider their platform support policies rather than passing the burden onto LLVM.

Copy link
Contributor

@Alcaro Alcaro Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did we figure out how to get synchronization.a into libc++.a? If no, we need GetProcAddress even on win11, and there is no additional support burden.

Copy link
Contributor

@Andarwinux Andarwinux Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we can rely on llvm-lib, it's very simple, but if we have to be compatible with ar, it becomes very ugly.

if(LIBCXX_ENABLE_STATIC AND MINGW)
find_library(libsynchronization synchronization REQUIRED)
add_custom_command(
    TARGET cxx_static
    POST_BUILD
    COMMAND llvm-lib /out:$<TARGET_FILE:cxx_static> $<TARGET_FILE:cxx_static> ${libsynchronization}
)
endif()

Copy link

@jbkempf jbkempf Nov 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VLC 4.0 will target Windows 7 as minimal, yes.

VLC 4.0 will be LLVM-focused now, this being the default on macOS/iOS/visionOS, Android, and all Windows versions. Only some Linux versions will stay on GCC, because it is the default on some distributions.

wait_on_address(const_cast<__cxx_atomic_contention_t*>(__ptr), &__val, sizeof(__val), INFINITE);
} else {
__libcpp_thread_poll_with_backoff(
[=]() -> bool { return !__cxx_nonatomic_compare_equal(__cxx_atomic_load(__ptr, memory_order_relaxed), __val); },
__libcpp_timed_backoff_policy());
}
}

static void __libcpp_platform_wake_by_address(__cxx_atomic_contention_t const volatile* __ptr, bool __notify_one) {
if (__notify_one) {
// WakeByAddressSingle was added in Windows 8 (build 9200)
static auto wake_by_address_single = reinterpret_cast<void(WINAPI*)(PVOID)>(
GetProcAddress(GetModuleHandleW(L"api-ms-win-core-synch-l1-2-0.dll"), "WakeByAddressSingle"));
if (wake_by_address_single != nullptr) {
wake_by_address_single(const_cast<__cxx_atomic_contention_t*>(__ptr));
} else {
// The fallback implementation of waking does nothing, as the fallback wait implementation just does polling, so
// there's nothing to do here.
}
} else {
// WakeByAddressAll was added in Windows 8 (build 9200)
static auto wake_by_address_all = reinterpret_cast<void(WINAPI*)(PVOID)>(
GetProcAddress(GetModuleHandleW(L"api-ms-win-core-synch-l1-2-0.dll"), "WakeByAddressAll"));
if (wake_by_address_all != nullptr) {
wake_by_address_all(const_cast<__cxx_atomic_contention_t*>(__ptr));
} else {
// The fallback implementation of waking does nothing, as the fallback wait implementation just does polling, so
// there's nothing to do here.
}
}
}

#else // <- Add other operating systems here

// Baseline is just a timed backoff
Expand Down
Loading