Skip to content

Conversation

@ringabout
Copy link
Member

@ringabout ringabout commented Apr 13, 2021

Ref timotheecour#693

close #7998
close #17669

genode OS is still left unchanged, I have no idea.

After CI is green, I will add changelog, tests and since.

It is the basis of concurrency programming.

In the future, Semaphore should be exported or re-implemented.

@ringabout ringabout changed the title close #7998 close #7998(complete condition variables) Apr 13, 2021
@ringabout ringabout changed the title close #7998(complete condition variables) [std/locks]close #7998(complete condition variables) Apr 13, 2021
@ringabout ringabout closed this Apr 14, 2021
@ringabout ringabout reopened this Apr 14, 2021
@ringabout
Copy link
Member Author

ringabout commented Apr 14, 2021

This PR workarounds the broadcast problem on genode, it is fixable in the future PR.

Ref: https://www.microsoft.com/en-us/research/wp-content/uploads/2004/12/ImplementingCVs.pdf

@ringabout

This comment has been minimized.

@ringabout

This comment has been minimized.

@Araq
Copy link
Member

Araq commented Apr 18, 2021

I'm not convinced that "broadcast" is a good design. If the threads check for different conditions then all waked up threads would cause contention on the condition variable's lock afterwards... The fact that its support needed these signification Windows API wrapper changes doesn't make it better. What are the use cases for "broadcast"? I have yet to see a convincing one.

@ringabout
Copy link
Member Author

ringabout commented Apr 18, 2021

For example implementing a barrier: when cond is ready, you need to unblock all threads instead of one or two threads.

https://github.com/boostorg/thread/blob/4abafccff4bdeb4b5ac516ff0c2bc7c0dad8bafb/include/boost/thread/barrier.hpp#L229

The Nim one:
https://github.com/planetis-m/nim-100days/blob/adee939211ecfadb138b4da4a2209c4340ece2da/multithread/pbarrier2.nim#L30

https://stackoverflow.com/questions/9815798/how-to-find-barrier-functions-implementation

cause contention on the condition variable's lock afterwards.

that's true, but we have to wake up all thread.

see http://www.qnx.com/developers/docs/qnxcar2/index.jsp?topic=%2Fcom.qnx.doc.neutrino.getting_started%2Ftopic%2Fs1_procs_Condvar_signal_vs_broadcast.html

So the usecase

The threads are all unique and are each waiting for a very specific condition to occur.

This nicely reflects the second case in our question above ("Why are these threads waiting?"). Since the threads are all waiting on different conditions (thread1() is waiting for x to be less than or equal to 7 or y to be 15, thread2() is waiting for x to be a prime number, and thread3() is waiting for x to be equal to y), we have no choice but to wake them all.

So if tasks are the same, signal should be used; while they are unique, broadcast should be used.

@saem
Copy link
Contributor

saem commented Apr 18, 2021

broadcast provides an optimization opportunity for the underlying threading implementation that wouldn't be possible if one was to instead repeatedly signal. The additional benefit I didn't think of is that designs using broadcast encourage the handling of spurious wakes, which will also occur in situations where signal is used.

The spurious wake issue is documented here in the rather short rationale section ([https://pubs.opengroup.org/onlinepubs/009696699/functions/pthread_cond_broadcast.html])

If the intention with what Nim exposes is to provide more Nim specific semantics that apply across systems, as opposed to lightly wrapping, then I can see why we'd want to think deeply about includingbroadcast. If it is to provide a light wrapper for largely similar semantics and paper over API differences then I'd recommend including it.

@planetis-m
Copy link
Contributor

planetis-m commented Apr 18, 2021

At first I thought I could do without broadcast in the Barrier implementation, thinking each thread would wake the next thread, etc until the last one would maybe signal the thread that waits on the next barrier and cause a spurious wake or just fail to find another suspended thread.

However soon enough, I hit an edge case where some other thread than the last, would wake a thread that is not at the current barrier causing the above logic to fail.

Point is signal is not a replacement for broadcast. Plus on a benchmark I did (with barriers), broadcast was twice as fast as signal (provided that it didn't get stuck).

@planetis-m
Copy link
Contributor

Forgot to add that one could make a barrier implementation using atomics.

@Araq
Copy link
Member

Araq commented Apr 18, 2021

However soon enough, I hit an edge case where some other thread than the last, would wake a thread that is not at the current barrier causing the above logic to fail.

How could this happen if every barrier had its own condition variable?

But ok, let's have "broadcast" then because all the others have it too, Turning the thundering herd problem into an API, what could go wrong.

@planetis-m
Copy link
Contributor

How could this happen if every barrier had its own condition variable?

Sorry to be more precise, I meant the local generation count, they are reusable barriers after all. It fits with the rule xflywind mentioned. This is a better explanation than I can provide.

Turning the thundering herd problem into an API, what could go wrong.

I believe linux solves this with "wait morphing", maybe this is a problem with windows.

@Araq
Copy link
Member

Araq commented Apr 19, 2021

I believe linux solves this with "wait morphing", maybe this is a problem with windows.

Aha! Good to know!

@Araq Araq merged commit dc89b21 into nim-lang:devel Apr 19, 2021
PMunch pushed a commit to PMunch/Nim that referenced this pull request Mar 28, 2022
…g#17711)

* close nim-lang#7998
* workaround genode

* Update lib/system/syslocks.nim
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.

conditional broadcast missing from locks module The condition variables API is incomplete

4 participants