On 4/29/19 7:53 AM, Eli Zaretskii wrote:
> see, for example, the comments by Florian
> Weimer in this GCC bug report:
>
>   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88793

Those comments don't invalidate all uses of __attribute__ ((cold)) in
Emacs. Florian says that in cold functions on some platforms, strlen on
large (5000-byte) strings can be as much as 60 times slower due to
questionable optimizations in GCC that are currently being rethought
<https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88809#c5>. However, if the
functions currently marked 'cold' in Emacs are called so rarely (at
least on such large strings) that the performance cost (factor of 60 for
rarely-used code) is outweighed by the performance benefit (small factor
of improvement in commonly-used code), then from a performance viewpoint
it's still preferable to mark these functions 'cold'.


> the GCC manual indicates that marking a function as 'cold'
> causes that function to be optimized for size, and we already know
> that GCC 8.x has at least one bug with -Os used together with -O2,

The GCC bug report <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90020>
says that the bug can also occur with -O2, and can occur without
__attribute__ ((cold)). So it's not as simple as saying "__attribute__
((cold)) is buggy".


> I think we have way too many functions marked with this
> attribute.  Functions like wrong_type_argument or Fthrow or
> Fabort_recursive_edit or Fsignal or error or Ftop_level -- do they
> really fit the below description?

It should be purely a performance issue.

For example, on my platform (x86-64, GCC 8.3.1, -O2, Fedora 29) after
inlining, Emacs has 1066 static calls to wrong_type_argument, and with
__attribute__ ((cold)) all these calling sites can be optimized. In
bytecode.c this causes GCC to move wrong_type_argument calls to a cold
section of the text, which means the bytecode interpreter should run
faster for all the usual branch-prediction and icache reasons.

I measured the effect of __attribute__ ((cold)) (including both costs
and benefits) as improving overall performance of a big benchmark ('make
compile-always') by 1.3% on my platform. This test included all the
roughly 50 functions currently marked as cold in Emacs. Although I can
imagine that removing __attribute__ ((cold)) from some of these
functions would not hurt overall performance significantly, I didn't
think it worth the time to investigate which ones. That is, I didn't
investigate marking a smaller set of functions. If someone wants to do
that work then I expect that we should be able to remove markings from
some of these functions without affecting performance significantly.


> if we leave it in the
> sources, we should disable it for MinGW, it seems.

Yes, that sounds like a good idea. I installed the attached patch. It
would be good to know what the actual bug is (it's possible that it's
not a compiler bug at all, but is a portability bug in Emacs), but
that's less urgent.