Re: [PATCH] libstdc++: Use RAII in std::vector::_M_realloc_insert

[Jonathan Wakely <jwakely@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3762 bytes --]

On Fri, 23 Jun 2023 at 17:44, Jan Hubicka wrote:

> > I intend to push this to trunk once testing finishes.
> >
> > I generated the diff with -b so the whitespace changes aren't shown,
> > because there was some re-indenting that makes the diff look larger than
> > it really is.
> >
> > Honza, I don't think this is likely to make much difference for the PR
> > 110287 testcases, but I think it simplifies the code and so is an
> > improvement in terms of maintenance and readability.
>
> Thanks for cleaning it up :)
> The new version seems slightly smaller than the original in inliner
> metrics.
>

Oh good. It's pushed to trunk now.

[snip]


> To work out that the code path is really very unlikely and should be
> offloaded to a cold section, we however need:
>
> diff --git a/libstdc++-v3/include/bits/functexcept.h
> b/libstdc++-v3/include/bits/functexcept.h
> index 89972baf2c9..2765f7865df 100644
> --- a/libstdc++-v3/include/bits/functexcept.h
> +++ b/libstdc++-v3/include/bits/functexcept.h
> @@ -46,14 +46,14 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>  #if _GLIBCXX_HOSTED
>    // Helper for exception objects in <except>
>    void
> -  __throw_bad_exception(void) __attribute__((__noreturn__));
> +  __throw_bad_exception(void) __attribute__((__noreturn__,__cold__));
>
>    // Helper for exception objects in <new>
>    void
> -  __throw_bad_alloc(void) __attribute__((__noreturn__));
> +  __throw_bad_alloc(void) __attribute__((__noreturn__,__cold__));
>
>    void
> -  __throw_bad_array_new_length(void) __attribute__((__noreturn__));
> +  __throw_bad_array_new_length(void)
> __attribute__((__noreturn__,__cold__));
>
>    // Helper for exception objects in <typeinfo>
>    void
>
> This makes us to drop cont to profile_count::zero which indicates that
> the code is very likely not executed at all during run of the program.
>
> The reason why we can't take such a strong hint from unreachable
> attribute is twofold.  First most programs do call "exit (0)" so taking
> this as a strong hint may make us to optimize whole program for size.
> Second is that we consider a possibility that insane developers may make
> EH delivery relatively common.
>
> Would be possible to annotate throw functions in libstdc++ which are
> very unlikely taken by a working program as __cold__ and possibly drop
> the redundant __builtin_expect?
>

Yes, I incorrectly thought they were already considered cold, but your
explanation of why noreturn is not as strong a hint as noreturn+cold makes
sense.

I think the __throw_bad_alloc() and __throw_bad_array_new_length()
functions should always be rare, so marking them cold seems fine (users who
define their own allocators that want to throw bad_alloc "often" will
probably throw it directly, they shouldn't be using our __throw_bad_alloc()
function anyway). I don't think __throw_bad_exception is ever used, so that
doesn't matter (we could remove it from the header and just keep its
definition in the library, but there's no big advantage to doing that).
Others like __throw_length_error() should also be very very rare, and could
be marked cold.

Maybe we should just mark everything in <bits/functexcept.h> as cold. If
users want to avoid the cost of calls to those functions they can do so by
checking function preconditions/arguments to avoid the exceptions. There
are very few places where a throwing libstdc++ API doesn't have a way to
avoid the exception. The only one that isn't easily avoidable is
__throw_bad_alloc but OOM should be rare.



> I will reorder predictors so __builtin_cold_noreturn and
> __builtin_expect_with_probability thakes precedence over
> __builtin_expect.
>
> It is fun to see how many things can go wrong in such a simple use of
> libstdc++ :)
>

Yes, it's very interesting!