Implement syncstream (p0053)

[zoecarver]

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.

[zoecarver]

libcxx/include/syncstream

16

Oops. I'll add this.

libcxx/test/std/input.output/sncstream/syncbuf/cons.pass.cpp

70 ↗

(On Diff #218391)

I was a bit confused by this. I am going to email Peter after I figure out what I think this should say.

libcxx/test/std/input.output/sncstream/syncbuf/members.pass.cpp

52 ↗

(On Diff #218391)

I'm planning on testing this more extensively and adding thread tests. What is the best way to test with threads (our current thread tests seem to be lacking)?

libcxx/test/std/input.output/sncstream/syncbuf/virtuals.pass.cpp

54 ↗

(On Diff #218391)

This is bad. I'm planning on updating it.

[zoecarver]

• tests for osyncstream
• implement osyncstream
• update status
• general cleanup

[mclow.lists]

A general comment about the tests: You've got a bunch of methods marked "noexcept", but nowhere to I see ASSERT_NOEXCEPT or ASSERT_NOT_NOEXCEPT in the tests to check that.

libcxx/include/iosfwd

86

You need to update the synopsis as well.

libcxx/include/syncstream

226

These one or two line functions can be just put inline.

_LIBCPP_INLINE_VISIBILITY
~basic_syncbuf() { emit(); }

426

These need to be wrapped in:

#ifndef _LIBCPP_NO_EXCEPTIONS
        try
        {
#endif  // _LIBCPP_NO_EXCEPTIONS
        emit();
#ifndef _LIBCPP_NO_EXCEPTIONS
        } catch(...) { }
#endif  // _LIBCPP_NO_EXCEPTIONS

[mclow.lists]

Maybe I'm missing something, but I don't see any synchronization between two syncstreams wrapping the same stream.

Like this:

ofstream f ("foo.txt");
fire_off_a_thread_with(basic_osyncstream(&f));
fire_off_a_thread_with(basic_osyncstream(&f));

How does the output to f get synchronized between the two threads?

libcxx/include/syncstream

103

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

227

What's the expected behavior here if emit throws an exception?

318

We don't usually test == true or == false.
How about if (!emit()) return -1; instead?
Or even if (__emit_on_sync && !emit()) return -1;

[zoecarver]

• Updated mutex for gaurd_lock so that it corresponds to a streambuf.
• Added more thread tests.

[zoecarver]

• 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]

• don't use not keyword

[mclow.lists]

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]

libcxx/include/syncstream

221

How do entries get removed from __mtx_ptr_map?

[zoecarver]

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]

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]

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]

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]

• use contains
• use function level static
• use map instead of unordered_map

[zoecarver]

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]

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)?

[zoecarver]

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

[mclow.lists]

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]

libcxx/include/iosfwd

100

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

libcxx/include/syncstream

52

@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.

226

Minor nit __gaurd -> __guard.

230

Is it intended to use assert in a header?

238

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

283

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