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

P0660R10 <stop_token> And jthread #32

Closed
StephanTLavavej opened this issue Sep 6, 2019 · 8 comments · Fixed by #1196
Closed

P0660R10 <stop_token> And jthread #32

StephanTLavavej opened this issue Sep 6, 2019 · 8 comments · Fixed by #1196
Assignees
Labels
cxx20 C++20 feature fixed Something works now, yay!
Milestone

Comments

@StephanTLavavej
Copy link
Member

StephanTLavavej commented Sep 6, 2019

P0660R10 <stop_token> And jthread

P1869R1 Renaming condition_variable_any's Interruptible Wait Methods

LWG-3254 Strike stop_token's operator!=
LWG-3362 Strike stop_source's operator!=

The IDE's list of extensionless headers has been updated for VS 2019 16.5.

Feature-test macro as of WG21-N4842:
#define __cpp_lib_jthread 201911L

@StephanTLavavej StephanTLavavej added the cxx20 C++20 feature label Sep 6, 2019
@NathanSWard
Copy link
Contributor

NathanSWard commented Oct 25, 2019

This is another issue I'd be very willing to implement! This one is definitely more involved than the other issues I've addressed and will require more involved testing as well. I'm willing to help implement this and provide the tests if this issue is open to work on?

@CaseyCarter
Copy link
Member

CaseyCarter commented Oct 25, 2019

My first plan of attack would be to reference Nicolai Josuttis's implementation and build from there.

I caution you not to examine or refer to Nico's implementation: the licensing is unclear. There is a clear license statement for the "code examples" - Nico doesn't want people stealing his book material! - but I can't determine what license applies to any non "code examples". (It's not terribly clear what is or is not a "code example.")

@NathanSWard
Copy link
Contributor

NathanSWard commented Oct 25, 2019

Would you then by chance have any recommendations, or should I just build upon the current std::thread implementation?

@CaseyCarter
Copy link
Member

or should I just build upon the current std::thread implementation?

I'd start by deriving jthread privately from thread, and work though the proposal from there. jthread without <stop_token> should be trivial. <stop_token> will make things much more exciting. (I suspect this will work better as a Draft PR with lots of communication than if you go if and try to finish everything on your own.)

@jbrezina
Copy link

Hi guys, what is the current state of this issue? @NathanSWard are you still working on this?
Would it be possible to start with the implementation in cpprest repository? @StephanTLavavej @BillyONeal
pplx::cancellation_token

@NathanSWard
Copy link
Contributor

Hi @jbrezina
Sorry I've been MIA for a while, but now that school is done with for the semester, I've actually been able to work on this a little bit and was actually planning on reopening the PR later today/tomorrow! I have a more-or-less functioning implementation (minus the cond_var side of the paper). Though I'd gladly take any feed back and if you want to help improve my code I'd gladly do joint development on this!

@NathanSWard
Copy link
Contributor

Update:
@jbrezina @StephanTLavavej @BillyONeal
Upon working on my jthread implementation that I initially set up last year, I realized that it is quite heavily based on Nicolai Josuttis's and after rereading my previous conversation with @CaseyCarter in this thread, I don't know if my branch will be able to be merged due to the licensing around Nicolai's implementation and my own. Here is the link to my branch on my forked STL repo, if interested. In light of the licensing dilemma, @jbrezina you are more than welcome to work on <stop_token>/thread implementation!

@StephanTLavavej
Copy link
Member Author

Thanks - yeah, we need to be careful about licensing. Code written from scratch, Apache 2.0 with LLVM exception, and Boost are all ok. For anything else, we need to look into it ahead of time, to avoid accumulating work that can't be merged.

@cbezault cbezault added this to the Conformance milestone May 7, 2020
BillyONeal added a commit to BillyONeal/STL that referenced this issue Aug 14, 2020
Implements P0660R10, P1869R1, resolves microsoftGH-32, and VSO-951574.

<atomic> / <memory>: Extract a new type _Locked_pointer which implements the low order bit tricks for atomic<shared_ptr> as a reusable component.

yvals_core.h / *_feature_test_macros/test.cpp: Add feature test macro __cpp_lib_jthread and note that the feature is implemented.

CMakeLists.txt: Add stop_token header. This will require copy_crt / setup changes in msvc.

`<stop_token>` : New header. FIXME This needs a writeup after review.

_Stop_callback_base forms a doubly-linked list of callbacks to invoke when stop is requested. It uses a function pointer rather than a virtual function for reduced RTTI codegen costs and to save an indirection of a vtbl access; the compiler being able to devirtualize this case is unlikely. (Using a function pointer rather than virtual was suggested by @lewissbaker )

_Stop_state is the shared state acting as the 'pipe' between stop sources and stop tokens. It has separate reference counts for sources and tokens, and all sources share 1 token count in the same way that all shared_ptrs share one weak count. The stop requested bit is merged into the sources counter to make atomically answering stop_possible possible without tricks. The counters are separate uint32_ts instead of one uint64_t to make operations manipulating them more efficient on 32 bit platforms, even though that means we might have to do a 1-2 more atomic ops on 64 bit platforms in some cases.

The tricky part here is primarily how we implement the dtor effects for stop_callback. We can't use a straightforward bool in the stop_callback indicating whether the callback has been called, because once we invoke the callback the memory on which the stop_callback is stored might be gone. To resolve this, we have the thread processing request_stop publish the callback it is currently retiring, so a wait_callback destructor, upon seeing that another thread is executing the callback, can wait until the currently being executed callback is something else.

`<thread>`:
Extract the thread startup machinery from thread's ctor; notably, this introduces another layer of forwards in the constructing thread, but doesn't introduce any additional stack frames in the thread procedure, preserving the debugger experience.

Implement jthread with a member thread and stop_source. Note that the standard currently makes the move assignment operator join on self move assign which is surprising; I asked L(E)WG if that is intentional.

`<condition_variable>`:
Implement the new interruptable waits; this code is effectively copied from the standard. I filed a defect report because [thread.condvarany.intwait]/7 depicts a call to a nonexistent variable `cv`.
BillyONeal added a commit to BillyONeal/STL that referenced this issue Aug 26, 2020
Implements P0660R10, P1869R1, resolves microsoftGH-32, and VSO-951574.

<atomic> / <memory>: Extract a new type _Locked_pointer which implements the low order bit tricks for atomic<shared_ptr> as a reusable component.

yvals_core.h / *_feature_test_macros/test.cpp: Add feature test macro __cpp_lib_jthread and note that the feature is implemented.

CMakeLists.txt: Add stop_token header. This will require copy_crt / setup changes in msvc.

`<stop_token>` : New header. FIXME This needs a writeup after review.

_Stop_callback_base forms a doubly-linked list of callbacks to invoke when stop is requested. It uses a function pointer rather than a virtual function for reduced RTTI codegen costs and to save an indirection of a vtbl access; the compiler being able to devirtualize this case is unlikely. (Using a function pointer rather than virtual was suggested by @lewissbaker )

_Stop_state is the shared state acting as the 'pipe' between stop sources and stop tokens. It has separate reference counts for sources and tokens, and all sources share 1 token count in the same way that all shared_ptrs share one weak count. The stop requested bit is merged into the sources counter to make atomically answering stop_possible possible without tricks. The counters are separate uint32_ts instead of one uint64_t to make operations manipulating them more efficient on 32 bit platforms, even though that means we might have to do a 1-2 more atomic ops on 64 bit platforms in some cases.

The tricky part here is primarily how we implement the dtor effects for stop_callback. We can't use a straightforward bool in the stop_callback indicating whether the callback has been called, because once we invoke the callback the memory on which the stop_callback is stored might be gone. To resolve this, we have the thread processing request_stop publish the callback it is currently retiring, so a wait_callback destructor, upon seeing that another thread is executing the callback, can wait until the currently being executed callback is something else.

`<thread>`:
Extract the thread startup machinery from thread's ctor; notably, this introduces another layer of forwards in the constructing thread, but doesn't introduce any additional stack frames in the thread procedure, preserving the debugger experience.

Implement jthread with a member thread and stop_source. Note that the standard currently makes the move assignment operator join on self move assign which is surprising; I asked L(E)WG if that is intentional.

`<condition_variable>`:
Implement the new interruptable waits; this code is effectively copied from the standard. I filed a defect report because [thread.condvarany.intwait]/7 depicts a call to a nonexistent variable `cv`.
@StephanTLavavej StephanTLavavej added fixed Something works now, yay! and removed work in progress labels Sep 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cxx20 C++20 feature fixed Something works now, yay!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants