This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Utilities for implementing stop_token
ClosedPublic

Authored by huixie90 on May 9 2023, 8:51 AM.

Details

Reviewers
ldionne
Group Reviewers
Restricted Project
Commits
rG544e38ca439f: [libc++] Utilities for implementing stop_token
Summary

This change contains three util classes that were out from D145183 to make incremental progress

  • automic_unique_lock
  • intrusive_list
  • intrusive_shared_ptr

Diff Detail

Event Timeline

huixie90 created this revision.May 9 2023, 8:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 9 2023, 8:51 AM
huixie90 updated this revision to Diff 520788.May 9 2023, 12:15 PM

add tests

huixie90 published this revision for review.May 12 2023, 3:23 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 12 2023, 3:23 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne accepted this revision.May 12 2023, 8:59 AM
ldionne added a subscriber: ldionne.

I really like this! Thanks a lot for splitting this into another review, I think this makes the stop_token code much easier to understand. LGTM with some comments.

libcxx/include/__stop_token/atomic_unique_lock.h
24–28

Nit: let's use a c++ style comment

libcxx/include/__stop_token/intrusive_list.h
23 ↗(On Diff #521583)

Note for yourself: you think it might be possible to instead own the elements of the list and simplify some stuff in stop_callback?

29 ↗(On Diff #521583)

Maybe a short comment explaining what the class does? Mention that it doesn't own the nodes.

31 ↗(On Diff #521583)

Currently, the list can be copied or copy-assigned. I think this is a bit misleading, since the list doesn't really own the elements. Normally, we expect that two copies of an object are independent, which is not the case here since it's not a value type. In a way, this is some kind of "view" like string_view. The "structure" created by the list elements exists regardless of whether we have an __intrusive_list object alive somewhere, and the __intrusive_list only gives us an API to manipulate it along with a handle to the first element in that "structure".

So I would suggest one of two things:

  1. Make it non-copyable, and use
__intrusive_list() = default;
__intrusive_list(__intrusive_list const&) = delete;
__intrusive_list(__intrusive_list&&) = default;
__intrusive_list& operator=(__intrusive_list const&) = delete;
__intrusive_list& operator=(__intrusive_list&&) = default;
  1. Change the name slightly to something like __intrusive_list_view (??) to make it clearer that it's a view. Along with a comment explaining the design of the class, I think this can go a long way. Then you probably also want to make the special members explicit like above with = default for everything, just to call it out. And finally, if you go for that approach, don't forget to update file names, header guards and the test!

Edit: Since the destructor doesn't destroy the elements in the list, I think we now agree that (2) is probably the way to go.

libcxx/include/__stop_token/intrusive_shared_ptr.h
29–33
libcxx/test/libcxx/thread/thread.stoptoken/atomic_unique_lock.pass.cpp
26

Can you also test when the lock bit is not the first one? It should be easy to do something like:

template <uint8_t LockBit>
void test() {
  // all the stuff
}

int main(...) {
  test<1>();
  test<2>();
  test<3>();
  // etc..
}
This revision is now accepted and ready to land.May 12 2023, 8:59 AM
huixie90 updated this revision to Diff 521730.May 12 2023, 11:19 AM
huixie90 marked 6 inline comments as done.

address comments

ldionne accepted this revision.May 15 2023, 11:39 AM

Thanks, nice!

This revision was automatically updated to reflect the committed changes.