Skip to content

Conversation

ghost
Copy link

@ghost ghost commented Oct 12, 2024

The bson_sync_synchronize() API, a basic platform-specific wrapper around an unspecific memory fence, was added back in 2013 and used briefly for mongoc-counters. That use has since been replaced by bson_atomic_thread_fence().

We don't know of any other users of this API but it was technically exposed as public, so third party code may still rely on it appearing in bson-compat.h.

We plan to remove atomics from the API in 2.0, see:
#1762

To align bson_sync_synchronize() with our plans, this patch replaces the original macro definition with an inline function that's equivalent to bson_atomic_thread_fence(), and this function is marked as deprecated. We will remove the function entirely for 2.0, along with bson-atomic.h.

Manually tested that the expected deprecation appears when using bson_sync_synchronize() from external code. Included a trivial automated test which can't verify the deprecation warning or the memory fence functionality, but it will catch compile failures or crashes.

@ghost ghost requested a review from eramongodb October 14, 2024 18:47
bson_sync_synchronize (void)
{
BSON_IF_MSVC (MemoryBarrier ();)
BSON_IF_GNU_LIKE (__sync_synchronize ();)
Copy link
Contributor

Choose a reason for hiding this comment

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

CC @vector-of-bool, requesting extra eyes to confirm this behavior is (sufficiently) equivalent to the former definition of bson_sync_synchronize given your prior work on bson-atomic.h. I think this should be acceptable (including platform support/compatibility) + I am in favor of avoiding/removing inline ASM where we can. Related GCC 4.8.4 documentation:

__sync_synchronize (...): This built-in function issues a full memory barrier.

Copy link
Author

Choose a reason for hiding this comment

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

The asm shouldn't be applicable on any supported platforms. Both the before and after implementations use __sync_synchronize() on gcc. I think in practice the biggest difference might be that unoptimized builds may see a function call now whereas previously that was impossible.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think @mdbmes is correct, and the inline asm statements are no longer expanding on any supported platforms. I believe this change is good.

Copy link
Contributor

@vector-of-bool vector-of-bool left a comment

Choose a reason for hiding this comment

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

LGTM

@ghost ghost merged commit 71475df into mongodb:master Oct 18, 2024
42 of 44 checks passed
@ghost ghost deleted the CDRIVER-4220 branch October 18, 2024 17:58
This pull request was closed.
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.

2 participants