Skip to content

Conversation

@Reneg973
Copy link

Fixes # (issue)

Changes

  • load() not required in try_lock()
  • shortened loop in lock()
  • fixed erroneous yield() in BM_ThreadYieldSpinLockThrashing
  • added std::mutex to benchmark
  • removed superfluous static as code is already in unnamed namespace

It seems that at least on Apple M1 (lClang+libc) the std::mutex outperforms SpinlockMutex with higher number of threads.

@Reneg973 Reneg973 requested a review from a team as a code owner October 19, 2025 21:16
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Oct 19, 2025

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: lalitb / name: Lalit Kumar Bhasin (61f60a7)
  • ✅ login: Reneg973 / name: Rene Greiner (0fac0b9, 5bfa70d)

@lalitb lalitb requested a review from Copilot October 20, 2025 17:39
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the SpinLockMutex implementation and its benchmark suite to improve code clarity and performance testing. The changes simplify the locking logic and expand benchmark coverage to include std::mutex for performance comparison.

Key changes:

  • Simplified try_lock() by removing redundant load() check before exchange operation
  • Refactored lock() to use try_lock() internally for cleaner code
  • Fixed erroneous yield logic in BM_ThreadYieldSpinLockThrashing benchmark
  • Added std::mutex benchmark for performance comparison across different thread counts

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
api/include/opentelemetry/common/spin_lock_mutex.h Simplified try_lock() and lock() implementations by removing redundant checks and restructuring loop logic
api/test/common/spinlock_benchmark.cc Refactored benchmark template to use lambdas with captures, removed static qualifiers, fixed yield bug, and added std::mutex benchmark

Comment on lines +98 to 102
while (!try_lock())
{
// Try once
if (!flag_.exchange(true, std::memory_order_acquire))
{
return;
}
// Spin-Fast (goal ~10ns)
for (std::size_t i = 0; i < SPINLOCK_FAST_ITERATIONS; ++i)
{
Copy link

Copilot AI Oct 20, 2025

Choose a reason for hiding this comment

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

The spin-fast loop at line 101 is now unreachable. The while (!try_lock()) loop continuously retries try_lock() without ever entering the body to execute the spin-fast logic. The original structure with explicit if (!flag_.exchange(...)) return; allowed falling through to the spin logic when the lock failed. This breaks the intended backoff behavior.

Copilot uses AI. Check for mistakes.
std::atomic_flag mutex{};
#else
std::atomic_flag mutex = ATOMIC_FLAG_INIT;
alignas(8) std::atomic_flag mutex = ATOMIC_FLAG_INIT;
Copy link

Copilot AI Oct 20, 2025

Choose a reason for hiding this comment

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

[nitpick] The alignas(8) specifier appears to be added without explanation. If alignment is needed for performance, consider adding a comment explaining why 8-byte alignment is chosen, or remove it if it was added unintentionally.

Suggested change
alignas(8) std::atomic_flag mutex = ATOMIC_FLAG_INIT;
std::atomic_flag mutex = ATOMIC_FLAG_INIT;

Copilot uses AI. Check for mistakes.
{
for (;;)
// Try once
while (!try_lock())
Copy link
Member

Choose a reason for hiding this comment

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

This seems less efficient under contention. Calling try_lock() in every iteration causes repeated atomic exchanges and cache invalidations. The previous version avoided that by doing the initial exchange outside the spin loop.

@codecov
Copy link

codecov bot commented Oct 20, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.95%. Comparing base (70daba9) to head (5bfa70d).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3706      +/-   ##
==========================================
+ Coverage   89.90%   89.95%   +0.06%     
==========================================
  Files         225      225              
  Lines        7281     7101     -180     
==========================================
- Hits         6545     6387     -158     
+ Misses        736      714      -22     
Files with missing lines Coverage Δ
api/include/opentelemetry/common/spin_lock_mutex.h 100.00% <100.00%> (ø)

... and 47 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

{
return !flag_.load(std::memory_order_relaxed) &&
!flag_.exchange(true, std::memory_order_acquire);
return !flag_.exchange(true, std::memory_order_acquire);

Choose a reason for hiding this comment

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

How is this more efficient? load(std::memory_order_relaxed) would avoid having a memory fence and wait for cache sync. Having said that the whole idea of SpinLock in user mode seems dubious especially in high contention scenarios.

@Reneg973
Copy link
Author

I'm confused why this CI/Format check fails! Any hint is welcome

@marcalff
Copy link
Member

@Reneg973

Tip to format all the code (C++, CMake, Bazel):

[malff@malff-desktop ~]$ more ~/bin/otel_cf.sh 
#!/bin/bash
podman run --rm --privileged=true --volume ${PWD}:/otel otel/cpp_format_tools

See https://github.com/open-telemetry/cpp-build-tools/tree/main/cpp_format_tools

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants