Re: [lmi] Parallel blues

[Greg Chicares]

On 2021-08-09 14:25, Vadim Zeitlin wrote:
> 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).

I had persuaded myself that I was trying to do something that really
seems reasonable but just couldn't be done with iostreams, and wanted
to use that perceived shortcoming of the standard library as an excuse
to accept failure (after spending a lot of time fruitlessly).

Then I reviewed that excuse later, and found that the failure wasn't
just aesthetic--it was UB as you had reported.

Then it occurred to me that I could solve the problem by overloading.
That's simple and easy, and AFAICT it would work correctly, so the
question is whether or not to use overloading that way (below).

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

Yes, exactly.

> If so, couldn't we just outlaw changing null stream rdbuf in lmi code? Or
> do we actually need to do it sometimes?

We can't outlaw it: we actually do need it sometimes.

> 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()?
[...]
>  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.

Okay, UB is exactly what I'd like to prevent.

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

Okay, then that's a solution for the long term, but not for today.
Then again, we don't use multiple threads today; by the time we do,
clang will presumably offer <syncstream>. The question then becomes
whether to commit an easy solution based on overloading now, or plan
to implement a <syncstream> solution later (which would take more
work, but would be an interesting learning experience--though I'm not
sure it would produce a better final outcome).

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

IIRC, that was mete_dob_limit() in 'calendar_date_test.cpp'. Last week, I
looked through the applicable series of commits there, and it looked like
the formerly-observed problem has been solved. Given the test output today:
  DOB limit    : 2.591e-05 s mean;         24 us least of 386 runs
it looks like we have little to gain by further optimizing that test,
which is now relevant only because it found UB.

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

Correct. Let's not even think of that for 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).

Since writing that, I've received vaccines against tetanus, pneumonia,
herpes zoster, and pointers. Even thinking about pointers is likely to
trigger asteriskosis or ampersanditis.

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

For the long term, maybe std::format or <syncstream> is the answer.
For the short term, there's no problem to fix, because lmi isn't
using multiple threads. My only question is whether or not I should
implement the simple overloading solution, above, at this time.
I tend to favor doing that anyway because it's fresh in my mind.
But do you see any reason not to proceed that way?