Page MenuHomePhabricator

Implement syncstream (p0053)
Needs ReviewPublic

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

Details

Reviewers
mclow.lists
EricWF
ldionne
Group Reviewers
Restricted Project
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.

Diff Detail

Unit TestsFailed

TimeTest
4,990 mslibcxx CI ASAN > libc++.std/input_output/syncstream/syncbuf::assign.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/bin/clang++ /home/libcxx-builder/.buildkite-agent/builds/b069686b2355-1/llvm-project/libcxx-ci/libcxx/test/std/input.output/syncstream/syncbuf/assign.pass.cpp -v --target=x86_64-unknown-linux-gnu -g -fno-omit-frame-pointer -fsanitize=address -include /home/libcxx-builder/.buildkite-agent/builds/b069686b2355-1/llvm-project/libcxx-ci/build/generic-asan/projects/libcxx/__config_site -include /home/libcxx-builder/.buildkite-agent/builds/b069686b2355-1/llvm-project/libcxx-ci/libcxx/test/support/nasty_macros.h -nostdinc++ -I/home/libcxx-builder/.buildkite-agent/builds/b069686b2355-1/llvm-project/libcxx-ci/libcxx/include -I/home/libcxx-builder/.buildkite-agent/builds/b069686b2355-1/llvm-project/libcxx-ci/build/generic-asan/projects/libcxx/include/c++build -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -D__STDC_CONSTANT_MACROS -I/home/libcxx-builder/.buildkite-agent/builds/b069686b2355-1/llvm-project/libcxx-ci/libcxx/test/support -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -O1 -std=c++2a -Werror -Wall -Wextra -Wshadow -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-user-defined-literals -Wno-noexcept-type -Wno-atomic-alignment -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -fmodules-cache-path=/home/libcxx-builder/.buildkite-agent/builds/b069686b2355-1/llvm-project/libcxx-ci/build/generic-asan/projects/libcxx/test/std/input.output/syncstream/syncbuf/Output/assign.pass.cpp.dir/t.tmp/ModuleCache -Wno-macro-redefined -D_LIBCPP_HAS_THREAD_API_PTHREAD -Wno-macro-redefined -D_LIBCPP_ABI_VERSION=1 -L/home/libcxx-builder/.buildkite-agent/builds/b069686b2355-1/llvm-project/libcxx-ci/build/generic-asan/./lib -Wl,-rpath,/home/libcxx-builder/.buildkite-agent/builds/b069686b2355-1/llvm-project/libcxx-ci/build/generic-asan/./lib -nodefaultlibs -lc++ -lm -lgcc_s -lgcc -lpthread -lc -lgcc_s -lgcc -latomic -lpthread…
1,070 mslibcxx CI C++03 > libc++.std/input_output/syncstream/osyncstream::assign.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/bin/clang++ /home/libcxx-builder/.buildkite-agent/builds/b069686b2355-1/llvm-project/libcxx-ci/libcxx/test/std/input.output/syncstream/osyncstream/assign.pass.cpp -v --target=x86_64-unknown-linux-gnu -include /home/libcxx-builder/.buildkite-agent/builds/b069686b2355-1/llvm-project/libcxx-ci/build/generic-cxx03/projects/libcxx/__config_site -include /home/libcxx-builder/.buildkite-agent/builds/b069686b2355-1/llvm-project/libcxx-ci/libcxx/test/support/nasty_macros.h -nostdinc++ -I/home/libcxx-builder/.buildkite-agent/builds/b069686b2355-1/llvm-project/libcxx-ci/libcxx/include -I/home/libcxx-builder/.buildkite-agent/builds/b069686b2355-1/llvm-project/libcxx-ci/build/generic-cxx03/projects/libcxx/include/c++build -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -D__STDC_CONSTANT_MACROS -I/home/libcxx-builder/.buildkite-agent/builds/b069686b2355-1/llvm-project/libcxx-ci/libcxx/test/support -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -std=c++03 -Werror -Wall -Wextra -Wshadow -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-user-defined-literals -Wno-noexcept-type -Wno-atomic-alignment -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -fmodules-cache-path=/home/libcxx-builder/.buildkite-agent/builds/b069686b2355-1/llvm-project/libcxx-ci/build/generic-cxx03/projects/libcxx/test/std/input.output/syncstream/osyncstream/Output/assign.pass.cpp.dir/t.tmp/ModuleCache -Wno-macro-redefined -D_LIBCPP_HAS_THREAD_API_PTHREAD -Wno-macro-redefined -D_LIBCPP_ABI_VERSION=1 -L/home/libcxx-builder/.buildkite-agent/builds/b069686b2355-1/llvm-project/libcxx-ci/build/generic-cxx03/./lib -Wl,-rpath,/home/libcxx-builder/.buildkite-agent/builds/b069686b2355-1/llvm-project/libcxx-ci/build/generic-cxx03/./lib -nodefaultlibs -lc++ -lm -lgcc_s -lgcc -lpthread -lc -lgcc_s -lgcc -latomic -lc++experimental -o /home/libcxx-builder/.
1,200 mslibcxx CI C++03 > libc++.std/input_output/syncstream/osyncstream::const.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/bin/clang++ /home/libcxx-builder/.buildkite-agent/builds/b069686b2355-1/llvm-project/libcxx-ci/libcxx/test/std/input.output/syncstream/osyncstream/const.pass.cpp -v --target=x86_64-unknown-linux-gnu -include /home/libcxx-builder/.buildkite-agent/builds/b069686b2355-1/llvm-project/libcxx-ci/build/generic-cxx03/projects/libcxx/__config_site -include /home/libcxx-builder/.buildkite-agent/builds/b069686b2355-1/llvm-project/libcxx-ci/libcxx/test/support/nasty_macros.h -nostdinc++ -I/home/libcxx-builder/.buildkite-agent/builds/b069686b2355-1/llvm-project/libcxx-ci/libcxx/include -I/home/libcxx-builder/.buildkite-agent/builds/b069686b2355-1/llvm-project/libcxx-ci/build/generic-cxx03/projects/libcxx/include/c++build -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -D__STDC_CONSTANT_MACROS -I/home/libcxx-builder/.buildkite-agent/builds/b069686b2355-1/llvm-project/libcxx-ci/libcxx/test/support -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -std=c++03 -Werror -Wall -Wextra -Wshadow -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-user-defined-literals -Wno-noexcept-type -Wno-atomic-alignment -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -fmodules-cache-path=/home/libcxx-builder/.buildkite-agent/builds/b069686b2355-1/llvm-project/libcxx-ci/build/generic-cxx03/projects/libcxx/test/std/input.output/syncstream/osyncstream/Output/const.pass.cpp.dir/t.tmp/ModuleCache -Wno-macro-redefined -D_LIBCPP_HAS_THREAD_API_PTHREAD -Wno-macro-redefined -D_LIBCPP_ABI_VERSION=1 -L/home/libcxx-builder/.buildkite-agent/builds/b069686b2355-1/llvm-project/libcxx-ci/build/generic-cxx03/./lib -Wl,-rpath,/home/libcxx-builder/.buildkite-agent/builds/b069686b2355-1/llvm-project/libcxx-ci/build/generic-cxx03/./lib -nodefaultlibs -lc++ -lm -lgcc_s -lgcc -lpthread -lc -lgcc_s -lgcc -latomic -lc++experimental -o /home/libcxx-builder/.
1,130 mslibcxx CI C++03 > libc++.std/input_output/syncstream/osyncstream::members.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/bin/clang++ /home/libcxx-builder/.buildkite-agent/builds/b069686b2355-1/llvm-project/libcxx-ci/libcxx/test/std/input.output/syncstream/osyncstream/members.pass.cpp -v --target=x86_64-unknown-linux-gnu -include /home/libcxx-builder/.buildkite-agent/builds/b069686b2355-1/llvm-project/libcxx-ci/build/generic-cxx03/projects/libcxx/__config_site -include /home/libcxx-builder/.buildkite-agent/builds/b069686b2355-1/llvm-project/libcxx-ci/libcxx/test/support/nasty_macros.h -nostdinc++ -I/home/libcxx-builder/.buildkite-agent/builds/b069686b2355-1/llvm-project/libcxx-ci/libcxx/include -I/home/libcxx-builder/.buildkite-agent/builds/b069686b2355-1/llvm-project/libcxx-ci/build/generic-cxx03/projects/libcxx/include/c++build -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -D__STDC_CONSTANT_MACROS -I/home/libcxx-builder/.buildkite-agent/builds/b069686b2355-1/llvm-project/libcxx-ci/libcxx/test/support -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -std=c++03 -Werror -Wall -Wextra -Wshadow -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-user-defined-literals -Wno-noexcept-type -Wno-atomic-alignment -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -fmodules-cache-path=/home/libcxx-builder/.buildkite-agent/builds/b069686b2355-1/llvm-project/libcxx-ci/build/generic-cxx03/projects/libcxx/test/std/input.output/syncstream/osyncstream/Output/members.pass.cpp.dir/t.tmp/ModuleCache -Wno-macro-redefined -D_LIBCPP_HAS_THREAD_API_PTHREAD -Wno-macro-redefined -D_LIBCPP_ABI_VERSION=1 -L/home/libcxx-builder/.buildkite-agent/builds/b069686b2355-1/llvm-project/libcxx-ci/build/generic-cxx03/./lib -Wl,-rpath,/home/libcxx-builder/.buildkite-agent/builds/b069686b2355-1/llvm-project/libcxx-ci/build/generic-cxx03/./lib -nodefaultlibs -lc++ -lm -lgcc_s -lgcc -lpthread -lc -lgcc_s -lgcc -latomic -lc++experimental -o /home/libcxx-builder/.
1,070 mslibcxx CI C++03 > libc++.std/input_output/syncstream/osyncstream::types.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/bin/clang++ /home/libcxx-builder/.buildkite-agent/builds/b069686b2355-1/llvm-project/libcxx-ci/libcxx/test/std/input.output/syncstream/osyncstream/types.pass.cpp -v --target=x86_64-unknown-linux-gnu -include /home/libcxx-builder/.buildkite-agent/builds/b069686b2355-1/llvm-project/libcxx-ci/build/generic-cxx03/projects/libcxx/__config_site -include /home/libcxx-builder/.buildkite-agent/builds/b069686b2355-1/llvm-project/libcxx-ci/libcxx/test/support/nasty_macros.h -nostdinc++ -I/home/libcxx-builder/.buildkite-agent/builds/b069686b2355-1/llvm-project/libcxx-ci/libcxx/include -I/home/libcxx-builder/.buildkite-agent/builds/b069686b2355-1/llvm-project/libcxx-ci/build/generic-cxx03/projects/libcxx/include/c++build -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -D__STDC_CONSTANT_MACROS -I/home/libcxx-builder/.buildkite-agent/builds/b069686b2355-1/llvm-project/libcxx-ci/libcxx/test/support -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -std=c++03 -Werror -Wall -Wextra -Wshadow -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-user-defined-literals -Wno-noexcept-type -Wno-atomic-alignment -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -fmodules-cache-path=/home/libcxx-builder/.buildkite-agent/builds/b069686b2355-1/llvm-project/libcxx-ci/build/generic-cxx03/projects/libcxx/test/std/input.output/syncstream/osyncstream/Output/types.pass.cpp.dir/t.tmp/ModuleCache -Wno-macro-redefined -D_LIBCPP_HAS_THREAD_API_PTHREAD -Wno-macro-redefined -D_LIBCPP_ABI_VERSION=1 -L/home/libcxx-builder/.buildkite-agent/builds/b069686b2355-1/llvm-project/libcxx-ci/build/generic-cxx03/./lib -Wl,-rpath,/home/libcxx-builder/.buildkite-agent/builds/b069686b2355-1/llvm-project/libcxx-ci/build/generic-cxx03/./lib -nodefaultlibs -lc++ -lm -lgcc_s -lgcc -lpthread -lc -lgcc_s -lgcc -latomic -lc++experimental -o /home/libcxx-builder/.
View Full Test Results (79 Failed)

Event Timeline

zoecarver created this revision.Sep 2 2019, 1:58 PM
zoecarver marked 4 inline comments as done.Sep 2 2019, 2:07 PM
zoecarver added inline comments.
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 updated this revision to Diff 219471.Sep 9 2019, 8:12 PM
  • tests for osyncstream
  • implement osyncstream
  • update status
  • general cleanup
zoecarver retitled this revision from Implement std::basic_syncbuf (p0053) to Implement syncstream (p0053).Sep 9 2019, 8:15 PM
zoecarver edited the summary of this revision. (Show Details)

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 added a comment.EditedSep 10 2019, 10:38 AM

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 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
100

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.