Re: [lmi] Parallel blues

[Vadim Zeitlin]

On Sat, 7 Aug 2021 23:18:50 +0000 Greg Chicares <gchicares@sbcglobal.net> wrote:

GC> On 2017-06-20 17:09, Vadim Zeitlin wrote:
GC> [...parallelizing 'calendar_date_test'...> 
GC> >  Unfortunately, it doesn't work and it took me slightly longer to find out
GC> > why does it crash when I run it. It turns out, that test code calls
GC> > {min,max}imum_birthdate(), which calls birthdate_limit::operator(), which
GC> > calls decimal_root() which outputs some (debugging?) messages to the
GC> > provided iteration_stream which is by default just null_stream(). And
GC> > null_stream() returns a reference to a static object, which ends up being
GC> > used from multiple threads at once -- hence the crash.
GC> After writing commit message 100320168adb ...
GC> 
GC> | Given
GC> |   foo(arg1, arg2, std::ostream& os=some_default())
GC> | it's not possible to write some_default() in a way that serves up an
GC> | already-constructed (hence, static or global) stream while avoiding the
GC> | peril that its streambuf can be replaced (with global effect). These
GC> | techniques don't work:
GC> |  - Derive from std::ostream, and override rdbuf()...which isn't virtual,
GC> |    so the std::ostream& argument doesn't know about it.
GC> |  - Look for a virtual function that might be overridden, at least to
GC> |    assert that the streambuf hasn't been altered...but there is none in
GC> |    std::ostream, and overriding a streambuf function doesn't help when
GC> |    the streambuf has been replaced.
GC> |  - Try messing with unique_ptr...which means changing the problem to
GC> |      foo(arg1, arg2, SomeNewCustomStreamClass& os=some_default())
GC> |    but that's too much work.
GC> |  - Move constructor...nope:
GC> |      https://cplusplus.github.io/LWG/lwg-defects.html#911

 I did read this message when you made this commit and decided that I
needed to return to it later, because I didn't fully understand the problem
on first reading. Unfortunately after rereading it again now, I still don't
quite understand it: what exactly is the peril that it is talking about?
Is it the one demonstrated by 048b2cc9d (Demonstrate a peril, 2021-08-04)?
If so, couldn't we just outlaw changing null stream rdbuf in lmi code? Or
do we actually need to do it sometimes? Sorry if I'm missing something
obvious here.

GC> Let me start by asking a question about this:
GC> 
GC> > null_stream() returns a reference to a static object, which ends up being
GC> > used from multiple threads at once -- hence the crash.
GC> 
GC> Would it have crashed if std::cout had been used instead of null_stream()?

 To answer this, I first tried to remember why did it crash with null_stream
in the first place, but, of course, couldn't recall it. And unfortunately I
can't reproduce the crash any more. Although my patch from
https://lists.nongnu.org/archive/html/lmi/2017-06/msg00007.html
still applies and still results in thread sanitizer errors due to
manipulating the same object from multiple threads without locking, it
doesn't crash any longer. Note that when I say "manipulating", it means, by
a nice coincidence, using stream manipulators here, e.g. std::setw() in the
"expatiate" lambda or std::setprecision() in the "recapitulate" one. IOW,
even if operator<<() itself is safe, the manipulators are definitely not.

 If we reformulate the question as "would it have still contain data races
and so be subject to undefined behaviour if std::cout had been used", then
the answer is "Yes": we still can't call std::ios_base::width() on the same
stream object from multiple threads even when using std::cout.

GC> If the answer is "No", then that must be because of some magical property
GC> that std::cout possesses, and the question becomes how to imbue this
GC> iteration-tracing stream with similar magic. Is std::osyncstream the
GC> C++20 voodoo that makes this work?

 I didn't know about this class, thanks. Using it would indeed be a safe
way of writing to the same stream from multiple threads -- in fact, before
reading this message and osyncstream description, I was going to propose
doing something like this ourselves (and am now very relieved that we don't
have to deal with stream buffers and other iostream black magic).

 I see that <syncstream> is already supported by libstdc++, so I could try
using it. It's not supported by clang/libc++ however, so we'd lose the
possibility to build with clang if we start using it.

GC> (The goal of this email thread was to parallelize a unit test which
GC> always uses a "null stream", so it doesn't matter if characters written
GC> to it by different threads are interleaved, because no such character
GC> will actually be written.)

 BTW, the strange thing is that testing right now I don't see any speed
advantage from parallelizing this test. It takes 2.0s on my test machine on
master (consuming 100% CPU) and 2.7s with the parallelization patch from
above (consuming 330% CPU). And even putting each test function in its own
thread doesn't change this (neither the run-time nor the CPU utilization),
so it looks like there is some function which takes much longer than the
other ones, although I didn't check which one it was yet (please let me
know if I should).


GC> Otherwise, I fear I've painted myself into a corner, out of which I was
GC> hoping I could teleport without leaving a real problem behind. I suppose
GC> this could be resolved by overloading:
GC> 
GC> - void foo(int arg0, double arg1, std::ostream& os=null_stream()) {...}
GC> 
GC> + void foo(int arg0, double arg1, std::ostream& os) {...}
GC> + void foo(int arg0, double arg1)
GC> + {
GC> +     std::ostream os(&null_streambuf());
GC> +     os.setstate(std::ios::badbit);
GC> +     foo(arg0, arg1, os);
GC> + }
GC> 
GC> That's only somewhat ugly. I like it better than
GC> 
GC> - void foo(int arg0, double arg1, std::ostream& os=null_stream()) {...}
GC> + void foo(int arg0, double arg1, std::ostream* os=nullptr)
GC> + {
GC> +     if(nullptr == os)
GC> +         {
GC> +         std::ostream ephemeral_os(&null_streambuf());
GC> +         ephemeral_os.setstate(std::ios::badbit);
GC> +         os = &ephemeral_os;
GC> +         }
GC> + }
GC> 
GC> What do you think?

 As always, my first instinct when having a problem with iostreams is to
try to avoid using them at all. In this case it means that I'd rather take
neither a std::ostream reference nor a pointer as an argument here, but a
reference to some lmi diagnostics_writer class that would natively support
suppressing diagnostics. However I don't think you'd want to contemplate
changes of such magnitude right now.

 And if we have to choose among the variants above, I'd probably prefer
passing a pointer and test it before using it, i.e. avoid constructing any
ephemeral streams as in the second example above (you're probably aware of
it, but this example is broken in any case as the pointer would become
dangling on "if" scope exit). This is less elegant than the "null object"
approach, but is simple, relatively safe (if you forget to check for null
pointer, you get an immediate crash rather than some subtle bug) and
optimal from performance point of view.

 Sorry if all this is not very helpful, but I'm afraid I still don't know
which problem exactly are we trying to solve here. Unless the problem is
using iostreams in the first place, in which case we do finally have an
answer for it, which is to use fmt.

 Regards,
VZ

pgpyG14w9Mk47.pgp
Description: PGP signature