GNU bug report logs - #55676
[PATCH] non-recursive Lisp reader

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 55676 in the body.
You can then email your comments to 55676 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Mattias Engdegård <mattiase <at> acm.org>
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] non-recursive Lisp reader
Date: Fri, 27 May 2022 15:33:35 +0200

[Message part 1 (text/plain, inline)]

You probably saw this one coming. It's a restructuring of the Lisp reader so that it no longer uses recursion for reading nested data structures.

The primary motivation is eliminating the limitation of the C stack (and potential overflow crash). As a happy side-effect, the change improves reader performance by a few percent, exact amount depending on what is being read. This translates into a small but measurable speed-up in loading packages (.el and .elc), and in byte-compilation. The performance increase is both from removal of recursion and closer attention to performance.

Care has been taken to not change the reader behaviour, although some error handling may differ in unimportant ways. Some obvious bugs found during the conversion have been fixed: for example, #_ followed by whitespace now represents the interned empty symbol; previously, it gave an unintended empty symbol. A non-breaking space after a single dot now results in the dot token instead of the dot as a symbol.

[lread-nonrec.diff (application/octet-stream, attachment)]

Message #8 received at 55676 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Mattias Engdegård <mattiase <at> acm.org>
Cc: 55676 <at> debbugs.gnu.org
Subject: Re: bug#55676: [PATCH] non-recursive Lisp reader
Date: Fri, 27 May 2022 16:03:32 +0200

Mattias Engdegård <mattiase <at> acm.org> writes:

> You probably saw this one coming.

😀

> It's a restructuring of the Lisp reader so that it no longer uses
> recursion for reading nested data structures.

Wow, cool.

> The primary motivation is eliminating the limitation of the C stack
> (and potential overflow crash). As a happy side-effect, the change
> improves reader performance by a few percent, exact amount depending
> on what is being read. This translates into a small but measurable
> speed-up in loading packages (.el and .elc), and in
> byte-compilation. The performance increase is both from removal of
> recursion and closer attention to performance.

Sounds excellent.

> Care has been taken to not change the reader behaviour, although some
> error handling may differ in unimportant ways. Some obvious bugs found
> during the conversion have been fixed: for example, #_ followed by
> whitespace now represents the interned empty symbol; previously, it
> gave an unintended empty symbol. A non-breaking space after a single
> dot now results in the dot token instead of the dot as a symbol.

Hm...  sounds logical, I think.

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

Message #11 received at 55676 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Mattias Engdegård <mattiase <at> acm.org>
Cc: 55676 <at> debbugs.gnu.org
Subject: Re: bug#55676: [PATCH] non-recursive Lisp reader
Date: Fri, 27 May 2022 19:01:54 +0300

> From: Mattias Engdegård <mattiase <at> acm.org>
> Date: Fri, 27 May 2022 15:33:35 +0200
> 
> You probably saw this one coming. It's a restructuring of the Lisp reader so that it no longer uses recursion for reading nested data structures.
> 
> The primary motivation is eliminating the limitation of the C stack (and potential overflow crash). As a happy side-effect, the change improves reader performance by a few percent, exact amount depending on what is being read. This translates into a small but measurable speed-up in loading packages (.el and .elc), and in byte-compilation. The performance increase is both from removal of recursion and closer attention to performance.

Thanks.  One minor nit: you seem to have customized indentation in C
source files in a way that is different from our conventions.  Please
reformat using our defaults (which use a mix of TABs and SPACEs).

Message #14 received at 55676 <at> debbugs.gnu.org (full text, mbox):

From: Mattias Engdegård <mattiase <at> acm.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 55676 <at> debbugs.gnu.org
Subject: Re: bug#55676: [PATCH] non-recursive Lisp reader
Date: Fri, 27 May 2022 18:49:49 +0200

27 maj 2022 kl. 18.01 skrev Eli Zaretskii <eliz <at> gnu.org>:

> One minor nit: you seem to have customized indentation in C
> source files in a way that is different from our conventions.  Please
> reformat using our defaults (which use a mix of TABs and SPACEs).

Sure, but I thought I used the standard settings. Where did I err?

Message #17 received at 55676 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Mattias Engdegård <mattiase <at> acm.org>
Cc: 55676 <at> debbugs.gnu.org
Subject: Re: bug#55676: [PATCH] non-recursive Lisp reader
Date: Fri, 27 May 2022 20:26:02 +0300

> From: Mattias Engdegård <mattiase <at> acm.org>
> Date: Fri, 27 May 2022 18:49:49 +0200
> Cc: 55676 <at> debbugs.gnu.org
> 
> 27 maj 2022 kl. 18.01 skrev Eli Zaretskii <eliz <at> gnu.org>:
> 
> > One minor nit: you seem to have customized indentation in C
> > source files in a way that is different from our conventions.  Please
> > reformat using our defaults (which use a mix of TABs and SPACEs).
> 
> Sure, but I thought I used the standard settings. Where did I err?

I don't know.  I see indentation made of spaces where tabs should do.
Or so it seemed; apologies if I'm mistaken.  If you don't see more
than 8 SPACE characters in a row, then it's my mistake.

Message #20 received at 55676 <at> debbugs.gnu.org (full text, mbox):

From: Po Lu <luangruo <at> yahoo.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: Mattias Engdegård <mattiase <at> acm.org>,
 55676 <at> debbugs.gnu.org
Subject: Re: bug#55676: [PATCH] non-recursive Lisp reader
Date: Sun, 29 May 2022 09:15:11 +0800

Eli Zaretskii <eliz <at> gnu.org> writes:

> Thanks.  One minor nit: you seem to have customized indentation in C
> source files in a way that is different from our conventions.  Please
> reformat using our defaults (which use a mix of TABs and SPACEs).

Another minor nit:

>> +	    read_stack_push ((struct read_stack_entry){
>> +		.type = RE_record,
>> +		.u.vector.elems = Qnil,
>> +		.u.vector.old_locate_syms = locate_syms,
>> +	      });
>> +	    locate_syms = false;
>> +	    goto read_obj;

There should at least be a space between the cast and the opening brace
of the compound literal, right?  That applies to the rest of the printer
code you installed lately.

And I think using compound literals in `read_stack_push' (and
`print_stack_push') is very ugly, but there's no rule against it.

Thanks.

Message #23 received at 55676 <at> debbugs.gnu.org (full text, mbox):

From: Mattias Engdegård <mattiase <at> acm.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 55676 <at> debbugs.gnu.org
Subject: Re: bug#55676: [PATCH] non-recursive Lisp reader
Date: Sun, 29 May 2022 10:27:47 +0200

27 maj 2022 kl. 19.26 skrev Eli Zaretskii <eliz <at> gnu.org>:

> I don't know.  I see indentation made of spaces where tabs should do.
> Or so it seemed; apologies if I'm mistaken.

No, you are right, I had used spaces for a hand-indented line. Now fixed, good spotting.

Message #26 received at 55676 <at> debbugs.gnu.org (full text, mbox):

From: Mattias Engdegård <mattiase <at> acm.org>
To: Po Lu <luangruo <at> yahoo.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 55676 <at> debbugs.gnu.org
Subject: Re: bug#55676: [PATCH] non-recursive Lisp reader
Date: Sun, 29 May 2022 10:45:58 +0200

29 maj 2022 kl. 03.15 skrev Po Lu <luangruo <at> yahoo.com>:

> There should at least be a space between the cast and the opening brace
> of the compound literal, right?

No, that's not a cast. Existing usage seems to vary; I don't see either as strictly more readable than the other.

> And I think using compound literals in `read_stack_push' (and
> `print_stack_push') is very ugly, but there's no rule against it.

In other words, you wish there were a rule against a C feature because you find it very ugly?
I know precisely how you feel, friend.

Message #29 received at 55676 <at> debbugs.gnu.org (full text, mbox):

From: Po Lu <luangruo <at> yahoo.com>
To: Mattias Engdegård <mattiase <at> acm.org>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 55676 <at> debbugs.gnu.org
Subject: Re: bug#55676: [PATCH] non-recursive Lisp reader
Date: Sun, 29 May 2022 17:09:44 +0800

Mattias Engdegård <mattiase <at> acm.org> writes:

> No, that's not a cast.

It visually is one.  We never write code that looks like this:

  (int *)foo

instead writing

  (int *) foo

so I don't see why

  (struct foo){ ...

has to differ.

> Existing usage seems to vary; I don't see either as strictly more
> readable than the other.

I don't often see compound literals used as arguments to functions in
Emacs code, but certainly never where there is no space between the
parenthesized type name and initializer list.

Searching for "){" only reveals code that you installed.  Meanwhile, in
lisp.h, there is:

#define list(...) \
  listn (ARRAYELTS (((Lisp_Object []) {__VA_ARGS__})), __VA_ARGS__)
#define pure_list(...) \
  pure_listn (ARRAYELTS (((Lisp_Object []) {__VA_ARGS__})), __VA_ARGS__)

in xfont.c:

            char2b[i] = (XChar2b) { .byte1 = code[i] >> 8,
                                    .byte2 = code[i] & 0xFF };

and in fns.c:

Lisp_Object
concat2 (Lisp_Object s1, Lisp_Object s2)
{
  return concat_strings (2, ((Lisp_Object []) {s1, s2}));
}

Lisp_Object
concat3 (Lisp_Object s1, Lisp_Object s2, Lisp_Object s3)
{
  return concat_strings (3, ((Lisp_Object []) {s1, s2, s3}));
}

where there is a space between the type name and initializer list.

Changing instances of the wrong coding style that are already installed
would mess up the VCS history, but new code should comply with our prior
precedent, since applying coding style inconsistently leads to code that
is difficult to read.

Message #32 received at 55676 <at> debbugs.gnu.org (full text, mbox):

From: Mattias Engdegård <mattiase <at> acm.org>
To: Po Lu <luangruo <at> yahoo.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 55676 <at> debbugs.gnu.org
Subject: Re: bug#55676: [PATCH] non-recursive Lisp reader
Date: Sun, 29 May 2022 12:33:51 +0200

29 maj 2022 kl. 11.09 skrev Po Lu <luangruo <at> yahoo.com>:

> so I don't see why
> 
>  (struct foo){ ...
> 
> has to differ.

It still isn't a cast. You have the right to propose new rules, of course -- we all do.
It's all very interesting to debate this but I'm just going with a space between the closing and opening brackets so that we can go on with other things.

Thanks for looking at the patch!

Message #37 received at 55676-done <at> debbugs.gnu.org (full text, mbox):

From: Mattias Engdegård <mattiase <at> acm.org>
To: 55676-done <at> debbugs.gnu.org
Cc: Po Lu <luangruo <at> yahoo.com>, Eli Zaretskii <eliz <at> gnu.org>,
 Lars Ingebrigtsen <larsi <at> gnus.org>
Subject: Re: bug#55676: [PATCH] non-recursive Lisp reader
Date: Mon, 30 May 2022 17:21:53 +0200

With objections apparently resolved, the change has been pushed to master, including extra regression tests for the minor bugs mentioned earlier.

Thank you for reading the patch!

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.