Re: [Patch] libstdc++/28277 valarray bits

[Gabriel Dos Reis <gdr at integrable-solutions dot net>]

Paolo Carlini <pcarlini@suse.de> writes:

| Hi all, hi Gaby,
| 
| I'm trying to fix the valarray bits of this issue, which seem
| selfcontained. As you can see, currently valarray::shift and
| valarray::cshift go through an unbounded __builtin_alloca in order to
| construct in it the shifted valarray and then construct the result
| from that memory. A lot of work and memory, instead of constructing
| the result in place. Apparently, that is done in order to avoid
| leaking the raw memory allocated by __valarray_get_storage in case
| either __valarray_default_construct or __valarray_copy_construct throw.
| 
| However, it's known (and reminded many times in bits/valarray_array.h)
| that valarray is not required to be exception safe (for performance
| reasons, obviously) and, for example, *all* our constructors leak the
| raw memory in case one of the above helpers throws (+ the individual
| elements are not destructed, in other terms we don't use something
| like uninitialized_copy and uninitialized_fill, again see
| bits/valarray_array.h). Therefore, taking into account the general
| design of our implementation I think it's correct to have shift and
| cshift completely similar to the constructors, that is, efficient,
| using well the memory, but not trying to be partially exception safe
| (at best only partially, of course, because the issue with individual
| elements not destructed currently affects shift and csfift too,
| exactly like the constructors).
| 
| Gaby, makes sense to you?


I think you have tried hard to find lot of reasons to justify the
current shape of the codes; thanks for the credit :-)

I don't think I've tried to make valarray exception safe.  This stuff
is for number crunching, and as recognized in the standard wording it
is not the kind of thing that is supposed to handle exceptions correctly.
However, it is expected to do its job pretty fast.  Consequently, were
temporary storage is needed for local computation, I've tried to use
the stack -- worries about unbounded alloca is NOT an issue here.
High performance number crunchers are willing to adjust the stack --
that has been existing practice since FORTRANS ages.  So, I don't
think you should spend effort to remove alloca from the valarray
implementation for the sake of removing alloca -- using the stack also
gives the compiler a hint about the containment of the storage and
when it gets smart apply appropriate optimization.o
This stuff was written when we did not have the NRVO, and the other
way of doing it on average was faster.

However I do agree with you that where the work can be done
efficiently in place, that should happen.

OK.

-- Gaby