Crashes in "C-h h"

Crashes in "C-h h"

[Eli Zaretskii]

I'm getting crashes in "C-h h", they are caused by some commit between
e51f328 and cb182ce.  I suspect f59a3f3 and/or 4893a09.  The backtrace
is below.

Does anyone else see this, in particular on X?

  lisp.h:1198: Emacs fatal error: assertion failed: FIXNUMP (a)

  Thread 1 hit Breakpoint 1, terminate_due_to_signal (sig=22,
      backtrace_limit=2147483647) at emacs.c:377
  377       signal (sig, SIG_DFL);
  (gdb) bt
  #0  terminate_due_to_signal (sig=22, backtrace_limit=2147483647)
      at emacs.c:377
  #1  0x011ff850 in die (msg=0x16f14a8 <PSEUDOVECTOR_FLAG+4> "FIXNUMP (a)",
      file=0x16f1478 <Qzlib+8> "lisp.h", line=1198) at alloc.c:7194
  #2  0x011467ac in XFIXNUM (a=XIL(0)) at lisp.h:1198
  #3  0x0135a8ea in fontset_find_font (fontset=XIL(0xa000000006a97b18), c=1047,
      face=0x68dc4f8, charset_id=15, fallback=false) at fontset.c:578
  #4  0x0135b689 in fontset_font (fontset=XIL(0xa000000006a94248), c=1047,
      face=0x68dc4f8, id=15) at fontset.c:773
  #5  0x0135c617 in face_for_char (f=0x6894338, face=0x68dc4f8, c=1047,
      pos=278, object=XIL(0)) at fontset.c:987
  #6  0x0114b5d5 in FACE_FOR_CHAR (f=0x6894338, face=0x68dc4f8, character=1047,
      pos=278, object=XIL(0)) at dispextern.h:1869
  #7  0x0104884e in get_next_display_element (it=0x82b660) at xdisp.c:7425
  #8  0x01078c76 in display_line (it=0x82b660, cursor_vpos=3) at xdisp.c:21830
  #9  0x0106b92e in try_window (window=XIL(0xa0000000068c6aa8), pos=...,
      flags=1) at xdisp.c:17884
  #10 0x01068a9e in redisplay_window (window=XIL(0xa0000000068c6aa8),
      just_this_one_p=false) at xdisp.c:17332
  #11 0x01060bd3 in redisplay_window_0 (window=XIL(0xa0000000068c6aa8))
      at xdisp.c:15057
  #12 0x012379da in internal_condition_case_1 (
      bfun=0x1060b78 <redisplay_window_0>, arg=XIL(0xa0000000068c6aa8),
      handlers=XIL(0xc0000000058cfe48), hfun=0x1060b35 <redisplay_window_error>)
      at eval.c:1376
  #13 0x01060af8 in redisplay_windows (window=XIL(0xa0000000068c6aa8))
      at xdisp.c:15037
  #14 0x0105f241 in redisplay_internal () at xdisp.c:14520
  #15 0x0105c673 in redisplay () at xdisp.c:13747
  #16 0x0115949a in read_char (commandflag=1, map=XIL(0xc000000006924650),
      prev_event=XIL(0), used_mouse_menu=0x82f47f, end_time=0x0)
      at keyboard.c:2474
  #17 0x0116f73f in read_key_sequence (keybuf=0x82f780, prompt=XIL(0),
      dont_downcase_last=false, can_return_switch_frame=true,
      fix_current_buffer=true, prevent_redisplay=false) at keyboard.c:9125
  #18 0x01155180 in command_loop_1 () at keyboard.c:1348
  #19 0x012378f0 in internal_condition_case (bfun=0x1154a7b <command_loop_1>,
      handlers=XIL(0x5430), hfun=0x1153d24 <cmd_error>) at eval.c:1352
  #20 0x01154527 in command_loop_2 (ignore=XIL(0)) at keyboard.c:1091
  #21 0x01236abb in internal_catch (tag=XIL(0xdcb0),
      func=0x11544eb <command_loop_2>, arg=XIL(0)) at eval.c:1113
  #22 0x011544a6 in command_loop () at keyboard.c:1070
  #23 0x011537ac in recursive_edit_1 () at keyboard.c:714
  #24 0x01153a22 in Frecursive_edit () at keyboard.c:786
  #25 0x011512a7 in main (argc=2, argv=0xa42828) at emacs.c:2103

  Lisp Backtrace:
  "redisplay_internal (C function)" (0x0)

Re: Crashes in "C-h h"

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> I'm getting crashes in "C-h h", they are caused by some commit between
> e51f328 and cb182ce.  I suspect f59a3f3 and/or 4893a09.  The backtrace
> is below.
>
> Does anyone else see this, in particular on X?

I did a git pull now and compiled and C-h h didn't crash for me on:

In GNU Emacs 27.0.50 (build 12, x86_64-pc-linux-gnu, GTK+ Version 3.22.11)


-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Crashes in "C-h h"

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: Paul Eggert <eggert@cs.ucla.edu>,  emacs-devel@gnu.org
> Date: Mon, 01 Jul 2019 16:44:11 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > I'm getting crashes in "C-h h", they are caused by some commit between
> > e51f328 and cb182ce.  I suspect f59a3f3 and/or 4893a09.  The backtrace
> > is below.
> >
> > Does anyone else see this, in particular on X?
> 
> I did a git pull now and compiled and C-h h didn't crash for me on:
> 
> In GNU Emacs 27.0.50 (build 12, x86_64-pc-linux-gnu, GTK+ Version 3.22.11)

Could be specific to Windows, then.  I'll take a look if no one beats
me to it.

Thanks.

Re: Crashes in "C-h h"

[Andreas Schwab]

On Jul 01 2019, Eli Zaretskii <eliz@gnu.org> wrote:

>   #2  0x011467ac in XFIXNUM (a=XIL(0)) at lisp.h:1198
>   #3  0x0135a8ea in fontset_find_font (fontset=XIL(0xa000000006a97b18), c=1047,
>       face=0x68dc4f8, charset_id=15, fallback=false) at fontset.c:578

	    repertory = FONT_DEF_REPERTORY (RFONT_DEF_FONT_DEF (rfont_def));

	    if (XFIXNUM (repertory) == charset_id)

Looks like repertory is nil.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: Crashes in "C-h h"

[Robert Pluim]

>>>>> On Mon, 01 Jul 2019 17:38:04 +0300, Eli Zaretskii <eliz@gnu.org> said:

    Eli> I'm getting crashes in "C-h h", they are caused by some commit between
    Eli> e51f328 and cb182ce.  I suspect f59a3f3 and/or 4893a09.  The backtrace
    Eli> is below.

    Eli> Does anyone else see this, in particular on X?

Of course I donʼt, because I donʼt have --enable-checking turned on
:-)

With --enable-checking enabled it crashes for me in the same place as
you under both X and on macOS. Reverting 4893a09 fixes things for me
under X.

Robert

Re: Crashes in "C-h h"

[Eli Zaretskii]

> From: Andreas Schwab <schwab@suse.de>
> Cc: Paul Eggert <eggert@cs.ucla.edu>,  emacs-devel@gnu.org
> Date: Mon, 01 Jul 2019 17:01:32 +0200
> 
> On Jul 01 2019, Eli Zaretskii <eliz@gnu.org> wrote:
> 
> >   #2  0x011467ac in XFIXNUM (a=XIL(0)) at lisp.h:1198
> >   #3  0x0135a8ea in fontset_find_font (fontset=XIL(0xa000000006a97b18), c=1047,
> >       face=0x68dc4f8, charset_id=15, fallback=false) at fontset.c:578
> 
> 	    repertory = FONT_DEF_REPERTORY (RFONT_DEF_FONT_DEF (rfont_def));
> 
> 	    if (XFIXNUM (repertory) == charset_id)
> 
> Looks like repertory is nil.

Yes, that part is clear.  The question is why...

Thanks.

Re: Crashes in "C-h h"

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Cc: Paul Eggert <eggert@cs.ucla.edu>,  emacs-devel@gnu.org
> Date: Mon, 01 Jul 2019 17:06:28 +0200
> 
> >>>>> On Mon, 01 Jul 2019 17:38:04 +0300, Eli Zaretskii <eliz@gnu.org> said:
> 
>     Eli> I'm getting crashes in "C-h h", they are caused by some commit between
>     Eli> e51f328 and cb182ce.  I suspect f59a3f3 and/or 4893a09.  The backtrace
>     Eli> is below.
> 
>     Eli> Does anyone else see this, in particular on X?
> 
> Of course I donʼt, because I donʼt have --enable-checking turned on
> :-)

I guess neither did Lars.  Sorry for not mentioning that, I sometimes
forget that not everybody who is tracking master do that ;-)

> With --enable-checking enabled it crashes for me in the same place as
> you under both X and on macOS. Reverting 4893a09 fixes things for me
> under X.

Great, thanks.  This narrows the search quite a bit.

Re: Crashes in "C-h h"

[Pip Cet]

On Mon, Jul 1, 2019 at 3:29 PM Eli Zaretskii <eliz@gnu.org> wrote:
> > >   #2  0x011467ac in XFIXNUM (a=XIL(0)) at lisp.h:1198
> > >   #3  0x0135a8ea in fontset_find_font (fontset=XIL(0xa000000006a97b18), c=1047,
> > >       face=0x68dc4f8, charset_id=15, fallback=false) at fontset.c:578
> >
> >           repertory = FONT_DEF_REPERTORY (RFONT_DEF_FONT_DEF (rfont_def));
> >
> >           if (XFIXNUM (repertory) == charset_id)
> >
> > Looks like repertory is nil.
>
> Yes, that part is clear.  The question is why...

Is there a problem simply testing FIXNUMP in this place? If I'm
reading this correctly, repertory may be nil, but it might also be a
char table, which might, by sheer accident, have XFIXNUM_RAW
(repertory) == charset_id, so it looks like a bug that was caught, not
caused, by the recent XFIXNUM change.

Re: Crashes in "C-h h"

[Andreas Schwab]

On Jul 01 2019, Robert Pluim <rpluim@gmail.com> wrote:

> With --enable-checking enabled it crashes for me in the same place as
> you under both X and on macOS. Reverting 4893a09 fixes things for me
> under X.

But calling XFIXNUM on a non-FIXNUM is not a valid use.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: Crashes in "C-h h"

[Pip Cet]

> the recent XFIXNUM change.

"The recent XFIXNUM change" I rather cryptically alluded to is that
you're no longer allowed to call XFIXNUM on a Lisp_Object that isn't
FIXNUMP; previously, this was valid in some circumstances, either
because XFIXNUM (non_fixnum_object) wasn't actually ever used (FIXNUMP
check after the XFIXNUM), or because the only thing that was relied on
was that XFIXNUM (Qnil) == 0.

There are two fixes:
- check FIXNUMP
- use XFIXNUM_RAW

The second fix is only available to us if we're definitely sure that
XFIXNUM_RAW will not be used (except, maybe, if we want to introduce a
Qnil-is-0 dependency, in the case of a nil repertory). I don't think
that's the case here.

Re: Crashes in "C-h h"

[Eli Zaretskii]

> Date: Mon, 01 Jul 2019 18:27:29 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: eggert@cs.ucla.edu, emacs-devel@gnu.org
> 
> > 	    if (XFIXNUM (repertory) == charset_id)
> > 
> > Looks like repertory is nil.
> 
> Yes, that part is clear.  The question is why...

Wrong question.  Should be fixed now.

Re: Crashes in "C-h h"

[Eli Zaretskii]

> From: Pip Cet <pipcet@gmail.com>
> Date: Mon, 1 Jul 2019 15:40:49 +0000
> Cc: Andreas Schwab <schwab@suse.de>, eggert@cs.ucla.edu, emacs-devel@gnu.org
> 
> > >           if (XFIXNUM (repertory) == charset_id)
> > >
> > > Looks like repertory is nil.
> >
> > Yes, that part is clear.  The question is why...
> 
> Is there a problem simply testing FIXNUMP in this place? If I'm
> reading this correctly, repertory may be nil, but it might also be a
> char table, which might, by sheer accident, have XFIXNUM_RAW
> (repertory) == charset_id, so it looks like a bug that was caught, not
> caused, by the recent XFIXNUM change.

Yes, see what I eventually pushed (only now, as the gnu.org machines
went down yesterday when I needed them).

Thanks.

Re: Crashes in "C-h h"

[Martin]

Eli Zaretskii writes:

> I'm getting crashes in "C-h h", they are caused by some commit between
> e51f328 and cb182ce.  I suspect f59a3f3 and/or 4893a09.  The backtrace
> is below.
>
> Does anyone else see this, in particular on X?

GNU Emacs 26.2 (build 1, x86_64-pc-cygwin) of 2019-04-15 hangs;
no backtrace available

Re: Crashes in "C-h h"

[Eli Zaretskii]

> From: Martin <parozusa@web.de>
> Date: Tue, 02 Jul 2019 16:34:38 +0200
> 
> > I'm getting crashes in "C-h h", they are caused by some commit between
> > e51f328 and cb182ce.  I suspect f59a3f3 and/or 4893a09.  The backtrace
> > is below.
> >
> > Does anyone else see this, in particular on X?
> 
> GNU Emacs 26.2 (build 1, x86_64-pc-cygwin) of 2019-04-15 hangs;
> no backtrace available

That's unrelated: I was talking about the master branch, which is
Emacs 27.

Please report your hang as a separate bug report using
report-emacs-bug.  Maybe you could also try the pretest of what will
be Emacs 26.3 -- it could be that this problem is already fixed.

Thanks.

Re: Crashes in "C-h h"

[Paul Eggert]

Pip Cet wrote:
> Is there a problem simply testing FIXNUMP in this place?

As Eli's revised code suggests, if n is in fixnum range, then instead of 
(FIXNUMP (x) && XFIXNUM (x) == n) it's typically a bit cleaner (and faster) to 
write EQ (x, make_fixnum (n)).

Re: Crashes in "C-h h"

[Eli Zaretskii]

> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Tue, 2 Jul 2019 19:38:00 -0700
> Cc: emacs-devel@gnu.org
> 
> Pip Cet wrote:
> > Is there a problem simply testing FIXNUMP in this place?
> 
> As Eli's revised code suggests, if n is in fixnum range, then instead of 
> (FIXNUMP (x) && XFIXNUM (x) == n) it's typically a bit cleaner (and faster) to 
> write EQ (x, make_fixnum (n)).

It normally shouldn't matter either way, but in that case the
comparison is done in a loop, so the make_fixnum call can be done just
once, outside the loop, which AFAIU makes the loop a tad faster.

Crashes in "C-h h"

[Paul Eggert]

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

>> As Eli's revised code suggests, if n is in fixnum range, then instead of
>> (FIXNUMP (x) && XFIXNUM (x) == n) it's typically a bit cleaner (and faster) to
>> write EQ (x, make_fixnum (n)).

> It normally shouldn't matter either way, but in that case the
> comparison is done in a loop, so the make_fixnum call can be done just
> once, outside the loop, which AFAIU makes the loop a tad faster.

Although it indeed doesn't matter normally, the EQ+make_fixnum version 
should be smaller and faster in typical use, even without hoisting the 
make_fixnum out of a loop. I just now ran the attached microbenchmark on 
a Xeon E5-2640 v2 with code compiled by GCC 9.1 x86-64, and got these 
results:

1031-lnxsrv09 $ time ./a.out 0 0   # no operation

real	0m28.150s
user	0m28.148s
sys	0m0.001s
1032-lnxsrv09 $ time ./a.out 0 0 0   # FIXNUMP+XFIXNUM

real	0m34.229s
user	0m34.227s
sys	0m0.001s
1033-lnxsrv09 $ time ./a.out 0 0 0 0    # EQ+make_fixnum

real	0m32.213s
user	0m32.211s
sys	0m0.001s

which indicates that the EQ+make_fixnum version was about 50% faster 
than the FIXNUMP+XFIXNUM version, once you subtract the overhead of the 
no-op benchmark control.

[-- Attachment #2: t.c --]
[-- Type: text/x-csrc, Size: 499 bytes --]

#include <config.h>
#include <lisp.h>

int
noop (Lisp_Object x, ptrdiff_t n)
{
  return 0;
}

int
f (Lisp_Object x, ptrdiff_t n)
{
  return FIXNUMP (x) && XFIXNUM (x) == n;
}

int
g (Lisp_Object x, ptrdiff_t n)
{
  return EQ (x, make_fixnum (n));
}

int
main (int argc, char **argv)
{
  int sum = 0;
  int (*p) (Lisp_Object, ptrdiff_t)
    = argc % 3 == 0 ? noop : argc % 3 == 1 ? f : g;
  for (long i = 0; i < 10000000000; i++)
    sum += p (i & 1 ? Qnil : make_fixnum (i), i);
  return sum & 1;
}

Re: Crashes in "C-h h"

[Eli Zaretskii]

> From: Paul Eggert <eggert@cs.ucla.edu>
> Cc: pipcet@gmail.com, emacs-devel@gnu.org
> Date: Wed, 3 Jul 2019 14:05:32 -0700
> 
> 1031-lnxsrv09 $ time ./a.out 0 0   # no operation
> 
> real	0m28.150s
> user	0m28.148s
> sys	0m0.001s
> 1032-lnxsrv09 $ time ./a.out 0 0 0   # FIXNUMP+XFIXNUM
> 
> real	0m34.229s
> user	0m34.227s
> sys	0m0.001s
> 1033-lnxsrv09 $ time ./a.out 0 0 0 0    # EQ+make_fixnum
> 
> real	0m32.213s
> user	0m32.211s
> sys	0m0.001s
> 
> which indicates that the EQ+make_fixnum version was about 50% faster 
> than the FIXNUMP+XFIXNUM version, once you subtract the overhead of the 
> no-op benchmark control.

Is this with USE_LSB or without?  I think we need to time both
variants.

Re: Crashes in "C-h h"

[Pip Cet]

On Wed, Jul 3, 2019 at 9:05 PM Paul Eggert <eggert@cs.ucla.edu> wrote:
> >> As Eli's revised code suggests, if n is in fixnum range, then instead of
> >> (FIXNUMP (x) && XFIXNUM (x) == n) it's typically a bit cleaner (and faster) to
> >> write EQ (x, make_fixnum (n)).
>
> > It normally shouldn't matter either way, but in that case the
> > comparison is done in a loop, so the make_fixnum call can be done just
> > once, outside the loop, which AFAIU makes the loop a tad faster.
>
> Although it indeed doesn't matter normally, the EQ+make_fixnum version
> should be smaller and faster in typical use, even without hoisting the
> make_fixnum out of a loop.

When they're equivalent, EQ+make_fixnum seems more readable to me, too.

> I just now ran the attached microbenchmark on
> a Xeon E5-2640 v2 with code compiled by GCC 9.1 x86-64, and got these
> results:

Funnily, I get the same timings for each of the three versions on this machine:

model name    : Intel(R) Core(TM) i7-6820HQ CPU @ 2.70GHz

with trunk GCC.

But the assembly code generated is clearly better for the make_fixnum
version (as a nitpick, f and g aren't equivalent, but the code
produced is worse for f even with assume()s that make it equivalent to
g).

Re: Crashes in "C-h h"

[Paul Eggert]

Eli Zaretskii wrote:
> Is this with USE_LSB or without?  I think we need to time both
> variants.

That benchmark was with USE_LSB. I see no reason to think the !USE_LSB case 
would cause FIXNUMP+XFIXNUM to beat EQ+make_fixnum, but just to double-check 
here are the timings for that benchmark compiled with the additional GCC flags 
-m32 -DWIDE_EMACS_INT (the only significant !USE_LSB platform these days) and 
with 'long' replaced by 'long long' to get the loop to work on a 32-bit platform:

1030-lnxsrv09 $ time ./a.out 0 0

real	0m51.933s
user	0m51.927s
sys	0m0.001s
1031-lnxsrv09 $ time ./a.out 0 0 0

real	2m16.960s
user	2m16.960s
sys	0m0.000s
1032-lnxsrv09 $ time ./a.out 0 0 0 0

real	0m59.678s
user	0m59.677s
sys	0m0.001s

This indicates that on this !USE_LSB platform, the EQ+make_fixnum code is about 
ten times faster than the FIXNUMP+XFIXNUM code on this benchmark. I'm not 
surprised by the performance disparity, as branch-prediction failures can be 
pretty expensive.

Re: Crashes in "C-h h"

[Eli Zaretskii]

> Cc: pipcet@gmail.com, emacs-devel@gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Thu, 4 Jul 2019 00:15:12 -0700
> 
> 1030-lnxsrv09 $ time ./a.out 0 0
> 
> real	0m51.933s
> user	0m51.927s
> sys	0m0.001s
> 1031-lnxsrv09 $ time ./a.out 0 0 0
> 
> real	2m16.960s
> user	2m16.960s
> sys	0m0.000s
> 1032-lnxsrv09 $ time ./a.out 0 0 0 0
> 
> real	0m59.678s
> user	0m59.677s
> sys	0m0.001s

Thanks.

> This indicates that on this !USE_LSB platform, the EQ+make_fixnum code is about 
> ten times faster than the FIXNUMP+XFIXNUM code on this benchmark.

You mean twice faster (~2m vs ~1m), right?

> I'm not surprised by the performance disparity, as branch-prediction
> failures can be pretty expensive.

Which indeed means that if the object is not guaranteed to be a
fixnum, using XFIXNUM is generally suboptimal.

Re: Crashes in "C-h h"

[Pip Cet]

On Thu, Jul 4, 2019 at 1:06 PM Eli Zaretskii <eliz@gnu.org> wrote:
> > Cc: pipcet@gmail.com, emacs-devel@gnu.org
> > From: Paul Eggert <eggert@cs.ucla.edu>
> > Date: Thu, 4 Jul 2019 00:15:12 -0700
> >
> > 1030-lnxsrv09 $ time ./a.out 0 0
> >
> > real  0m51.933s
> > user  0m51.927s
> > sys   0m0.001s
> > 1031-lnxsrv09 $ time ./a.out 0 0 0
> >
> > real  2m16.960s
> > user  2m16.960s
> > sys   0m0.000s
> > 1032-lnxsrv09 $ time ./a.out 0 0 0 0
> >
> > real  0m59.678s
> > user  0m59.677s
> > sys   0m0.001s
>
> Thanks.

I'm seeing a much smaller slowdown with "perf stat": 12.3 cycles/loop
rather than 11.7.

> > This indicates that on this !USE_LSB platform, the EQ+make_fixnum code is about
> > ten times faster than the FIXNUMP+XFIXNUM code on this benchmark.
>
> You mean twice faster (~2m vs ~1m), right?

"g" is 8 seconds slower than "noop", but "f" is 76 seconds slower.

> > I'm not surprised by the performance disparity, as branch-prediction
> > failures can be pretty expensive.

Do you really see mispredicted branches in this simple example? I'm surprised.

I'm seeing a single branch in "f", which is well-predicted as it
alternates between being taken and not being taken.

> Which indeed means that if the object is not guaranteed to be a
> fixnum, using XFIXNUM is generally suboptimal.

XFIXNUM is half a clock cycle more expensive, maybe, in non-equivalent
code, on a peculiar platform. That's hardly "generally".

(It is a pity that gcc doesn't optimize bit field comparisons very
well, so we have to resort to the ((x-c) & mask) == 0 construction in
TAGGEDP. Maybe we could make it better by throwing in some eassumes,
but really gcc needs to recognize (x-c) & mask == 0 as equivalent to
(x & mask) == c (in the right conditions) and emit whichever variant
is faster.)

I think it would make most sense to introduce a macro for comparing a
Lisp object to a C integer, which does the right thing even outside
the fixnum range. Slower, but it would reduce our dependencies on the
fixnum range being large enough to hold all practical integers.

Re: Crashes in "C-h h"

[Paul Eggert]

Pip Cet wrote:

> I'm seeing a much smaller slowdown with "perf stat": 12.3 cycles/loop
> rather than 11.7.

It very much depends on the CPU. I got results all over the map when I used 
different CPUs. Sometimes there wasn't that much difference, sometimes more than 
what I mentioned. FIXNUMP+XFIXNUM was always slower than EQ+make_fixnum, though.

>> You mean twice faster (~2m vs ~1m), right?
> 
> "g" is 8 seconds slower than "noop", but "f" is 76 seconds slower.

My calculations have "f" 85 not 76 seconds slower, which means FIXNUMP+XFIXNUM 
was about 11x slower (10.972x times slower, to be absurdly precise).
> I'm seeing a single branch in "f", which is well-predicted as it
> alternates between being taken and not being taken.

Maybe my CPU isn't using 2-bit branch prediction for this case, whereas yours is.

> gcc needs to recognize (x-c) & mask == 0 as equivalent to
> (x & mask) == c (in the right conditions) and emit whichever variant
> is faster.)

Yes, this is the GCC performance bug I reported about a year ago 
<https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87104>. You came up with a GCC 
patch which you labeled "WIP". Maybe it's time to get that patch out the door?

> I think it would make most sense to introduce a macro for comparing a
> Lisp object to a C integer, which does the right thing even outside
> the fixnum range. Slower, ...

All this talk about optimization, and now you want things to be *slower*? :-)

Re: Crashes in "C-h h"

[Pip Cet]

On Thu, Jul 4, 2019 at 11:04 PM Paul Eggert <eggert@cs.ucla.edu> wrote:
> Pip Cet wrote:
>
> > I'm seeing a much smaller slowdown with "perf stat": 12.3 cycles/loop
> > rather than 11.7.
>
> It very much depends on the CPU. I got results all over the map when I used
> different CPUs. Sometimes there wasn't that much difference, sometimes more than
> what I mentioned. FIXNUMP+XFIXNUM was always slower than EQ+make_fixnum, though.

I don't see how it can be any faster. The code generated by
EQ+make_fixnum seems optimal to me, and the code for even an optimal
version of FIXNUMP+XFIXNUM would have to sign-extend the INTMASK bits
first.

> >> You mean twice faster (~2m vs ~1m), right?
> >
> > "g" is 8 seconds slower than "noop", but "f" is 76 seconds slower.

Oops, I should have said "again", here.

> My calculations have "f" 85 not 76 seconds slower, which means FIXNUMP+XFIXNUM
> was about 11x slower (10.972x times slower, to be absurdly precise).

> > I'm seeing a single branch in "f", which is well-predicted as it
> > alternates between being taken and not being taken.
>
> Maybe my CPU isn't using 2-bit branch prediction for this case, whereas yours is.

Maybe. Looking at the assembler code next to "perf stat" output would
be one way to find out (if you're running Linux).

> > gcc needs to recognize (x-c) & mask == 0 as equivalent to
> > (x & mask) == c (in the right conditions) and emit whichever variant
> > is faster.)
>
> Yes, this is the GCC performance bug I reported about a year ago
> <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87104>. You came up with a GCC
> patch which you labeled "WIP". Maybe it's time to get that patch out the door?

I sent the patch to gcc-patches, got no response, and then my old
computer broke, so part of the patch is now untestable.

I get the impression the GCC folks barely even get around to important
bug reports, so micro-optimization is probably a low priority.

> > I think it would make most sense to introduce a macro for comparing a
> > Lisp object to a C integer, which does the right thing even outside
> > the fixnum range. Slower, ...
>
> All this talk about optimization, and now you want things to be *slower*? :-)

I doubt it's actually going to be slower, since memory access to the
data probably dominates whatever register operations we do. The
important thing is not to have mispredicted branches...

Anyway, is there any reason not to have an assert in make_fixnum to
make sure the number fits?

Re: Crashes in "C-h h"

[Eli Zaretskii]

> From: Pip Cet <pipcet@gmail.com>
> Date: Fri, 5 Jul 2019 07:07:48 +0000
> Cc: emacs-devel@gnu.org
> 
> > >> You mean twice faster (~2m vs ~1m), right?
> > >
> > > "g" is 8 seconds slower than "noop", but "f" is 76 seconds slower.
> 
> Oops, I should have said "again", here.

Parse error.  What is "f", what is "g", where should I insert
"again".  Maybe ENOCOFFEE.

Re: Crashes in "C-h h"

[Pip Cet]

On Fri, Jul 5, 2019 at 7:52 AM Eli Zaretskii <eliz@gnu.org> wrote:
> > > > "g" is 8 seconds slower than "noop", but "f" is 76 seconds slower.
> >
> > Oops, I should have said "again", here.
>
> Parse error.  What is "f", what is "g", where should I insert
> "again".  Maybe ENOCOFFEE.

int
noop (Lisp_Object x, ptrdiff_t n)
{
  return 0;
}

int
f (Lisp_Object x, ptrdiff_t n)
{
  return FIXNUMP (x) && XFIXNUM (x) == n;
}

int
g (Lisp_Object x, ptrdiff_t n)
{
  return EQ (x, make_fixnum (n));
}

Using "g" is about 8 seconds slower than "noop". Using "f" is about 85
seconds slower than "noop". Using "f" is about 76 seconds slower than
"g".

Paul thinks those numbers are as expected. I'm very surprised the
difference is so pronounced for Paul, and can't reproduce it here, but
we all agree that EQ (x, make_fixnum (n)) is the right thing to do if
you are certain n is in the fixnum range.

Re: Crashes in "C-h h"

[Eli Zaretskii]

> From: Pip Cet <pipcet@gmail.com>
> Date: Fri, 5 Jul 2019 08:17:39 +0000
> Cc: eggert@cs.ucla.edu, emacs-devel@gnu.org
> 
> > Parse error.  What is "f", what is "g", where should I insert
> > "again".  Maybe ENOCOFFEE.
> 
> int
> noop (Lisp_Object x, ptrdiff_t n)
> {
>   return 0;
> }
> 
> int
> f (Lisp_Object x, ptrdiff_t n)
> {
>   return FIXNUMP (x) && XFIXNUM (x) == n;
> }
> 
> int
> g (Lisp_Object x, ptrdiff_t n)
> {
>   return EQ (x, make_fixnum (n));
> }

So you expected me to remember the function names by heart? ;-)

> Using "g" is about 8 seconds slower than "noop". Using "f" is about 85
> seconds slower than "noop". Using "f" is about 76 seconds slower than
> "g".

It is not useful to compare to a function that does nothing.  Useful
comparisons would be with functions that do this:

  return x == y;

or this:

  return XFIXNUM (a) == y;

where x and y are int's and a is a fixnum.

> we all agree that EQ (x, make_fixnum (n)) is the right thing to do if
> you are certain n is in the fixnum range.

That's not my conclusion.  (And if N is not in the fixnum range, why
would you call make_fixnum for it?)

My conclusion is that comparing with make_fixnum, i.e.

    EQ (x, make_fixnum (n))

is TRT when you are NOT certain that X is a fixnum, especially when
the comparison is done in a loop against many different values of X
and only one value of N, in which case make_fixnum can be called
outside of the loop.

If you ARE certain that X is a fixnum, then

    XFIXNUM (x) == n

is also OK.

The comparison with

    if (FIXNUMP (x) && XFIXNUM (x) == n)

is IMO not useful, because it should be clear up front that it will
always lose due to the additional test.

Re: Crashes in "C-h h"

[Paul Eggert]

Eli Zaretskii wrote:
> It is not useful to compare to a function that does nothing.

It's standard in benchmarking to do that, to try to distinguish the overhead of 
running the benchmark from the code one is actually trying to measure.

> Useful
> comparisons would be with functions that do this:
> 
>   return x == y;

That depends on what one means by "useful". I was trying to compare the 
performance of EQ+make_fixnum to that of FIXNUMP+XFIXNUM, not to that of ==.

> comparing with make_fixnum, i.e.
> 
>     EQ (x, make_fixnum (n))
> 
> is TRT when you are NOT certain that X is a fixnum ...
> If you ARE certain that X is a fixnum, then
> 
>     XFIXNUM (x) == n
> 
> is also OK.

Yes, that's right.

> The comparison with
> 
>     if (FIXNUMP (x) && XFIXNUM (x) == n)
> 
> is IMO not useful, because it should be clear up front that it will
> always lose due to the additional test.

I suspect much of this thread is due to a misunderstanding then, as I 
interpreted your earlier comment "It normally shouldn't matter either way" to 
mean the opposite, which is why I ran the benchmarks. Apparently my 
interpretation was a misunderstanding of what you intended. It is amusing that 
those benchmarks yield such wildly-different results on different CPUs, though.

Re: Crashes in "C-h h"

[Eli Zaretskii]

> Cc: emacs-devel@gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Fri, 5 Jul 2019 20:42:09 -0700
> 
> > The comparison with
> > 
> >     if (FIXNUMP (x) && XFIXNUM (x) == n)
> > 
> > is IMO not useful, because it should be clear up front that it will
> > always lose due to the additional test.
> 
> I suspect much of this thread is due to a misunderstanding then, as I 
> interpreted your earlier comment "It normally shouldn't matter either way" to 
> mean the opposite

Sorry about that.  I should have been clearer in my wording.

The original code there was just

  if (XFIXNUM (x) == y)

so that was my reference point for judging performance.

> It is amusing that those benchmarks yield such wildly-different
> results on different CPUs, though.

That's a mystery in itself, yes.  Whether we want to pursue it is
another matter, as it seems to involve subtle aspects of code
generation on modern CPUs, something we aren't the best specialists
in.

Re: Crashes in "C-h h"

[VanL]

Eli Zaretskii <eliz@gnu.org> writes:

>> It is amusing that those benchmarks yield such wildly-different
>> results on different CPUs, though.
>
> That's a mystery in itself, yes.  Whether we want to pursue it is
> another matter, as it seems to involve subtle aspects of code
> generation on modern CPUs, something we aren't the best specialists
> in.

Does the mystery lie below the assembly code in microcode?  A rule of
thump performant comparison between the latest Intel v. AMD offerings is
good to know.  In one market purchase orders are at parity 50-50.

Re: Crashes in "C-h h"

[Paul Eggert]

VanL wrote:
> Does the mystery lie below the assembly code in microcode?

Yes, much of it's there. CPUs differ in how they do branch prediction.

[OffTopic] Re: Crashes in "C-h h"

[VanL]

> On 7 Jul 2019, at 00:30, Paul Eggert <eggert@cs.ucla.edu> wrote:
> 
> CPUs differ in how they do branch prediction.

On a scale of zero to one where zero is your pdp-10 1966 one-megabyte-one-MIPS machine and where one is your pentium-m 2006 one-gigabyte-7400-MIPS machine, measuring raw branch prediction performance, where do you put along that line the 2019 Boeing CPU for MCAS fielded in the 737max8 and 2019 Tesla CPU for Autonomy fielded in the Model S?