This is an archive of the discontinued LLVM Phabricator instance.

Implement syncstream (p0053)
ClosedPublic

Authored by Mordante on Sep 2 2019, 1:58 PM.

Details

Reviewers
mclow.lists
EricWF
ldionne
zoecarver
Group Reviewers
Restricted Project
Commits
rG7cc72a0a2ec2: Implement syncstream (p0053)
Summary

This patch implements std::basic_syncbuf and std::basic_osyncstream as specified in paper p0053r7. For ease of reviewing I am submitting this patch before submitting a patch for std::basic_osyncstream.

Please note, this patch is not 100% complete. I plan on adding more tests (see comments), specifically I plan on adding tests for multithreading and synchronization.

Edit: I decided that it would be far easier for me to keep track of this and make changes that affect both std::basic_syncbuf and std::basic_osyncstream if both were in one patch.

The patch was originally written by @zoecarver

Implements

  • P0053R7 - C++ Synchronized Buffered Ostream
  • LWG-3127 basic_osyncstream::rdbuf needs a const_cast
  • LWG-3334 basic_osyncstream move assignment and destruction calls basic_syncbuf::emit() twice
  • LWG-3570 basic_osyncstream::emit should be an unformatted output function
  • LWG-3867 Should std::basic_osyncstream's move assignment operator be noexcept?

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
mclow.lists added inline comments.Sep 10 2019, 10:38 AM
libcxx/include/syncstream
103

Arrgh. Peter! Two functions with the same name, one returns bool the other void.
Not your problem, though.

zoecarver updated this revision to Diff 219650.Sep 10 2019, 6:41 PM
  • Updated mutex for gaurd_lock so that it corresponds to a streambuf.
  • Added more thread tests.
zoecarver updated this revision to Diff 219653.Sep 10 2019, 8:12 PM
zoecarver marked 5 inline comments as done.
  • update/add to the tests
libcxx/include/syncstream
227

Nothing. If an exception is thrown from emit(), the destructor catches and ignores that exception.

318

I like the verbosity of saying == false. I'll update it, though.

zoecarver updated this revision to Diff 219654.Sep 10 2019, 8:25 PM
  • don't use not keyword
mclow.lists added inline comments.Sep 11 2019, 8:28 AM
libcxx/include/syncstream
202

As a static member variable - when does this get initialized? On first use? before main? Does it get initialized if the user never instantiates a basic_syncbuf?

209

Why unordered_map as opposed to map? Does std::hash< streambuf_type*> have some behavior that you want? [ Note: unordered_map might be the right choice. But you should say why it is the right choice somewhere. ]

280

This adds a default constructed mutex_ptr_type to the map if it doesn't already exist.
I suspect that's not what you wanted.

mclow.lists added inline comments.Sep 11 2019, 8:52 AM
libcxx/include/syncstream
221

How do entries get removed from __mtx_ptr_map?

zoecarver marked an inline comment as done.Sep 11 2019, 9:19 AM
zoecarver added inline comments.
libcxx/include/syncstream
209

The pointer specialization of std::hash should help with memory usage. Unordered map has a find complexity of O(1) which is the real reason I think it should be used. This stream should be CPU performant above all else, with hundreds of threads trying to access an element, I think the benefit of find speed outweighs the slight increase in memory consumption. Do you agree?

mclow.lists added inline comments.Sep 11 2019, 9:57 AM
libcxx/include/syncstream
209

The pointer specialization of std::hash should help with memory usage.

How?

Unordered map has a find complexity of O(1) which is the real reason I think it should be used.

Unordered map has a find complexity of O(1) in the best case. O(N) in the worst case. Which one this?

zoecarver marked an inline comment as done.Sep 11 2019, 6:40 PM
zoecarver added inline comments.
libcxx/include/syncstream
209

Unordered map has a find complexity of O(1) in the best case.

Sorry, yes, that is what I meant. I suspect that I will be closer to O(1) because there won't be very many elements.

I ran some basic benchmarks and came up with very similar results. std::unordered_map seems to be roughly 150ms slower (total time 16675ms vs 16521ms). Given this, I will update to use std::map to shrink the memory footprint.

zoecarver marked 3 inline comments as done.Sep 11 2019, 7:01 PM
zoecarver added inline comments.
libcxx/include/syncstream
202

Yes, because it is initialized out of line, it will be initialized any time this file is included.

I will update this to use an enum and get around the static data member initialization.

221

They don't. I don't think there is any way to say "will any threads ever need to access this streambuf again?"

If there was a way to only keep the streambuf in the static map while the object is active, I would be open to that. But, I am hesitant to do that because I want to keep the read/writes to the map as low as possible.

What would you suggest?

280

I can update this to use contains but, I don't think it matters much. There should never be a scenario where __mtx_ptr_map[__wrapped] doesn't exist. And if there is, then the below code will still pass because mutex_ptr_type == nullptr_t is OK.

zoecarver updated this revision to Diff 220126.Sep 13 2019, 9:26 AM
  • use contains
  • use function level static
  • use map instead of unordered_map
zoecarver updated this revision to Diff 221876.Sep 25 2019, 6:29 PM

Add std::lock_guards to prevent races on the map. I ran with TSAN and got no warnings. I also increased the number of times I run the larger thread tests and now get no errors.

zoecarver marked an inline comment as done.Oct 28 2019, 8:10 AM
zoecarver added inline comments.
libcxx/include/syncstream
221

Given the "new" structure of __mtx_ptr_map, I think I can add a function to remove elements from the map safely. Would this be a good thing to add (the only reason it might not is for performance but, I think stale memory outweighs performance here)?

Ping @mclow.lists can you take another look at this?

ldionne added a reviewer: Restricted Project.Nov 2 2020, 2:26 PM
mclow.lists added inline comments.Dec 15 2020, 7:32 AM
libcxx/include/syncstream
221

You could store a count, like shared_ptr does. How many sync streams are using this streambuf?

Increment the count on construction, decrement on destruction, remove at zero.

Mordante added inline comments.
libcxx/include/iosfwd
103

Can you comment these classes and typedefs have been added in C++20?

libcxx/include/syncstream
53

@mclow.lists Do you know whether these functions should be noexcept? There's a contraction in the standard
http://eel.is/c++draft/syncstream.syncbuf#assign

// [syncstream.syncbuf.assign], assignment and swap
 basic_syncbuf& operator=(basic_syncbuf&&);
 void swap(basic_syncbuf&);

http://eel.is/c++draft/syncstream.syncbuf#assign
basic_syncbuf& operator=(basic_syncbuf&& rhs) noexcept;
void swap(basic_syncbuf& other) noexcept;

The move constructor is not marked as noexcept and neither is emit so I assume [syncstream.syncbuf.assign] is wrong.

227

Minor nit __gaurd -> __guard.

231

Is it intended to use assert in a header?

239

Minor nit __gaurd -> __guard. Please grep for this typo, there's at least one more occurrence.

284

http://eel.is/c++draft/syncstream.syncbuf#assign-1 "Effects : Calls emit()..." I don't see that call.

zoecarver added inline comments.Jan 23 2021, 2:20 PM
libcxx/include/syncstream
53

See https://cplusplus.github.io/LWG/issue3498 I'm going to leave them as not-noexcept until we determine a resolution for this.

231

Fixed. Changed to a _LIBCPP_ASSERT

284

Good catch!

284

I wonder if the same goes for basic_osyncstream& operator=(basic_osyncstream&&) noexcept;. It seems like that operator just isn't specified at all... another LWG issue :P

zoecarver added inline comments.Jan 23 2021, 2:23 PM
libcxx/include/syncstream
53

Also, I just realized, the assignment operator 100% has to be noexcept because it calls emit which is designed to sometimes throw.

zoecarver updated this revision to Diff 318797.Jan 23 2021, 5:25 PM
  • Address review comments
  • Switch back to unordered_map
  • Replace shared_ptr with manual ref count
zoecarver updated this revision to Diff 318801.Jan 23 2021, 5:42 PM
  • Add comments
  • Feature test macros
  • Update www/status docs

Sorry for being so slow to update this patch. I think all review comments have been addressed, and this is now ready for re-review.

cc @ldionne and @mclow.lists.

joe.sylve added inline comments.
libcxx/include/syncstream
289

This move constructor will lose the existing buffered contents. Is this intended? I'd expect that the __str buffer would also be moved and the base class's pointers adjusted.

Mordante commandeered this revision.Sep 5 2023, 11:02 AM
Mordante added a reviewer: zoecarver.
Herald added a project: Restricted Project. · View Herald TranscriptSep 5 2023, 11:02 AM
Mordante edited the summary of this revision. (Show Details)Sep 5 2023, 11:02 AM
Mordante updated this revision to Diff 556228.Sep 7 2023, 10:55 PM
Mordante edited the summary of this revision. (Show Details)

Rebased, updated coding style. fix tests.

Note I still need to do a review of the patch and address the open review comments.
Mainly a test to see the status of the CI.

Mordante updated this revision to Diff 556244.Sep 8 2023, 3:37 AM

CI fixes.

Mordante edited the summary of this revision. (Show Details)Sep 9 2023, 5:47 AM
Mordante marked 10 inline comments as done.Sep 9 2023, 5:54 AM
Mordante added inline comments.
libcxx/include/syncstream
221

I implemented this part and indeed use a count to. Not cleaning up sound like a memory leak which might affect long running applications that create a lot syncbufs.

289

This comment seems no longer be attached to its original location so not sure what you're referring to. I have noticed several issues with the move assignment and constructor.

Mordante updated this revision to Diff 556362.Sep 10 2023, 3:26 AM
Mordante marked an inline comment as done.
Mordante edited the summary of this revision. (Show Details)

Rebased and lots of improvements

  • fixed CI issues
  • fixed modules
  • added nonmember swap
  • fixed member swap signature
  • changed implementation for the mutex addresses a review comment
  • Added more tests to validate the class, manual review found several issues in the overflow function and move operations
  • Testing found more issues, for example in emit
  • Removed a non-needed class member for flusing

Implemented 3 LWG issues

  • 3334. basic_osyncstream move assignment and destruction calls basic_syncbuf::emit() twice
  • 3867. Should std::basic_osyncstream's move assignment operator be noexcept?
  • 3570. basic_osyncstream::emit should be an unformatted output function

Marked one LWG as done

  • 3127. basic_osyncstream::rdbuf needs a const_cast
Mordante updated this revision to Diff 556365.Sep 10 2023, 3:38 AM

Tab -> space

Mordante updated this revision to Diff 556370.Sep 10 2023, 8:52 AM

CI fixes.

Mordante updated this revision to Diff 556411.Sep 11 2023, 3:37 AM

CI fixes.

ldionne added inline comments.Sep 12 2023, 10:07 AM
libcxx/include/syncstream
207

It *might* be possible to use a bit in the locale to implement a lock for all streambufs. Something like

diff --git a/libcxx/include/__locale b/libcxx/include/__locale
index ccac748c44e4..4942808ac2bc 100644
--- a/libcxx/include/__locale
+++ b/libcxx/include/__locale
@@ -131,9 +131,11 @@ private:
     static locale& __global();
     bool has_facet(id&) const;
     const facet* use_facet(id&) const;
+};
 
-    template <class _Facet> friend bool has_facet(const locale&)  _NOEXCEPT;
-    template <class _Facet> friend const _Facet& use_facet(const locale&);
+template <>
+struct __free_bit_traits<locale> {
+    // expose the free bit in `__imp*`
 };
 
 class _LIBCPP_EXPORTED_FROM_ABI locale::facet
diff --git a/libcxx/include/streambuf b/libcxx/include/streambuf
index 095ac7d3ad83..b42fae608a5a 100644
--- a/libcxx/include/streambuf
+++ b/libcxx/include/streambuf
@@ -296,7 +296,9 @@ protected:
     virtual int_type overflow(int_type __c = traits_type::eof());
 
 private:
-    locale __loc_;
+    static_assert(__has_free_bits<locale>);
+    __free_bit_pair<locale, 1> __loc_;
+    // Now implement a lock using that single free bit.
     char_type* __binp_;
     char_type* __ninp_;
     char_type* __einp_;
diff --git a/libcxx/src/locale.cpp b/libcxx/src/locale.cpp
index 59c7ce4d43d6..ade1aa688c4d 100644
--- a/libcxx/src/locale.cpp
+++ b/libcxx/src/locale.cpp
@@ -543,7 +543,7 @@ locale::__imp::make_classic()
     // only one thread can get in here and it only gets in once
     static aligned_storage<sizeof(locale)>::type buf;
     locale* c = reinterpret_cast<locale*>(&buf);
-    c->__locale_ = &make<__imp>(1u);
+    c->__get_locale() = &make<__imp>(1u);
     return *c;
 }

Do you know what other implementations have done? Have they gone through the trouble of finding a lock inside streambufs or do they use a global map?

387

Do we take advantage of what std::string provides (i.e. growth characteristics, SBO, etc..). If we do take advantage of those, then it might be worth using std::string, otherwise I think we might as well use a character buffer we manually manage to save on the duplication of begin/end information.

Mordante added inline comments.Sep 12 2023, 12:36 PM
libcxx/include/syncstream
207

They use "global" maps. As discussed I think it makes more sense not to do this and mark the feature as experimental. Then investigate whether this alternative approach is worth the hassle. I still think we should finish it before LLVM 18.

387

No basically the SBO is "killed" by "str_.resize(str_.capacity() + 1);" This could be smarter and keep the SBO. I'm not sure how useful SBO is. It only helps if all data flushed fits in the SBO area.

Mordante updated this revision to Diff 557562.Oct 3 2023, 8:13 AM

Addresses review comments and updates tests.

ldionne requested changes to this revision.Oct 3 2023, 10:41 AM

Thanks a lot for picking up this patch! I have a bunch of comments but this is starting to look pretty good.

libcxx/include/map
1702–1703

Can you file a Github issue and link it here? In the issue you can say something like "we have a potential use-after-move in map::operator[]" and link to https://godbolt.org/z/e1x9c1h6Y.

libcxx/include/syncstream
158

This seems to be a generic utility to lock addresses. We could extract it from <syncstream> and make it generic, and then reuse it from here in <syncstream>. That way we could potentially reuse this code if we have a need for it in the future.

Or we could also maybe even abstract away the fact that we're containing a mutex inside the value_type. Then this would be a generic map from addresses to some payload.

191

operator[] or .find() maybe? Note that operator[] has an unfortunate precedent of default-constructing the key if it's not present in std::map so we might want to be careful about re-using operator[], but this is worth thinking about.

199

If this class represents a by-address lock, perhaps this should be bool __is_locked(void* __ptr).

207

I think this usage of a function-local static is quite interesting. It's really important to have exactly one instance of this object in the program, otherwise the locking won't work as intended. However, I think it is possible to end up with multiple copies of that object right now:

  1. If we have two TUs that use different versions of libc++, I think we could end up with e.g. __wrapped_streambuf_mutex::__instance[v1800]::__result and __wrapped_streambuf_mutex::__instance[v1900]::__result.
  2. On GCC where we use always_inline, I think this might also end up generating multiple function-local statics but I am not certain about that. The function-local static itself might be linkonce_odr and deduplicated across TUs correctly.

I wonder whether we should instead be stuffing that into the dylib -- that would definitely remove any ambiguity about there being multiple copies of the object. And if we do that, we could also hide the implementation behind a pimpl-like idiom, that way

  • we do not have to depend on std::map in syncstream
  • we can change to another implementation (e.g. std::flat_map) in the future without impacting our ABI

Concretely, since this is pretty heavyweight anyway, I don't think that we gain much by having these methods defined in headers in the hopes that Clang would inline stuff better.

239
442–445

I think these can be removed since they are in iosfwd already.

525–528

Same, can be removed!

libcxx/test/std/input.output/syncstream/osyncstream/members/get_wrapped.pass.cpp
1 ↗(On Diff #557562)

Are we missing a test for rdbuf?

libcxx/test/std/input.output/syncstream/osyncstream/syncstream.osyncstream.cons/cons.move.pass.cpp
17 ↗(On Diff #557562)
36 ↗(On Diff #557562)

Let's add a test for noexcept-ness since the current WP clearly specifies it, but we might remove it in the future.

54 ↗(On Diff #557562)

This tests for implicit-ness too. IMO using std::move here instead of a temporary makes the test clearer, but this is equivalent so your choice.

libcxx/test/std/input.output/syncstream/osyncstream/syncstream.osyncstream.cons/cons.ostream.allocator.pass.cpp
34 ↗(On Diff #557562)

const?

42 ↗(On Diff #557562)

For explicit.

libcxx/test/std/input.output/syncstream/osyncstream/syncstream.osyncstream.cons/cons.ostream.pass.cpp
34 ↗(On Diff #557562)

Reversed arguments.

libcxx/test/std/input.output/syncstream/osyncstream/thread/basic.pass.cpp
19–20

Can you replace this by a short comment explaining what this tests?

libcxx/test/std/input.output/syncstream/osyncstream/types.compile.pass.cpp
39–40

Remove?

libcxx/test/std/input.output/syncstream/syncbuf/special.pass.cpp
2

I think this is now tested in cons.pointer.allocator.pass.cpp and cons.move.pass.cpp?

libcxx/test/std/input.output/syncstream/syncbuf/sputc.pass.cpp
149

return 0!

libcxx/test/std/input.output/syncstream/syncbuf/sputn.pass.cpp
132

return 0

libcxx/test/std/input.output/syncstream/syncbuf/syncstream.syncbuf.assign/assign.pass.cpp
18

We don't seem to be testing that this is noexcept, and also I don't think we test the return type + return value of the function (with the usual same_as<basic_syncbuf&> decltype(auto) ret = (assignment); assert(&ret == &obj);).

72–73
106

I think this is a copy-paste, you probably don't need to check this again here. Probably same thing below?

108

Duplicated too. Below as well?

232

Generally speaking, I find it useful to add a short plain english comment explaining these test cases, since what's being tested may not be easy to figure out a few years down the line. I also wouldn't mind if these were defined inline inside test() w/ the comments.

259
288

etc..

libcxx/test/std/input.output/syncstream/syncbuf/syncstream.syncbuf.cons/cons.pointer.allocator.pass.cpp
32

We should make this std::allocator<CharT> const alloc; (below too), that way the test would fail if the constructor were declared as basic_syncbuf(streambuf_type* obuf, Allocator&);, which would pass right now.

34

Here and below, we should use Buf buf = {nullptr, alloc} to test that this is an implicit constructor.

40–42

We could change those to

#ifndef TEST_HAS_NO_THREADS
  LIBCPP_ASSERT(...);
#endif

Non-blocking, just a suggestion if you like that better.

44

So here IIUC, the following would be equally valid:

std::basic_stringbuf<CharT> w;
Buf buf = {&w, alloc};

IMO not reusing basic_syncbuf here would make the test clearer.

libcxx/test/std/input.output/syncstream/syncbuf/syncstream.syncbuf.cons/cons.pointer.pass.cpp
33

I think what you want here is

static_assert(!std::convertible_to<std::basic_streambuf<CharT>*, Buf>);
42

My comment above about reusing Buf applies here and everywhere else.

libcxx/test/std/input.output/syncstream/syncbuf/syncstream.syncbuf.cons/const.move.pass.cpp
1 ↗(On Diff #557562)

Name of the file: cons.move.pass.cpp instead of const.move.pass.cpp?

33 ↗(On Diff #557562)
Buf buf = std::move(b);

For implicit-ness?

44 ↗(On Diff #557562)

Consider using b1 and b2 or something like that to make it clearer what is what. Just a suggestion.

135 ↗(On Diff #557562)

Not attached to this file: can we add a dtor.pass.cpp test that ensures that we call emit()?

libcxx/test/std/input.output/syncstream/syncbuf/syncstream.syncbuf.members/emit.pass.cpp
2

You swapped the filename with the one for set_emit_on_sync.pass.cpp.

28

I can't find a test for sync. There is a protected method sync() but I don't see a test for it either, I think.

30

We should at least run the code buff.set_emit_on_sync, right now we don't even codegen it.

libcxx/test/std/input.output/syncstream/syncbuf/syncstream.syncbuf.members/get_wrapped.pass.cpp
30–31

That way we test the return type properly.

libcxx/test/std/input.output/syncstream/syncbuf/syncstream.syncbuf.members/helpers.h
33–37

Can we replace this by the usual test_allocator?

libcxx/test/std/input.output/syncstream/syncbuf/syncstream.syncbuf.members/set_emit_on_sync.pass.cpp
28
libcxx/test/std/input.output/syncstream/syncbuf/types.compile.pass.cpp
37–42

I think we should remove the third assertion here.

64–65

Same here, remove.

libcxx/test/std/input.output/syncstream/syncbuf/virtuals.pass.cpp
15–19

This synopsis is wrong. I think this is the sync test we were looking for earlier.

37

Should we comment that this calls buff.sync() under the hood?

This revision now requires changes to proceed.Oct 3 2023, 10:41 AM
Mordante marked 14 inline comments as done.Oct 24 2023, 8:16 AM
Mordante added inline comments.
libcxx/include/syncstream
158

I prefer to do this in a followup commit. I feel the commit is already quite large and I prefer generic tools not to be hidden in these commits.

387

I prefer this too for a separate review. Especially since I already have something not generic in the implementation of format. That could benefit from a generic routine. The same for the places that use the now deprecated get_temporary_buffer. That would allow us to properly deprecate that function.

libcxx/test/std/input.output/syncstream/osyncstream/members/get_wrapped.pass.cpp
1 ↗(On Diff #557562)

Indeed we are, added it.

libcxx/test/std/input.output/syncstream/osyncstream/syncstream.osyncstream.cons/cons.move.pass.cpp
54 ↗(On Diff #557562)

I like the current way slightly better.

Mordante updated this revision to Diff 557867.Oct 24 2023, 8:21 AM
Mordante marked 2 inline comments as done.

Addresses review comments.

ldionne added inline comments.Oct 25 2023, 9:13 AM
libcxx/include/syncstream
158

Can we file a Github issue and add a TODO here to look into this? That way, we have something actionable to follow up on in a PR in the near future after shipping this patch.

387

This review is extremely relevant for any attempt to remove temporary_buffer again: https://reviews.llvm.org/D152208

FWIW I think we should 100% do it and we meant to pick this up again but it hasn't happened yet.

Mordante updated this revision to Diff 557885.Oct 25 2023, 1:56 PM

CI fixes.

Mordante updated this revision to Diff 557940.Oct 30 2023, 11:55 AM
Mordante marked 37 inline comments as done.

Rebased and addresses review comments.

Seems some of the comments I marked as done were not registered in Phab. Reviewed the entire patch again, all should be done now.

libcxx/include/map
1702–1703
libcxx/include/syncstream
158
442–445

Actually this header should provide them. When not doing that the std modules fail, so I reverted this part and the other using.

libcxx/test/std/input.output/syncstream/syncbuf/syncstream.syncbuf.assign/assign.pass.cpp
18

Actually it's not a noexcept function per https://cplusplus.github.io/LWG/issue3867. I had already updated the header but not the synopsis here. I added the other return type test.

libcxx/test/std/input.output/syncstream/syncbuf/syncstream.syncbuf.cons/const.move.pass.cpp
33 ↗(On Diff #557562)

As mentioned I prefer this way.

135 ↗(On Diff #557562)

I've added the test based on the emit test.

libcxx/test/std/input.output/syncstream/syncbuf/syncstream.syncbuf.members/emit.pass.cpp
28

Updated the comment, it was already tested.

libcxx/test/std/input.output/syncstream/syncbuf/syncstream.syncbuf.members/helpers.h
33–37

This is again a special allocator. As mentioned during a live review we should look at the number of "test allocators" we have and try to get a good set. I feel that's out of scope for this patch.

libcxx/test/std/input.output/syncstream/syncbuf/types.compile.pass.cpp
37–42

Odd I'm 100% certain I removed them before, seems I did make an error with the previous update.

ldionne accepted this revision.Nov 3 2023, 10:12 AM

I suggest we ship this but guard it under -fexperimental-library since we know there are some things we want to improve. That way there's no risk of shipping it before we're confident about our implementation choices.

libcxx/include/syncstream
3
473
This revision is now accepted and ready to land.Nov 3 2023, 10:12 AM
ldionne added inline comments.Nov 3 2023, 10:13 AM
libcxx/docs/FeatureTestMacroTable.rst
281

We should mention this in a release note (but mention it is experimental).

Mordante marked 3 inline comments as done.Nov 7 2023, 8:52 AM
Mordante updated this revision to Diff 558038.Nov 7 2023, 9:16 AM

Rebased and addresses are review comments.
Give it a CI run to test the change to the experimental library.

This revision was automatically updated to reflect the committed changes.