Skip to content

CDRIVER-4227 Add a shared_mutex, and use it when locking in shared_ptr atomic operations #895

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

Conversation

vector-of-bool
Copy link
Contributor

At very high thread counts, locking on the shared_ptr atomic mutex begins to dominate. Using a shared_mutex significantly reduces the time spent in locking code and increases the performance under high contention. It could be improved further with a smarter use of the shared_ptr, but this is a significant basic improvement.

Copy link
Contributor

@chardan chardan left a comment

Choose a reason for hiding this comment

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

+1. This looks good; please consider renaming to something closer to the underlying platform functions, see comments. Excited to see the performance improvements! :-)

*/
typedef struct bson_shared_mutex_t {
BSON_IF_WINDOWS (SRWLOCK native;)
BSON_IF_POSIX (pthread_rwlock_t native;)
Copy link
Contributor

Choose a reason for hiding this comment

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

to consider: Note that pthreads also has a pthread_mutex_t, which makes me wonder if bson_shared_mutex_t is the right name, especially as the platforms also make that distinction. bson_shared_rwlock? "Archibald"?

@kevinAlbs kevinAlbs changed the title Add a shared_mutex, and use it when locking in shared_ptr atomic operations CDRIVER-4227 Add a shared_mutex, and use it when locking in shared_ptr atomic operations Nov 12, 2021
Co-authored-by: Kevin Albertson <[email protected]>
Copy link
Contributor

@eramongodb eramongodb left a comment

Choose a reason for hiding this comment

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

A very nice and welcome addition! 👍

@kevinAlbs kevinAlbs self-requested a review November 12, 2021 22:10
Copy link
Collaborator

@kevinAlbs kevinAlbs left a comment

Choose a reason for hiding this comment

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

LGTM!

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