Skip to content
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

Clear up a note about condition variables #2647

Merged
merged 2 commits into from Jan 19, 2019
Merged

Conversation

kiroma
Copy link
Contributor

@kiroma kiroma commented Jan 14, 2019

I've been looking far and wide but nowhere in the papers about condition variables have I found note about efficiency of them being platform dependent. The implementation of std::condition_variable_any in libc++ and in libstdc++ is a generalization of std::condition_variable, which points towards the efficiency being bound more to the implementation than the platform.

I've been looking far and wide but nowhere in the papers about condition variables have I found note about efficiency of them being platform dependent. The implementation of `std::condition_variable_any` in libc++ is a generalization of `std::condition_variable`, which points towards the efficiency being bound more to the implementation than the platform.
@jwakely
Copy link
Member

jwakely commented Jan 15, 2019

The efficiency of std::condition_variable is of course platform-dependent, because it depends on the synchronization primitives provided by the OS and hardware. But this change seems to replace one vague, imprecise statement with another.

@jensmaurer
Copy link
Member

I'm mildly in favor of this change, because we usually talk about "implementation" in the C++ standard when we mean anything related to the compiler or OS platform. For example, [fs.conform.9945] consistently talks about "implementations", encompassing both the C++ library and the OS platform. I'm lukewarm about the "maximum" -> "better" change, although relative statements (as opposed to absolute ones) seem more appropriate for a standard document.

@jwakely
Copy link
Member

jwakely commented Jan 15, 2019

Yes, it looks like all the other uses of "platform" are at least in Notes (and although this preamble is only semi-normative, it's not actually in a Note).

@kiroma
Copy link
Contributor Author

kiroma commented Jan 15, 2019

With this PR I wanted to start off some discussion about the issue.
I should clear up that "better efficiency" in this case looks to be relative to std::condition_variable_any.
Maybe instead of std::condition_variable being faster it should be said that std::condition_variable_any can be slower?

@jwakely
Copy link
Member

jwakely commented Jan 15, 2019

Well the spec of std::condition_variable is intended to allow the implementation to achieve maximum performance for a condition variable that supports waiting and notifying, i.e. it shouldn't add any abstraction penalty to the underlying OS primitives. If the implementation fails to achieve maximum performance, that's not the spec's fault 😄

std::condition_variable_any does add additional abstraction, by supporting user-defined locks, and so generally won't be as fast as the OS primitives.

If we're going to change this, I'd also suggest changing "some implementations", as that is meaningless. Which ones? Does it tell a user whether their implementation gets maximum performance?

How about:

Class condition_variable provides a condition variable that can only wait on an object of type unique_lock<mutex>, allowing the implementation to be more efficient. Class condition_variable_any provides a general condition variable that can wait on objects of user-supplied lock types.

@jensmaurer
Copy link
Member

@jwakely, sounds good to me.

@kiroma
Copy link
Contributor Author

kiroma commented Jan 15, 2019

Yep, that sounds much better.

@zygoloid zygoloid merged commit 2fdd7e8 into cplusplus:master Jan 19, 2019
@kiroma kiroma deleted the patch-1 branch January 19, 2019 03:53
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.

None yet

4 participants