Excessive use of `eassert`

Excessive use of `eassert`

[Stefan Monnier]

Building with ENABLE_CHECKING results in an Emacs that's
substantially slower.  To some extent, this is unavoidable, but

    cd .../src
    rm process.o
    make CFLAGS="-Winline -O2 -DHAVE_CONFIG_H" process.o |&
        grep make_lisp_symbol

shows that `make_lisp_symbol` is not inlined, so NILP(x) ends up being
an actual function call to a function calling another function ....
which I think is definitely in the "excessive" camp :-)

The patch below seems to address this specific issue, tho I haven't
measured its performance impact yet.


        Stefan


diff --git a/src/lisp.h b/src/lisp.h
index 914d6dd9b07..6b47b92972a 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -1176,7 +1176,7 @@ make_lisp_symbol (struct Lisp_Symbol *sym)
      cast to char * rather than to intptr_t.  */
   char *symoffset = (char *) ((char *) sym - (char *) lispsym);
   Lisp_Object a = TAG_PTR (Lisp_Symbol, symoffset);
-  eassert (XSYMBOL (a) == sym);
+  /* eassert (XSYMBOL (a) == sym); */
   return a;
 }
 

Re: Excessive use of `eassert`

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Thu, 18 Jan 2024 17:35:53 -0500
> 
> Building with ENABLE_CHECKING results in an Emacs that's
> substantially slower.  To some extent, this is unavoidable, but
> 
>     cd .../src
>     rm process.o
>     make CFLAGS="-Winline -O2 -DHAVE_CONFIG_H" process.o |&
>         grep make_lisp_symbol
> 
> shows that `make_lisp_symbol` is not inlined, so NILP(x) ends up being
> an actual function call to a function calling another function ....
> which I think is definitely in the "excessive" camp :-)

I'm not sure I follow.  Can you elaborate?  Are you saying that the
assertion causes make_lisp_symbol not to be inlined?  And what
functions are called by NILP?

> The patch below seems to address this specific issue, tho I haven't
> measured its performance impact yet.

Is this specifically about NILP?  Or are there other situations where
this assertion slows us down considerably.  I wouldn't want to drop
this assertion so summarily, if possible.

Thanks.

Re: Excessive use of `eassert`

[Stefan Monnier]

>> shows that `make_lisp_symbol` is not inlined, so NILP(x) ends up being
>> an actual function call to a function calling another function ....
>> which I think is definitely in the "excessive" camp :-)
>
> I'm not sure I follow.  Can you elaborate?  Are you saying that the
> assertion causes make_lisp_symbol not to be inlined?  And what
> functions are called by NILP?

AFAICT it's worse than just `NILP`, I think, because every `Qnil` (same
thing with all other `Q<something>`, I guess) becomes a call to
`builtin_lisp_symbol` which itself has a call to `make_lisp_symbol`.

>> The patch below seems to address this specific issue, tho I haven't
>> measured its performance impact yet.
> Is this specifically about NILP?  Or are there other situations where
> this assertion slows us down considerably.  I wouldn't want to drop
> this assertion so summarily, if possible.

Why do you find this specific assertion important?  When building other
`Lisp_Object`s (like `make_fixnum`) we don't seem to have any
corresponding assertion that the revere operation (e.g. XFIXNUM) returns
the original value.


        Stefan

Re: Excessive use of `eassert`

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: emacs-devel@gnu.org
> Date: Fri, 19 Jan 2024 08:01:47 -0500
> 
> >> shows that `make_lisp_symbol` is not inlined, so NILP(x) ends up being
> >> an actual function call to a function calling another function ....
> >> which I think is definitely in the "excessive" camp :-)
> >
> > I'm not sure I follow.  Can you elaborate?  Are you saying that the
> > assertion causes make_lisp_symbol not to be inlined?  And what
> > functions are called by NILP?
> 
> AFAICT it's worse than just `NILP`, I think, because every `Qnil` (same
> thing with all other `Q<something>`, I guess) becomes a call to
> `builtin_lisp_symbol` which itself has a call to `make_lisp_symbol`.

Then how come this is suddenly an issue?  We've had that assertion
since 2016.  I use an Emacs build with ENABLE_CHECKING and without
optimizations every day, and while it is indeed slower than the
production build by a factor of 3.5, it is not unbearably slow.

> >> The patch below seems to address this specific issue, tho I haven't
> >> measured its performance impact yet.
> > Is this specifically about NILP?  Or are there other situations where
> > this assertion slows us down considerably.  I wouldn't want to drop
> > this assertion so summarily, if possible.
> 
> Why do you find this specific assertion important?  When building other
> `Lisp_Object`s (like `make_fixnum`) we don't seem to have any
> corresponding assertion that the revere operation (e.g. XFIXNUM) returns
> the original value.

make_fixnum is a trivial bit-shuffling, whereas make_lisp_symbol is
much trickier.  Perhaps especially so now that we have
symbols-with-positions as well as bare symbols.

I have added Paul, who introduced that assertion (and the code around
it).  Paul, do you think this assertion is important to keep?

I also added Alan, in case he has comments.

Re: Excessive use of `eassert`

[Stefan Monnier]

>> AFAICT it's worse than just `NILP`, I think, because every `Qnil` (same
>> thing with all other `Q<something>`, I guess) becomes a call to
>> `builtin_lisp_symbol` which itself has a call to `make_lisp_symbol`.
>
> Then how come this is suddenly an issue?

Oh, I don't think it's a new issue (tho maybe it is, I have no idea).
I just happened to hit `C-z` in Emacs for some unrelated reason and saw
on the stack `make_lisp_symbol` and `builtin_lisp_symbol`, which seemed
odd, so I looked a bit closer.

> since 2016.  I use an Emacs build with ENABLE_CHECKING and without
> optimizations every day, and while it is indeed slower than the
> production build by a factor of 3.5, it is not unbearably slow.

Same here.  I can't vouch for 3.5 specifically, but 3-4 sounds about
right for me as well.  Tho I'll also note that (many) years ago
the slowdown was lower, more in the 2x ballpark.

>> Why do you find this specific assertion important?  When building other
>> `Lisp_Object`s (like `make_fixnum`) we don't seem to have any
>> corresponding assertion that the revere operation (e.g. XFIXNUM) returns
>> the original value.
> make_fixnum is a trivial bit-shuffling, whereas make_lisp_symbol is
> much trickier.

`make_fixnum` does "shift + tag", whereas `make_lisp_symbol` does "add +
tag".  Doesn't seem so terribly more tricky.

> Perhaps especially so now that we have symbols-with-positions as well
> as bare symbols.

`make_lisp_symbol` doesn't touch SWPs.

In any case, I'm not insisting.  I already removed that assertion from
my local branch, which is the one that affects me.  My messages was
mostly intended to share my discovery/surprise: I always assumed that
something like `Qnil` in the source would turn into some kind of
constant in the machine code (possibly modulo relocation), regardless of
ENABLE_CHECKING.


        Stefan

Re: Excessive use of `eassert`

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Paul Eggert <eggert@cs.ucla.edu>,  Alan Mackenzie <acm@muc.de>,
>   emacs-devel@gnu.org
> Date: Fri, 19 Jan 2024 10:50:51 -0500
> 
> > since 2016.  I use an Emacs build with ENABLE_CHECKING and without
> > optimizations every day, and while it is indeed slower than the
> > production build by a factor of 3.5, it is not unbearably slow.
> 
> Same here.  I can't vouch for 3.5 specifically, but 3-4 sounds about
> right for me as well.  Tho I'll also note that (many) years ago
> the slowdown was lower, more in the 2x ballpark.

It's possible that GCC optimizes better nowadays.  Also, back then we
had macros, not inline functions that sometimes aren't inlined.

> In any case, I'm not insisting.

Neither am I.

> I already removed that assertion from
> my local branch, which is the one that affects me.  My messages was
> mostly intended to share my discovery/surprise: I always assumed that
> something like `Qnil` in the source would turn into some kind of
> constant in the machine code (possibly modulo relocation), regardless of
> ENABLE_CHECKING.

Let's see what Paul and Alan think.

Re: Excessive use of `eassert`

[Stefan Monnier]

>> Same here.  I can't vouch for 3.5 specifically, but 3-4 sounds about
>> right for me as well.  Tho I'll also note that (many) years ago
>> the slowdown was lower, more in the 2x ballpark.
> It's possible that GCC optimizes better nowadays.  Also, back then we
> had macros, not inline functions that sometimes aren't inlined.

IIRC that ~2x factor was back when we used macros rather than inline
static functions in .h files, indeed.  The code has changed
significantly since.


        Stefan

Re: Excessive use of `eassert`

[Alan Mackenzie]

Hello, Eli.

On Fri, Jan 19, 2024 at 17:02:06 +0200, Eli Zaretskii wrote:
> > From: Stefan Monnier <monnier@iro.umontreal.ca>
> > Cc: emacs-devel@gnu.org
> > Date: Fri, 19 Jan 2024 08:01:47 -0500

> > >> shows that `make_lisp_symbol` is not inlined, so NILP(x) ends up being
> > >> an actual function call to a function calling another function ....
> > >> which I think is definitely in the "excessive" camp :-)

> > > I'm not sure I follow.  Can you elaborate?  Are you saying that the
> > > assertion causes make_lisp_symbol not to be inlined?  And what
> > > functions are called by NILP?

> > AFAICT it's worse than just `NILP`, I think, because every `Qnil` (same
> > thing with all other `Q<something>`, I guess) becomes a call to
> > `builtin_lisp_symbol` which itself has a call to `make_lisp_symbol`.

> Then how come this is suddenly an issue?  We've had that assertion
> since 2016.  I use an Emacs build with ENABLE_CHECKING and without
> optimizations every day, and while it is indeed slower than the
> production build by a factor of 3.5, it is not unbearably slow.

> > >> The patch below seems to address this specific issue, tho I haven't
> > >> measured its performance impact yet.
> > > Is this specifically about NILP?  Or are there other situations where
> > > this assertion slows us down considerably.  I wouldn't want to drop
> > > this assertion so summarily, if possible.

> > Why do you find this specific assertion important?  When building other
> > `Lisp_Object`s (like `make_fixnum`) we don't seem to have any
> > corresponding assertion that the revere operation (e.g. XFIXNUM) returns
> > the original value.

> make_fixnum is a trivial bit-shuffling, whereas make_lisp_symbol is
> much trickier.  Perhaps especially so now that we have
> symbols-with-positions as well as bare symbols.

Not really.  Symbols with positions don't belong in the obarray.  If they
somehow get there, then that's a bug to be fixed.

> I have added Paul, who introduced that assertion (and the code around
> it).  Paul, do you think this assertion is important to keep?

> I also added Alan, in case he has comments.

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: Excessive use of `eassert`

[Eli Zaretskii]

> Date: Fri, 19 Jan 2024 19:42:39 +0000
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>,
>  Paul Eggert <eggert@cs.ucla.edu>, emacs-devel@gnu.org
> From: Alan Mackenzie <acm@muc.de>
> 
> > > Why do you find this specific assertion important?  When building other
> > > `Lisp_Object`s (like `make_fixnum`) we don't seem to have any
> > > corresponding assertion that the revere operation (e.g. XFIXNUM) returns
> > > the original value.
> 
> > make_fixnum is a trivial bit-shuffling, whereas make_lisp_symbol is
> > much trickier.  Perhaps especially so now that we have
> > symbols-with-positions as well as bare symbols.
> 
> Not really.  Symbols with positions don't belong in the obarray.  If they
> somehow get there, then that's a bug to be fixed.

Does that mean you'd like the assertion to be left there, or does that
mean you don't care if it were removed?

Re: Excessive use of `eassert`

[Paul Eggert]

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

On 2024-01-19 11:42, Alan Mackenzie wrote:

> On Fri, Jan 19, 2024 at 17:02:06 +0200, Eli Zaretskii wrote:

>> make_fixnum is a trivial bit-shuffling, whereas make_lisp_symbol is
>> much trickier.  Perhaps especially so now that we have
>> symbols-with-positions as well as bare symbols.
> 
> Not really.  Symbols with positions don't belong in the obarray.  If they
> somehow get there, then that's a bug to be fixed.

The problem here isn't calling make_lisp_symbol to make symbols with 
positions. It's that Qnil expands to builtin_lisp_symbol (0) which calls 
make_lisp_symbol (&lispsym[0]) which calls XSYMBOL, and XSYMBOL is 
significantly slower now that we have symbols-with-positions, even when 
it is applied to a bare symbol - and this is particularly true with 
--enable-checking and the eassert Stefan mentioned.

I looked into this a bit and installed the attached patches which should 
speed things slightly even with a default build. The main goal was to 
speed up debugging builds, though.

When I built with --enable-checking this seemed to help significantly on 
Ubuntu 23.10 x86-64 with GCC 13.2 -O2 (at least looking at the machine 
code; I didn't benchmark). This depends on compiler and platform so it'd 
be helpful if Stefan could try it out on his machine and see whether it 
helps his cases' performance.

[-- Attachment #2: 0001-Simplify-and-tune-XSYMBOL.patch --]
[-- Type: text/x-patch, Size: 1374 bytes --]

From cf26f573162130fed73c6e5603cb58e158903add Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Sat, 20 Jan 2024 16:52:31 -0800
Subject: [PATCH 1/2] Simplify and tune XSYMBOL
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* src/lisp.h (XSYMBOL): Simplify and tune.  There is no need to
examine symbols_with_pos_enabled here, since the arg must be a symbol
so if it's not a bare symbol then it must be a symbol_with_pos;
and checking whether a symbol is bare is cheap.

With Ubuntu 23.10 on a Xeon W-1350, this shrank Emacs’s executable
text size by 0.1% and sped up a default build of all *.elc files by
0.4%.

Remove unnecessary eassert, since XBARE_SYMBOL and XSYMBOL_WITH_POS
have easserts that suffice.
---
 src/lisp.h | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/src/lisp.h b/src/lisp.h
index 20b28e93c8d..c3309c81a16 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -1156,10 +1156,7 @@ XBARE_SYMBOL (Lisp_Object a)
 INLINE struct Lisp_Symbol * ATTRIBUTE_NO_SANITIZE_UNDEFINED
 XSYMBOL (Lisp_Object a)
 {
-  eassert (SYMBOLP (a));
-  if (!symbols_with_pos_enabled || BARE_SYMBOL_P (a))
-    return XBARE_SYMBOL (a);
-  return XBARE_SYMBOL (XSYMBOL_WITH_POS (a)->sym);
+  return XBARE_SYMBOL (BARE_SYMBOL_P (a) ? a : XSYMBOL_WITH_POS (a)->sym);
 }
 
 INLINE Lisp_Object
-- 
2.40.1


[-- Attachment #3: 0002-Speed-up-make_lisp_symbol-when-debugging.patch --]
[-- Type: text/x-patch, Size: 909 bytes --]

From bdcd662a21f4c4265f704b69deb9cf277a663ea7 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Sat, 20 Jan 2024 16:52:31 -0800
Subject: [PATCH 2/2] Speed up make_lisp_symbol when debugging

* src/lisp.h (make_lisp_symbol): In eassert use XBARE_SYMBOL
rather than XSYMBOL.  This is safe because the symbol must be
bare.  The change speeds up make_lisp_symbol when debugging.
---
 src/lisp.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/lisp.h b/src/lisp.h
index c3309c81a16..f0beafba42c 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -1166,7 +1166,7 @@ make_lisp_symbol (struct Lisp_Symbol *sym)
      cast to char * rather than to intptr_t.  */
   char *symoffset = (char *) ((char *) sym - (char *) lispsym);
   Lisp_Object a = TAG_PTR (Lisp_Symbol, symoffset);
-  eassert (XSYMBOL (a) == sym);
+  eassert (XBARE_SYMBOL (a) == sym);
   return a;
 }
 
-- 
2.40.1

Re: Excessive use of `eassert`

[Eli Zaretskii]

> Date: Sat, 20 Jan 2024 17:41:57 -0800
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>, emacs-devel@gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> 
> When I built with --enable-checking this seemed to help significantly on 
> Ubuntu 23.10 x86-64 with GCC 13.2 -O2 (at least looking at the machine 
> code; I didn't benchmark). This depends on compiler and platform so it'd 
> be helpful if Stefan could try it out on his machine and see whether it 
> helps his cases' performance.

If you didn't benchmark, how did you see that these changes help
significantly the ENABLE_CHECKING build?

Thanks.

Re: Excessive use of `eassert`

[Paul Eggert]

On 2024-01-21 01:57, Eli Zaretskii wrote:
> If you didn't benchmark, how did you see that these changes help
> significantly the ENABLE_CHECKING build?

I looked at the machine code generated (x86-64, current Fedora). In some 
cases it was quite a bit shorter, e.g., eliminating calls to 
make_lisp_ptr. Evidently I didn't catch the cases that Stefan noticed, 
so I'll try to see what's up there.

Re: Excessive use of `eassert`

[Alan Mackenzie]

Hello, Paul.

On Sat, Jan 20, 2024 at 17:41:57 -0800, Paul Eggert wrote:
> On 2024-01-19 11:42, Alan Mackenzie wrote:

> > On Fri, Jan 19, 2024 at 17:02:06 +0200, Eli Zaretskii wrote:

> >> make_fixnum is a trivial bit-shuffling, whereas make_lisp_symbol is
> >> much trickier.  Perhaps especially so now that we have
> >> symbols-with-positions as well as bare symbols.

> > Not really.  Symbols with positions don't belong in the obarray.  If they
> > somehow get there, then that's a bug to be fixed.

> The problem here isn't calling make_lisp_symbol to make symbols with 
> positions. It's that Qnil expands to builtin_lisp_symbol (0) which calls 
> make_lisp_symbol (&lispsym[0]) which calls XSYMBOL, and XSYMBOL is 
> significantly slower now that we have symbols-with-positions, even when 
> it is applied to a bare symbol - and this is particularly true with 
> --enable-checking and the eassert Stefan mentioned.

> I looked into this a bit and installed the attached patches which should 
> speed things slightly even with a default build. The main goal was to 
> speed up debugging builds, though.

I don't think the first patch is correct.  With it applied, the code
no longer signals the error of a symbol with position being processed
when symbols_with_pos_enabled is false.  That's in the debug build, of
course.

The second patch seems OK.

> When I built with --enable-checking this seemed to help significantly on 
> Ubuntu 23.10 x86-64 with GCC 13.2 -O2 (at least looking at the machine 
> code; I didn't benchmark). This depends on compiler and platform so it'd 
> be helpful if Stefan could try it out on his machine and see whether it 
> helps his cases' performance.

The "significantly" here means the 0.4% speed up in the default build
that you mention in the first patch, does it?

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: Excessive use of `eassert`

[Paul Eggert]

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

On 2024-01-21 02:59, Alan Mackenzie wrote:

> I don't think the first patch is correct.  With it applied, the code
> no longer signals the error of a symbol with position being processed
> when symbols_with_pos_enabled is false.  That's in the debug build, of
> course.

Oh, I think I see what you mean: although the patch is correct when 
callers use XSYMBOL correctly, if a caller has a bug that causes it to 
call XSYMBOL on a symbol-with-position when symbols_with_pos_enabled is 
false, then because of the patch this bug in the caller won't be 
diagnosed by an eassert failure within XSYMBOL in a debug build.

I installed the attached to try to fix that. This patch doesn't affect 
regular builds and so should preserve the minor performance advantage on 
regular builds.


>> When I built with --enable-checking this seemed to help significantly on
>> Ubuntu 23.10 x86-64 with GCC 13.2 -O2 (at least looking at the machine
>> code; I didn't benchmark). This depends on compiler and platform so it'd
>> be helpful if Stefan could try it out on his machine and see whether it
>> helps his cases' performance.
> 
> The "significantly" here means the 0.4% speed up in the default build
> that you mention in the first patch, does it?

Actually I meant the debug build, where I didn't measure the speedup and 
just looked at a bit of machine code. But as Stefan mentioned there were 
still issues there. I hope my recent patch to builtin_lisp_symbol has 
fixed that.

I hope this set of patches speeds things up enough on debug builds that 
Stefan can get his work done. If not, there are other easserts we can 
tune or remove. (Or perhaps it'd be cheaper to buy Stefan a faster 
machine - I know I could use one....)

[-- Attachment #2: 0001-Add-an-eassert-back-to-XSYMBOL.patch --]
[-- Type: text/x-patch, Size: 1351 bytes --]

From 088afa7e2f08f4eb4e39aae5db4faa33857bf544 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Sun, 21 Jan 2024 20:34:03 -0800
Subject: [PATCH] Add an eassert back to XSYMBOL
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Problem reported by Alan Mackenzie in:
https://lists.gnu.org/r/emacs-devel/2024-01/msg00755.html
* src/lisp.h (XSYMBOL): If the arg is not a bare symbol, then
eassert (symbols_with_pos_enabled).  This shouldn’t affect code
generated for regular builds, and could catch caller errors in
debug builds.  For debug builds although this slows things down
XSYMBOL should still be faster than it was the day before
yesterday, as there’s still no need to eassert (SYMBOLP	(a)).
---
 src/lisp.h | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/src/lisp.h b/src/lisp.h
index 29d2a08785a..efdb3886141 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -1161,7 +1161,12 @@ XBARE_SYMBOL (Lisp_Object a)
 INLINE struct Lisp_Symbol * ATTRIBUTE_NO_SANITIZE_UNDEFINED
 XSYMBOL (Lisp_Object a)
 {
-  return XBARE_SYMBOL (BARE_SYMBOL_P (a) ? a : XSYMBOL_WITH_POS (a)->sym);
+  if (!BARE_SYMBOL_P (a))
+    {
+      eassert (symbols_with_pos_enabled);
+      a = XSYMBOL_WITH_POS (a)->sym;
+    }
+  return XBARE_SYMBOL (a);
 }
 
 INLINE Lisp_Object
-- 
2.40.1

Re: Excessive use of `eassert`

[Stefan Monnier]

> I hope this set of patches speeds things up enough on debug builds that
> Stefan can get his work done. If not, there are other easserts we can tune
> or remove. (Or perhaps it'd be cheaper to buy Stefan a faster machine -
> I know I could use one....)

I already have the fastest machine I could find (within the constraints
of having a screen tall enough that I can work comfortably but such
that it still fits in my backpack).
The result is a Thinkpad T61 🙁

Also, since the slowdown is in Emacs itself (which is severely
single-threaded) and not in Emacs's build, even the very fastest CPUs
aren't so terribly faster 🙁

Furthermore it's not just "a faster machine" but "faster machines" since
I use about 4 different machines on a regular basis.


        Stefan

Re: Excessive use of `eassert`

[Alan Mackenzie]

Hello, Paul.

On Sun, Jan 21, 2024 at 21:19:30 -0800, Paul Eggert wrote:
> On 2024-01-21 02:59, Alan Mackenzie wrote:

> > I don't think the first patch is correct.  With it applied, the code
> > no longer signals the error of a symbol with position being processed
> > when symbols_with_pos_enabled is false.  That's in the debug build, of
> > course.

> Oh, I think I see what you mean: although the patch is correct when 
> callers use XSYMBOL correctly, ....

No, the patch is wrong.  If things only had to work when callers call
things correctly, we could scrap an awful lot more error checking code
and speed Emacs up enormously.

> .... if a caller has a bug that causes it to call XSYMBOL on a
> symbol-with-position when symbols_with_pos_enabled is false, then
> because of the patch this bug in the caller won't be diagnosed by an
> eassert failure within XSYMBOL in a debug build.

Yes.  This is a bug.

> I installed the attached to try to fix that. This patch doesn't affect 
> regular builds and so should preserve the minor performance advantage on 
> regular builds.

Your latest patch doesn't fix it.  In a regular build, symbols with
position are handled differently depending on the value of
symbols_with_pos_enabled.  Or at least they were and they must be.  Thus
in XBARE_SYMBOL, one MUST test s_w_p_e, regardless of whether the build
is a regular one or a debugging one.

Please fix this.

[ .... ]

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: Excessive use of `eassert`

[Paul Eggert]

On 2024-01-22 06:37, Alan Mackenzie wrote:
> In a regular build, symbols with
> position are handled differently depending on the value of
> symbols_with_pos_enabled.  Or at least they were and they must be.  Thus
> in XBARE_SYMBOL, one MUST test s_w_p_e, regardless of whether the build
> is a regular one or a debugging one.

None of my recent patches changed XBARE_SYMBOL. If XBARE_SYMBOL should 
test something it isn't currently testing, then that is a bug that has 
been present for some time.

However, I don't see a bug there. XBARE_SYMBOL should be used only on 
bare symbols, regardless of whether symbols_with_pos_enabled is set.

Perhaps you meant to write XSYMBOL instead of XBARE_SYMBOL? If so, I'm 
still not following, and more explanation would be helpful. What call to 
XSYMBOL formerly would have an eassert failure when debugging, but now 
won't have that failure?

Re: Excessive use of `eassert`

[Alan Mackenzie]

Hello, Paul.

On Mon, Jan 22, 2024 at 23:51:50 -0800, Paul Eggert wrote:
> On 2024-01-22 06:37, Alan Mackenzie wrote:
> > In a regular build, symbols with
> > position are handled differently depending on the value of
> > symbols_with_pos_enabled.  Or at least they were and they must be.  Thus
> > in XBARE_SYMBOL, one MUST test s_w_p_e, regardless of whether the build
> > is a regular one or a debugging one.

> None of my recent patches changed XBARE_SYMBOL. If XBARE_SYMBOL should 
> test something it isn't currently testing, then that is a bug that has 
> been present for some time.

> However, I don't see a bug there. XBARE_SYMBOL should be used only on 
> bare symbols, regardless of whether symbols_with_pos_enabled is set.

> Perhaps you meant to write XSYMBOL instead of XBARE_SYMBOL?

Yes I did.  Apologies.

> If so, I'm still not following, and more explanation would be helpful.
> What call to XSYMBOL formerly would have an eassert failure when
> debugging, but now won't have that failure?

It's not about the debug build, but a normal build, where easserts don't
play a role.

The scenario is when symbols_with_pos_enabled is false, and XSYMBOL is
invalidly given a symbol with position as argument.  Before your
patches, this would lead to an exception.  After your patches, this no
longer happens.  This is what I'm asking you to fix.

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: Excessive use of `eassert`

[Paul Eggert]

On 1/23/24 03:42, Alan Mackenzie wrote:
> It's not about the debug build, but a normal build, where easserts don't
> play a role.
> 
> The scenario is when symbols_with_pos_enabled is false, and XSYMBOL is
> invalidly given a symbol with position as argument.  Before your
> patches, this would lead to an exception.  After your patches, this no
> longer happens.

I don't see a problem there. XSYMBOL can be called only on symbols, 
regardless of whether symbols-with-pos-enabled is true. When 
symbols-with-pos-enabled is false, XSYMBOL's argument therefore cannot 
be a symbol with position as these are symbols only when 
symbol-with-pos-enabled is true.

It sounds like you're thinking that, even in normal builds (i.e., 
without --enable-debugging), XSYMBOL's implementation should have some 
sort of debugging code that is not needed for correct behavior and that 
can slow down execution. I don't see why that needs to happen. In normal 
builds, XSYMBOL does not check that its argument is a symbol, and it has 
undefined behavior if buggy C code gives it a non-symbol. As I 
understand it, a symbol with position SP is not a symbol if 
symbols-with-pos-enabled is false. This means it's OK if XSYMBOL (SP) 
has undefined behavior when symbols-with-pos-enabled is false in a 
normal build.

Here's an example to try to make my meaning clearer. Suppose we define a 
buggy Lisp function in C as follows:

   DEFUN ("buggy-symbol-name", Fbuggy_symbol_name,
          Sbuggy_symbol_name, 1, 1, 0,
          doc: /* Return SYMBOL's name, a string.  */)
     (Lisp_Object symbol)
   {
     return XSYMBOL (symbol)->u.s.name;
   }

This C function has a bug: it doesn't check that its argument is a 
symbol before calling XSYMBOL. And because of the bug, this:

   (buggy-symbol-name (position-symbol nil 1))

has undefined behavior when Emacs is built without --enable-debugging. 
In the current implementation this undefined behavior causes 
buggy-symbol-name to return "nil", whereas it would be nicer in some 
sense if Emacs crashed and dumped core due to the bug in the C code.

However, symbols with positions aren't necessarily being treated 
differently from other Lisp objects here. For example, this:

   (buggy-symbol-name "abc")

also has undefined behavior, and on my platform this latter expression 
returns nil, also without dumping core. Although it would also be nicer 
in some sense if Emacs crashed and dumped core due to the bug in the C 
code, Emacs doesn't do that, for valid performance reasons.

For Emacs to reliably crash right away in situations like these, it 
needs to be built with --enable-debugging.

Re: Excessive use of `eassert`

[Alan Mackenzie]

Hello, Paul.

On Tue, Jan 23, 2024 at 17:04:44 -0800, Paul Eggert wrote:
> On 1/23/24 03:42, Alan Mackenzie wrote:
> > It's not about the debug build, but a normal build, where easserts don't
> > play a role.

> > The scenario is when symbols_with_pos_enabled is false, and XSYMBOL is
> > invalidly given a symbol with position as argument.  Before your
> > patches, this would lead to an exception.  After your patches, this no
> > longer happens.

> I don't see a problem there. XSYMBOL can be called only on symbols, 
> regardless of whether symbols-with-pos-enabled is true. When 
> symbols-with-pos-enabled is false, XSYMBOL's argument therefore cannot 
> be a symbol with position as these are symbols only when 
> symbol-with-pos-enabled is true.

That's humpty-dumpty logic.  Just because s-w-p-e is false is no
guarantee that XSYMBOL won't get a SWP as argument.

> It sounds like you're thinking that, even in normal builds (i.e., 
> without --enable-debugging), XSYMBOL's implementation should have some 
> sort of debugging code that is not needed for correct behavior and that 
> can slow down execution.

No, I'm saying that an invalid bit pattern in XSYMBOL's logic should not
be treated as though it were valid.  No other invalid bit pattern allows
Emacs to continue blithely unaware.

And as for "can slow down execution", your measurement of 0.4%, if it's
not just background noise, is microscopic.  That would be less than a
second in a 4 minute bootstrap build.

> I don't see why that needs to happen. In normal builds, XSYMBOL does
> not check that its argument is a symbol, and it has undefined behavior
> if buggy C code gives it a non-symbol.

In normal builds, an invalid argument leads to a crash.  After your
patches, it only sometimes leads to a crash.  This is a loss of
functionality.

Why did you not discuss this change in functionality on the list
_before_ making it?

> As I understand it, a symbol with position SP is not a symbol if
> symbols-with-pos-enabled is false.  This means it's OK if XSYMBOL (SP)
> has undefined behavior when symbols-with-pos-enabled is false in a
> normal build.

The behaviour is not formally "undefined" and never has been.  It is not
formally anything (which is why we're arguing about it).  You've changed
the definition to "behaves as though symbols_with_pos_enabled were
true".  This breaks the consistency of the symbols with position
mechanism.

All for a barely measurable speed up.

Or should we just accept SWP everywhere, and abolish
symbols_with_pos_enabled?  That would at least preserve consistency,
though I don't think it would be a good thing to do.

[ .... ]

> However, symbols with positions aren't necessarily being treated 
> differently from other Lisp objects here. For example, this:

>    (buggy-symbol-name "abc")

> also has undefined behavior, and on my platform this latter expression 
> returns nil, also without dumping core.

That feels like a bug.

[ .... ]

> For Emacs to reliably crash right away in situations like these, it 
> needs to be built with --enable-debugging.

That is impractical for most developers due to the vast slowdown (not
just 0.4%) it causes.

Again, please restore the functionality you've ripped out.

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: Excessive use of `eassert`

[Paul Eggert]

On 2024-01-24 07:09, Alan Mackenzie wrote:
> The behaviour is not formally "undefined" and never has been.

Sure it is. In C, if you dereference a bad pointer the behavior is 
undefined, which means anything can happen. On common platforms, 
sometimes you'll get a crash and sometimes (as my example showed) you 
won't, and this is true regardless of whether symbols-with-pos-enabled 
is true. There has never been a guarantee in a normal build that XSYMBOL 
(S) crashes when S is not a symbol, and none of the recent changes have 
altered this fact about XSYMBOL.

Re: Excessive use of `eassert`

[Stefan Monnier]

> I looked into this a bit and installed the attached patches which should
> speed things slightly even with a default build.

Nice.

> The main goal was to speed up debugging builds, though.

I still get:

    (gdb) x/12i init_window
       0xbbabc <init_window>:       push   %ebx
       0xbbabd <init_window+1>:     sub    $0x18,%esp
       0xbbac0 <init_window+4>:     call   0x235b0 <__x86.get_pc_thunk.bx>
       0xbbac5 <init_window+9>:     add    $0x34652f,%ebx
       0xbbacb <init_window+15>:    lea    0xc(%esp),%eax
       0xbbacf <init_window+19>:    lea    0x383d4c(%ebx),%edx
       0xbbad5 <init_window+25>:    call   0xa2d57 <make_lisp_symbol>
       0xbbada <init_window+30>:    mov    0xc(%esp),%eax
       0xbbade <init_window+34>:    mov    %eax,0x334230(%ebx)
       0xbbae4 <init_window+40>:    add    $0x18,%esp
       0xbbae7 <init_window+43>:    pop    %ebx
       0xbbae8 <init_window+44>:    ret
    (gdb)

whereas with my patch this goes down to:

    (gdb) x/4i init_window
       0xb1715 <init_window>:       call   0x2ff9e <__x86.get_pc_thunk.ax>
       0xb171a <init_window+5>:     add    $0x31c8da,%eax
       0xb171f <init_window+10>:    movl   $0x0,0xe9d0(%eax)
       0xb1729 <init_window+20>:    ret
    (gdb)

🙁


        Stefan

Re: Excessive use of `eassert`

[Paul Eggert]

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

On 2024-01-21 07:54, Stefan Monnier wrote:
> I still get:
> 
>      (gdb) x/12i init_window
> ...
>         0xbbad5 <init_window+25>:    call   0xa2d57 <make_lisp_symbol>

Thanks, I reproduced that on Ubuntu 23.10 on x86-64 by configuring with 
--enable-checking and compiling with gcc -O0.

I installed the attached patch to try to speed things up for that setup.

[-- Attachment #2: 0001-Speed-up-builtin_lisp_symbol-when-not-optimizing.patch --]
[-- Type: text/x-patch, Size: 2170 bytes --]

From df7c6211cb960b88bc0aaef85babf7e9384d5f2e Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Sun, 21 Jan 2024 17:18:23 -0800
Subject: [PATCH] Speed up builtin_lisp_symbol when not optimizing
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This should help when building with --enable-checking and
compiling with gcc -O0.  Problem reorted by Stefan Monnier in:
https://lists.gnu.org/r/emacs-devel/2024-01/msg00770.html
* src/lisp.h (lisp_h_builtin_lisp_symbol): New macro,
with a body equivalent in effect to the old ‘builtin_lisp_symbol’
but faster when not optimizing.
(builtin_lisp_symbol): Use it.
If DEFINE_KEY_OPS_AS_MACROS, also define as macro.
---
 src/lisp.h | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/src/lisp.h b/src/lisp.h
index ae78947805e..29d2a08785a 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -409,6 +409,10 @@ #define lisp_h_FIXNUMP(x) \
        & ((1 << INTTYPEBITS) - 1)))
 #define lisp_h_FLOATP(x) TAGGEDP (x, Lisp_Float)
 #define lisp_h_NILP(x)  BASE_EQ (x, Qnil)
+/* Equivalent to "make_lisp_symbol (&lispsym[INDEX])",
+   and typically faster when compiling without optimization.  */
+#define lisp_h_builtin_lisp_symbol(index) \
+  TAG_PTR (Lisp_Symbol, (index) * sizeof *lispsym)
 #define lisp_h_SET_SYMBOL_VAL(sym, v) \
    (eassert ((sym)->u.s.redirect == SYMBOL_PLAINVAL), \
     (sym)->u.s.val.value = (v))
@@ -475,6 +479,7 @@ #define lisp_h_XHASH(a) XUFIXNUM_RAW (a)
 # define FLOATP(x) lisp_h_FLOATP (x)
 # define FIXNUMP(x) lisp_h_FIXNUMP (x)
 # define NILP(x) lisp_h_NILP (x)
+# define builtin_lisp_symbol(index) lisp_h_builtin_lisp_symbol (index)
 # define SET_SYMBOL_VAL(sym, v) lisp_h_SET_SYMBOL_VAL (sym, v)
 # define SYMBOL_CONSTANT_P(sym) lisp_h_SYMBOL_CONSTANT_P (sym)
 # define SYMBOL_TRAPPED_WRITE_P(sym) lisp_h_SYMBOL_TRAPPED_WRITE_P (sym)
@@ -1171,9 +1176,9 @@ make_lisp_symbol (struct Lisp_Symbol *sym)
 }
 
 INLINE Lisp_Object
-builtin_lisp_symbol (int index)
+(builtin_lisp_symbol) (int index)
 {
-  return make_lisp_symbol (&lispsym[index]);
+  return lisp_h_builtin_lisp_symbol (index);
 }
 
 INLINE bool
-- 
2.40.1

Re: Excessive use of `eassert`

[Stefan Monnier]

> Thanks, I reproduced that on Ubuntu 23.10 on x86-64 by configuring
> with --enable-checking and compiling with gcc -O0.

FWIW, I never compile with `-O0`, it's usually`-Og`.
And on i386 I saw it with `-O2` as well :-(

> +/* Equivalent to "make_lisp_symbol (&lispsym[INDEX])",
> +   and typically faster when compiling without optimization.  */
> +#define lisp_h_builtin_lisp_symbol(index) \
> +  TAG_PTR (Lisp_Symbol, (index) * sizeof *lispsym)
[...]
> +(builtin_lisp_symbol) (int index)
>  {
> -  return make_lisp_symbol (&lispsym[index]);
> +  return lisp_h_builtin_lisp_symbol (index);
>  }

Hmm... Compared to the patch I proposed this has the downside that it
duplicates the logic between `lisp_h_builtin_lisp_symbol` and
`make_lisp_symbol` (worse: the logic is actually not the same, so it's
not obvious that it ends up returning the same thing).


        Stefan

Re: Excessive use of `eassert`

[Paul Eggert]

On 2024-01-22 05:20, Stefan Monnier wrote:
> Compared to the patch I proposed this has the downside that it
> duplicates the logic between `lisp_h_builtin_lisp_symbol` and
> `make_lisp_symbol`
Yes, when --enable-checking is used the tradeoff is: would we rather 
omit all runtime checking in make_lisp_symbol (the patch you proposed), 
or omit it only for builtin symbols (the patch I installed)?

For builtin symbols like Qnil and Qt the runtime checking is not all 
that useful - if these symbols' data items are improperly aligned Emacs 
will crash early anyway. For non-builtin symbols the runtime checking is 
arguably useful, to catch (presumably rare) alignment bugs in the memory 
allocator.

If you'd rather have the simpler solution that doesn't catch alignment 
bugs in non-builtin symbols, I could prepare a patch along those lines. 
Any such alignment bugs are pretty unlikely, after all.


>  (worse: the logic is actually not the same, so it's
>> not obvious that it ends up returning the same thing).

I used that logic to convince gcc to optimize builtin symbols like Qt 
and Qnil into integer constants at the machine level even when -O0 is 
used. But if nobody needs that sort of performance with -O0, we could go 
back to the old way of doing things. (It's obvious to *me* that the two 
formulations are equivalent but hey! only four people in the world 
understand lisp.h and nobody knows which four people they are....)

Re: Excessive use of `eassert`

[Stefan Monnier]

BTW, gcc refuses to compile the current code on my i386 machine.
It gives me loads of things like:

    lisp.h:620:28: error: expected expression before ‘{’ token
      620 | # define LISP_INITIALLY(w) {w}
          |                            ^
    lisp.h:941:3: note: in expansion of macro ‘LISP_INITIALLY’
      941 |   LISP_INITIALLY ((Lisp_Word) ((uintptr_t) (ptr) + LISP_WORD_TAG (tag)))
          |   ^~~~~~~~~~~~~~
    lisp.h:415:3: note: in expansion of macro ‘TAG_PTR’
      415 |   TAG_PTR (Lisp_Symbol, (index) * sizeof *lispsym)
          |   ^~~~~~~
    lisp.h:482:37: note: in expansion of macro ‘lisp_h_builtin_lisp_symbol’
      482 | # define builtin_lisp_symbol(index) lisp_h_builtin_lisp_symbol (index)
          |                                     ^~~~~~~~~~~~~~~~~~~~~~~~~~
    ./globals.h:7597:14: note: in expansion of macro ‘builtin_lisp_symbol’
     7597 | #define Qnil builtin_lisp_symbol (0)
          |              ^~~~~~~~~~~~~~~~~~~
    dispnew.c:6110:39: note: in expansion of macro ‘Qnil’
     6110 |       file = Fexpand_file_name (file, Qnil);
          |                                       ^~~~

How 'bout the patch below.  It gives the expected optimization under
`-O2` and `-Og` and avoids duplicating the logic.  It doesn't give the
expected optimization under `-O0` because `-O0` doesn't do any inlining,
but we've lived with that for a while so maybe that's OK (admittedly,
I'm biased since I almost never use `-O0`).


        Stefan


diff --git a/src/lisp.h b/src/lisp.h
index 54d2f4d3dd1..74e4a78ae26 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -409,11 +409,7 @@ #define lisp_h_FIXNUMP(x) \
        & ((1 << INTTYPEBITS) - 1)))
 #define lisp_h_FLOATP(x) TAGGEDP (x, Lisp_Float)
 #define lisp_h_NILP(x)  BASE_EQ (x, Qnil)
-/* Equivalent to "make_lisp_symbol (&lispsym[INDEX])",
-   and typically faster when compiling without optimization.  */
-#define lisp_h_builtin_lisp_symbol(index) \
-  TAG_PTR (Lisp_Symbol, (index) * sizeof *lispsym)
-#define lisp_h_SET_SYMBOL_VAL(sym, v) \
+#define lisp_h_SET_SYMBOL_VAL(sym, v)                 \
    (eassert ((sym)->u.s.redirect == SYMBOL_PLAINVAL), \
     (sym)->u.s.val.value = (v))
 #define lisp_h_SYMBOL_CONSTANT_P(sym) \
@@ -479,7 +475,6 @@ #define lisp_h_XHASH(a) XUFIXNUM_RAW (a)
 # define FLOATP(x) lisp_h_FLOATP (x)
 # define FIXNUMP(x) lisp_h_FIXNUMP (x)
 # define NILP(x) lisp_h_NILP (x)
-# define builtin_lisp_symbol(index) lisp_h_builtin_lisp_symbol (index)
 # define SET_SYMBOL_VAL(sym, v) lisp_h_SET_SYMBOL_VAL (sym, v)
 # define SYMBOL_CONSTANT_P(sym) lisp_h_SYMBOL_CONSTANT_P (sym)
 # define SYMBOL_TRAPPED_WRITE_P(sym) lisp_h_SYMBOL_TRAPPED_WRITE_P (sym)
@@ -1169,13 +1164,24 @@ XSYMBOL (Lisp_Object a)
   return XBARE_SYMBOL (a);
 }
 
+/* Apparently the `eassert` in `make_lisp_symbol` makes it non-inlinable
+   for GCC under -Og and even -O2, so use a non-checking version
+   for builtin symbols, so "apparent constants" like `Qnil` are indeed
+   compiled as constants.  */
 INLINE Lisp_Object
-make_lisp_symbol (struct Lisp_Symbol *sym)
+make__lisp_symbol (struct Lisp_Symbol *sym)
 {
   /* GCC 7 x86-64 generates faster code if lispsym is
      cast to char * rather than to intptr_t.  */
   char *symoffset = (char *) ((char *) sym - (char *) lispsym);
   Lisp_Object a = TAG_PTR (Lisp_Symbol, symoffset);
+  return a;
+}
+
+INLINE Lisp_Object
+make_lisp_symbol (struct Lisp_Symbol *sym)
+{
+  Lisp_Object a = make__lisp_symbol (sym);
   eassert (XBARE_SYMBOL (a) == sym);
   return a;
 }
@@ -1183,7 +1189,7 @@ make_lisp_symbol (struct Lisp_Symbol *sym)
 INLINE Lisp_Object
 (builtin_lisp_symbol) (int index)
 {
-  return lisp_h_builtin_lisp_symbol (index);
+  return make__lisp_symbol (&lispsym[index]);
 }
 
 INLINE bool

Re: Excessive use of `eassert`

[Paul Eggert]

On 2024-01-23 09:11, Stefan Monnier wrote:
> How 'bout the patch below.

Thanks, looks good, I installed it with minor changes (notably, renamed 
the new make__lisp_symbol to make_lisp_symbol_internal).

I occasionally compile with -O0 on an older (circa 2010) CPU. If the 
newly-installed code turns out to be too slow for that in some cases, we 
can turn builtin_lisp_symbol and make_lisp_symbol_internal into lisp_h_ 
macros in the same style as the existing macros. This would avoid the 
problem that my previous patch had with dueling implementations.

Re: Excessive use of `eassert`

[Eli Zaretskii]

> Date: Tue, 23 Jan 2024 00:15:03 -0800
> Cc: Alan Mackenzie <acm@muc.de>, Eli Zaretskii <eliz@gnu.org>,
>  emacs-devel@gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> 
> On 2024-01-22 05:20, Stefan Monnier wrote:
> > Compared to the patch I proposed this has the downside that it
> > duplicates the logic between `lisp_h_builtin_lisp_symbol` and
> > `make_lisp_symbol`
> Yes, when --enable-checking is used the tradeoff is: would we rather 
> omit all runtime checking in make_lisp_symbol (the patch you proposed), 
> or omit it only for builtin symbols (the patch I installed)?
> 
> For builtin symbols like Qnil and Qt the runtime checking is not all 
> that useful - if these symbols' data items are improperly aligned Emacs 
> will crash early anyway. For non-builtin symbols the runtime checking is 
> arguably useful, to catch (presumably rare) alignment bugs in the memory 
> allocator.
> 
> If you'd rather have the simpler solution that doesn't catch alignment 
> bugs in non-builtin symbols, I could prepare a patch along those lines. 
> Any such alignment bugs are pretty unlikely, after all.

I think we should settle for builtin symbols, since the other kind is
unlikely to cause any significant slowdown, and the extra level of
testing is valuable in debug builds.

I hope Stefan will agree.

Re: Excessive use of `eassert`

[Stefan Monnier]

> I think we should settle for builtin symbols, since the other kind is
> unlikely to cause any significant slowdown, and the extra level of
> testing is valuable in debug builds.

Fine by me, yes.


        Stefan