[libcxx-commits] [PATCH] D145183: [libc++] Implement `stop_token`

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added a comment.
This revision is now accepted and ready to land.

I am extremely happy with the state of this patch! Thanks so much for all the iterations, I feel like we really have something good with great test coverage now. I'm especially happy about how things ended up after intrusive shared ptr and other generic bits were extracted, I think the code is really neat now.

Once the remaining comments have been addressed and the CI is passing, feel free to ship this. Thank you!



================
Comment at: libcxx/include/__stop_token/stop_callback.h:42
+public:
+  using callback_type = _Callback;
+
----------------
Do you have a test for this typedef?


================
Comment at: libcxx/include/__stop_token/stop_callback.h:84
+        __state_() {
+    if (__state) {
+      if (__state->__add_callback(this)) {
----------------
ldionne wrote:
> I might combine these two ifs into `if (__state && __state->__add_callback(this))`.
Do you have a test for the fact that the `stop_callback` won't get shared ownership of the `__stop_state` if the constructor runs the callback immediately (aka if a stop has been requested at the moment of calling this constructor).

Edit: I don't think there's a way to test that without poking into the library internals -- IOW I'm not sure this is user-observable.


================
Comment at: libcxx/include/__stop_token/stop_callback.h:84-85
+        __state_() {
+    if (__state) {
+      if (__state->__add_callback(this)) {
+        // st.stop_requested() was false and this is successfully added to the linked list
----------------
I might combine these two ifs into `if (__state && __state->__add_callback(this))`.


================
Comment at: libcxx/include/__stop_token/stop_source.h:28
+struct nostopstate_t {
+  explicit nostopstate_t() = default;
+};
----------------
Do you have a test for this? We need to make sure that:

1. It exists
2. The constructor is explicit
3. Maybe that it's empty and trivially default constructible?
4. That `std::nostopstate` (the variable, not the class) exists and is constexpr.


================
Comment at: libcxx/include/__stop_token/stop_state.h:1
+// -*- C++ -*-
+//===----------------------------------------------------------------------===//
----------------
Recording the discussion: We considered whether we should move some of these classes to the dylib, and I think the answer should be no. There isn't *that much* code, it's not platform-specific (e.g. no including `windows.h`), and it would add backdeployment requirements. All in all, I don't think there's enough bang for the buck.


================
Comment at: libcxx/include/__stop_token/stop_state.h:56
+  // When the counter reaches zero, the state is destroyed
+  // It is used by __ref_counted_stop_state, but it is stored here for better layout
+  std::atomic<uint32_t> __ref_count_ = 0;
----------------



================
Comment at: libcxx/include/__stop_token/stop_state.h:70
+  _LIBCPP_HIDE_FROM_ABI void __increment_stop_source_counter() noexcept {
+    __state_.fetch_add(1 << __stop_source_counter_shift, std::memory_order_relaxed);
+  }
----------------
Could we add a `_LIBCPP_ASSERT` here to make sure that we don't overflow? Same below for the decrement: if we decrement when we are already at 0, I guess it means that there's a lifetime bug in the program somewhere and someone's using a `stop_source` that has already been destroyed. Since that's easy to catch, I think we should add those assertions.


================
Comment at: libcxx/include/__stop_token/stop_state.h:109
+      bool __destroyed   = false;
+      __cb->__destroyed_ = &__destroyed;
+
----------------
Taking note of what you explained just now:

Since there's only one thread looping through the list and executing callbacks, it should be possible to store a single pointer to the currently-executing callback in the stop state. That would avoid requiring a `__completed_` and a `__destroyed_` member in the callback. Then in `__remove_callback`, we would atomic-wait on the currently-executing callback not being `this`.

I would suggest that we ship this patch with the current approach, and we investigate this improvement in a separate patch.


================
Comment at: libcxx/include/__stop_token/stop_state.h:175
+        // The destructor of stop_callback runs on the same thread of the thread that invokes the callback.
+        // Potentially the callback is invoking the destuctor of itself. Set the flag to avoid accessing
+        // destroyed members on the invoking side
----------------



================
Comment at: libcxx/include/__stop_token/stop_token.h:52
+
+private:
+  friend class stop_source;
----------------
This `private` is redundant with the one above.


================
Comment at: libcxx/test/std/thread/thread.stoptoken/stopcallback/cons.const.token.pass.cpp:95
+
+    int calledTimes = 0;
+    std::stop_callback sc(st, [&] { ++calledTimes; });
----------------
Don't change anything, but we generally use snake_case for variable names. Just in the future.


================
Comment at: libcxx/test/std/thread/thread.stoptoken/stopsource/cons.copy.pass.cpp:9
+//
+// UNSUPPORTED: no-threads
+// UNSUPPORTED: no-threads
----------------
This is duplicated with the line below (please double-check throughout all your tests).


================
Comment at: libcxx/test/std/thread/thread.stoptoken/stopsource/cons.copy.pass.cpp:12
+// UNSUPPORTED: c++03, c++11, c++14, c++17
+
+// stop_source(const stop_source&) noexcept;
----------------
Here you're missing `// XFAIL: availability-synchronization_library-missing`, that would fix some of your CI.


================
Comment at: libcxx/utils/generate_header_tests.py:1
 #!/usr/bin/env python
 
----------------
To fix your CI issue, I think you need to add `"stop_token": ["UNSUPPORTED: no-threads, availability-synchronization_library-missing"],` to `libcxx/utils/generate_feature_test_macro_components.py`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145183/new/

https://reviews.llvm.org/D145183