GNU bug report logs - #79035
Remove micro-optimization for Funintern for performance

Previous Next

To reply to this bug, email your comments to 79035 AT debbugs.gnu.org.

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

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

From: Lynn Winebarger <owinebar <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org,
 Mattias Engdegård <mattias.engdegard <at> gmail.com>, 
 Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Remove micro-optimization for Funintern for performance
Date: Wed, 16 Jul 2025 23:25:15 -0400

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

Based on my measurement of my reentrant reader submission, I tried
just eliminating the static variable used to "optimize" out a
redundant lookup by stashing the index in a static variable used only
by Funintern.  See attached - I just repeated the lookup code in
Funintern, removed the assignment from oblookup, and eliminated the
static variable.  At least for -O0, storing this information in the
frequently called oblookup to save a lookup in the very infrequently
called Funintern is a bit of a pessimization.  Based on summing the
profiles of all the byte-compile jobs, and filtering for
obarray-related functions in read.c, I get the following for commit
a8b65860a5e28ee0867e8506a17d74d4a9b7783a (the cumulative is all
functions in lread.c, most of which I am eliding):
Each sample counts as 0.01 seconds.
  %   cumulative   self              self     total
 time   seconds   seconds    calls   s/call   s/call  name
 13.44     31.82     8.68 79508607     0.00     0.00  oblookup
  0.85     58.97     0.55      260     2.12     2.21  grow_obarray
  0.84     59.51     0.54 83352123     0.00     0.00  obarray_index
  0.81     60.03     0.52 78691233     0.00     0.00
oblookup_considering_shorthand
  0.37     62.19     0.24  6858863     0.00     0.00  intern_sym
  0.29     63.03     0.19  4635384     0.00     0.00  read_internal_start
  0.08     64.05     0.05 20570557     0.00     0.00  mapatoms_1
  0.06     64.23     0.04     1060     0.04     0.04  map_obarray
  0.05     64.30     0.03  6858863     0.00     0.00  intern_driver
  0.05     64.33     0.03  1143939     0.00     0.00  Fintern
  0.03     64.48     0.02                             Funintern
  0.02     64.57     0.01    66751     0.00     0.00  Fintern_soft
  0.00     64.59     0.00    22626     0.00     0.00
Flread__substitute_object_in_subtree
  0.00     64.59     0.00    22626     0.00     0.00  substitute_object_recurse
  0.00     64.59     0.00    13223     0.00     0.00  Fobarray_make
  0.00     64.59     0.00    13223     0.00     0.00  allocate_obarray
  0.00     64.59     0.00    13223     0.00     0.00  make_obarray
  0.00     64.59     0.00    10916     0.00     0.00  Fobarrayp
  0.00     64.59     0.00     5032     0.00     0.00  Flocate_file_internal
  0.00     64.59     0.00     2522     0.00     0.00  intern_1
  0.00     64.59     0.00     1043     0.00     0.00  Fmapatoms

The same commit with the attached patch:
 13.22     31.13     8.23 79563723     0.00     0.00  oblookup
  0.67     57.22     0.42      264     1.59     1.66  grow_obarray
  0.61     57.60     0.38 83407339     0.00     0.00  obarray_index
  0.61     57.98     0.38 78745509     0.00     0.00
oblookup_considering_shorthand
  0.25     60.64     0.16  6858324     0.00     0.00  intern_sym
  0.19     61.32     0.12  4635494     0.00     0.00  read_internal_start
  0.11     61.59     0.07 20669351     0.00     0.00  mapatoms_1
  0.11     61.66     0.07     1067     0.07     0.07  map_obarray
  0.10     61.84     0.06                             Funintern
  0.05     62.10     0.03    66913     0.00     0.00  Fintern_soft
  0.01     62.27     0.01  6858324     0.00     0.00  intern_driver
  0.00     62.28     0.00  1146143     0.00     0.00  Fintern
  0.00     62.28     0.00    22460     0.00     0.00
Flread__substitute_object_in_subtree
  0.00     62.28     0.00    22460     0.00     0.00  substitute_object_recurse
  0.00     62.28     0.00    13219     0.00     0.00  Fobarray_make
  0.00     62.28     0.00    13219     0.00     0.00  allocate_obarray
  0.00     62.28     0.00    13219     0.00     0.00  make_obarray
  0.00     62.28     0.00    10918     0.00     0.00  Fobarrayp
  0.00     62.28     0.00     5031     0.00     0.00  Flocate_file_internal
  0.00     62.28     0.00     2522     0.00     0.00  intern_1
  0.00     62.28     0.00     1050     0.00     0.00  Fmapatoms

The total reported cpu time for
a8b65860a5e28ee0867e8506a17d74d4a9b7783a is 3479.51s, and with the
patch it is 3460.69.
You can see Funintern is more expensive, but also that in all those
jobs no actual calls were recorded in either case.  I see similar
differences for these functions when I compare results for commit
2bdcf0250acecdb0719203ae711aedf5baad1783.

As a reminder, I submitted my assignment paperwork a couple of years
ago, although this seems like a pretty trivial change.
Lynn

[0001-Remove-micro-optimization-for-Funintern.patch (text/x-patch, attachment)]

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Lynn Winebarger <owinebar <at> gmail.com>
Cc: mattias.engdegard <at> gmail.com, monnier <at> iro.umontreal.ca,
 79035 <at> debbugs.gnu.org
Subject: Re: bug#79035: Remove micro-optimization for Funintern for performance
Date: Thu, 17 Jul 2025 08:04:47 +0300

> From: Lynn Winebarger <owinebar <at> gmail.com>
> Date: Wed, 16 Jul 2025 23:25:15 -0400
> 
> Based on my measurement of my reentrant reader submission, I tried
> just eliminating the static variable used to "optimize" out a
> redundant lookup by stashing the index in a static variable used only
> by Funintern.  See attached - I just repeated the lookup code in
> Funintern, removed the assignment from oblookup, and eliminated the
> static variable.  At least for -O0, storing this information in the
> frequently called oblookup to save a lookup in the very infrequently
> called Funintern is a bit of a pessimization.  Based on summing the
> profiles of all the byte-compile jobs, and filtering for
> obarray-related functions in read.c, I get the following for commit
> a8b65860a5e28ee0867e8506a17d74d4a9b7783a (the cumulative is all
> functions in lread.c, most of which I am eliding):
> Each sample counts as 0.01 seconds.
>   %   cumulative   self              self     total
>  time   seconds   seconds    calls   s/call   s/call  name
>  13.44     31.82     8.68 79508607     0.00     0.00  oblookup
>   0.85     58.97     0.55      260     2.12     2.21  grow_obarray
>   0.84     59.51     0.54 83352123     0.00     0.00  obarray_index
>   0.81     60.03     0.52 78691233     0.00     0.00
> oblookup_considering_shorthand
>   0.37     62.19     0.24  6858863     0.00     0.00  intern_sym
>   0.29     63.03     0.19  4635384     0.00     0.00  read_internal_start
>   0.08     64.05     0.05 20570557     0.00     0.00  mapatoms_1
>   0.06     64.23     0.04     1060     0.04     0.04  map_obarray
>   0.05     64.30     0.03  6858863     0.00     0.00  intern_driver
>   0.05     64.33     0.03  1143939     0.00     0.00  Fintern
>   0.03     64.48     0.02                             Funintern
>   0.02     64.57     0.01    66751     0.00     0.00  Fintern_soft
>   0.00     64.59     0.00    22626     0.00     0.00
> Flread__substitute_object_in_subtree
>   0.00     64.59     0.00    22626     0.00     0.00  substitute_object_recurse
>   0.00     64.59     0.00    13223     0.00     0.00  Fobarray_make
>   0.00     64.59     0.00    13223     0.00     0.00  allocate_obarray
>   0.00     64.59     0.00    13223     0.00     0.00  make_obarray
>   0.00     64.59     0.00    10916     0.00     0.00  Fobarrayp
>   0.00     64.59     0.00     5032     0.00     0.00  Flocate_file_internal
>   0.00     64.59     0.00     2522     0.00     0.00  intern_1
>   0.00     64.59     0.00     1043     0.00     0.00  Fmapatoms
> 
> The same commit with the attached patch:
>  13.22     31.13     8.23 79563723     0.00     0.00  oblookup
>   0.67     57.22     0.42      264     1.59     1.66  grow_obarray
>   0.61     57.60     0.38 83407339     0.00     0.00  obarray_index
>   0.61     57.98     0.38 78745509     0.00     0.00
> oblookup_considering_shorthand
>   0.25     60.64     0.16  6858324     0.00     0.00  intern_sym
>   0.19     61.32     0.12  4635494     0.00     0.00  read_internal_start
>   0.11     61.59     0.07 20669351     0.00     0.00  mapatoms_1
>   0.11     61.66     0.07     1067     0.07     0.07  map_obarray
>   0.10     61.84     0.06                             Funintern
>   0.05     62.10     0.03    66913     0.00     0.00  Fintern_soft
>   0.01     62.27     0.01  6858324     0.00     0.00  intern_driver
>   0.00     62.28     0.00  1146143     0.00     0.00  Fintern
>   0.00     62.28     0.00    22460     0.00     0.00
> Flread__substitute_object_in_subtree
>   0.00     62.28     0.00    22460     0.00     0.00  substitute_object_recurse
>   0.00     62.28     0.00    13219     0.00     0.00  Fobarray_make
>   0.00     62.28     0.00    13219     0.00     0.00  allocate_obarray
>   0.00     62.28     0.00    13219     0.00     0.00  make_obarray
>   0.00     62.28     0.00    10918     0.00     0.00  Fobarrayp
>   0.00     62.28     0.00     5031     0.00     0.00  Flocate_file_internal
>   0.00     62.28     0.00     2522     0.00     0.00  intern_1
>   0.00     62.28     0.00     1050     0.00     0.00  Fmapatoms
> 
> The total reported cpu time for
> a8b65860a5e28ee0867e8506a17d74d4a9b7783a is 3479.51s, and with the
> patch it is 3460.69.

That's just 0.5% of speedup, which doesn't pass my threshold for
worthy changes, especially when the code changes are non-trivial (as
they are in this case).  So my vote is against installing this, sorry.

However, I'm open to other opinions.

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

From: Lynn Winebarger <owinebar <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: Mattias Engdegård <mattias.engdegard <at> gmail.com>,
 Stefan Monnier <monnier <at> iro.umontreal.ca>, 79035 <at> debbugs.gnu.org
Subject: Re: bug#79035: Remove micro-optimization for Funintern for performance
Date: Thu, 17 Jul 2025 01:24:03 -0400

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

On Thu, Jul 17, 2025, 1:04 AM Eli Zaretskii <eliz <at> gnu.org> wrote:

> > From: Lynn Winebarger <owinebar <at> gmail.com>
> > Date: Wed, 16 Jul 2025 23:25:15 -0400
> >
> > Based on my measurement of my reentrant reader submission, I tried
> > just eliminating the static variable used to "optimize" out a
> > redundant lookup by stashing the index in a static variable used only
> > by Funintern.  See attached - I just repeated the lookup code in
> > Funintern, removed the assignment from oblookup, and eliminated the
> > static variable.  At least for -O0, storing this information in the
> > frequently called oblookup to save a lookup in the very infrequently
> > called Funintern is a bit of a pessimization.  Based on summing the
> > profiles of all the byte-compile jobs, and filtering for
> > obarray-related functions in read.c, I get the following for commit
> > a8b65860a5e28ee0867e8506a17d74d4a9b7783a (the cumulative is all
> > functions in lread.c, most of which I am eliding):
> > Each sample counts as 0.01 seconds.
> >   %   cumulative   self              self     total
> >  time   seconds   seconds    calls   s/call   s/call  name
> >  13.44     31.82     8.68 79508607     0.00     0.00  oblookup
> >   0.85     58.97     0.55      260     2.12     2.21  grow_obarray
> >   0.84     59.51     0.54 83352123     0.00     0.00  obarray_index
> >   0.81     60.03     0.52 78691233     0.00     0.00
> > oblookup_considering_shorthand
> >   0.37     62.19     0.24  6858863     0.00     0.00  intern_sym
> >   0.29     63.03     0.19  4635384     0.00     0.00  read_internal_start
> >   0.08     64.05     0.05 20570557     0.00     0.00  mapatoms_1
> >   0.06     64.23     0.04     1060     0.04     0.04  map_obarray
> >   0.05     64.30     0.03  6858863     0.00     0.00  intern_driver
> >   0.05     64.33     0.03  1143939     0.00     0.00  Fintern
> >   0.03     64.48     0.02                             Funintern
> >   0.02     64.57     0.01    66751     0.00     0.00  Fintern_soft
> >   0.00     64.59     0.00    22626     0.00     0.00
> > Flread__substitute_object_in_subtree
> >   0.00     64.59     0.00    22626     0.00     0.00
> substitute_object_recurse
> >   0.00     64.59     0.00    13223     0.00     0.00  Fobarray_make
> >   0.00     64.59     0.00    13223     0.00     0.00  allocate_obarray
> >   0.00     64.59     0.00    13223     0.00     0.00  make_obarray
> >   0.00     64.59     0.00    10916     0.00     0.00  Fobarrayp
> >   0.00     64.59     0.00     5032     0.00     0.00
> Flocate_file_internal
> >   0.00     64.59     0.00     2522     0.00     0.00  intern_1
> >   0.00     64.59     0.00     1043     0.00     0.00  Fmapatoms
> >
> > The same commit with the attached patch:
> >  13.22     31.13     8.23 79563723     0.00     0.00  oblookup
> >   0.67     57.22     0.42      264     1.59     1.66  grow_obarray
> >   0.61     57.60     0.38 83407339     0.00     0.00  obarray_index
> >   0.61     57.98     0.38 78745509     0.00     0.00
> > oblookup_considering_shorthand
> >   0.25     60.64     0.16  6858324     0.00     0.00  intern_sym
> >   0.19     61.32     0.12  4635494     0.00     0.00  read_internal_start
> >   0.11     61.59     0.07 20669351     0.00     0.00  mapatoms_1
> >   0.11     61.66     0.07     1067     0.07     0.07  map_obarray
> >   0.10     61.84     0.06                             Funintern
> >   0.05     62.10     0.03    66913     0.00     0.00  Fintern_soft
> >   0.01     62.27     0.01  6858324     0.00     0.00  intern_driver
> >   0.00     62.28     0.00  1146143     0.00     0.00  Fintern
> >   0.00     62.28     0.00    22460     0.00     0.00
> > Flread__substitute_object_in_subtree
> >   0.00     62.28     0.00    22460     0.00     0.00
> substitute_object_recurse
> >   0.00     62.28     0.00    13219     0.00     0.00  Fobarray_make
> >   0.00     62.28     0.00    13219     0.00     0.00  allocate_obarray
> >   0.00     62.28     0.00    13219     0.00     0.00  make_obarray
> >   0.00     62.28     0.00    10918     0.00     0.00  Fobarrayp
> >   0.00     62.28     0.00     5031     0.00     0.00
> Flocate_file_internal
> >   0.00     62.28     0.00     2522     0.00     0.00  intern_1
> >   0.00     62.28     0.00     1050     0.00     0.00  Fmapatoms
> >
> > The total reported cpu time for
> > a8b65860a5e28ee0867e8506a17d74d4a9b7783a is 3479.51s, and with the
> > patch it is 3460.69.
>
> That's just 0.5% of speedup, which doesn't pass my threshold for
> worthy changes, especially when the code changes are non-trivial (as
> they are in this case).  So my vote is against installing this, sorry.
>

The performance improvement is the punchline.  The purpose of the change is
to remove a static variable.  That's why I originally included it in the
patch to make the code in lread.c re-entrant.  But it  probably makes more
sense as a stand-alone change.

Lynn

[Message part 2 (text/html, inline)]

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Lynn Winebarger <owinebar <at> gmail.com>
Cc: mattias.engdegard <at> gmail.com, monnier <at> iro.umontreal.ca,
 79035 <at> debbugs.gnu.org
Subject: Re: bug#79035: Remove micro-optimization for Funintern for performance
Date: Thu, 17 Jul 2025 09:44:22 +0300

> From: Lynn Winebarger <owinebar <at> gmail.com>
> Date: Thu, 17 Jul 2025 01:24:03 -0400
> Cc: 79035 <at> debbugs.gnu.org, Mattias Engdegård <mattias.engdegard <at> gmail.com>, 
> 	Stefan Monnier <monnier <at> iro.umontreal.ca>
> 
>  > The total reported cpu time for
>  > a8b65860a5e28ee0867e8506a17d74d4a9b7783a is 3479.51s, and with the
>  > patch it is 3460.69.
> 
>  That's just 0.5% of speedup, which doesn't pass my threshold for
>  worthy changes, especially when the code changes are non-trivial (as
>  they are in this case).  So my vote is against installing this, sorry.
> 
> The performance improvement is the punchline.  The purpose of the change is to remove a static variable. 
> That's why I originally included it in the patch to make the code in lread.c re-entrant.  But it  probably makes
> more sense as a stand-alone change.

If this change is related to some other changeset, let alone is needed
by it, it is not standalone, and should be part of that other
changeset.  It would make sense to have it as standalone if it brought
us some significant advantages, which doesn't seem to be the case
here.

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

From: Lynn Winebarger <owinebar <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: Mattias Engdegård <mattias.engdegard <at> gmail.com>,
 Stefan Monnier <monnier <at> iro.umontreal.ca>, 79035 <at> debbugs.gnu.org
Subject: Re: bug#79035: Remove micro-optimization for Funintern for performance
Date: Thu, 17 Jul 2025 09:32:25 -0400

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

On Thu, Jul 17, 2025, 2:44 AM Eli Zaretskii <eliz <at> gnu.org> wrote:

> > From: Lynn Winebarger <owinebar <at> gmail.com>
> > Date: Thu, 17 Jul 2025 01:24:03 -0400
> > Cc: 79035 <at> debbugs.gnu.org, Mattias Engdegård <
> mattias.engdegard <at> gmail.com>,
> >       Stefan Monnier <monnier <at> iro.umontreal.ca>
> >
> >  > The total reported cpu time for
> >  > a8b65860a5e28ee0867e8506a17d74d4a9b7783a is 3479.51s, and with the
> >  > patch it is 3460.69.
> >
> >  That's just 0.5% of speedup, which doesn't pass my threshold for
> >  worthy changes, especially when the code changes are non-trivial (as
> >  they are in this case).  So my vote is against installing this, sorry.
> >
> > The performance improvement is the punchline.  The purpose of the change
> is to remove a static variable.
> > That's why I originally included it in the patch to make the code in
> lread.c re-entrant.  But it  probably makes
> > more sense as a stand-alone change.
>
> If this change is related to some other changeset, let alone is needed
> by it, it is not standalone, and should be part of that other
> changeset.  It would make sense to have it as standalone if it brought
> us some significant advantages, which doesn't seem to be the case

here.
>

It's related in the sense that it's removing static variables in lread.c.
But this patch should probably be considered cleanup from the introduction
of shorthands.  I think the change is actually trivial for someone familiar
with lread.c.

At any rate, the feedback on the patch for making read and friends
reentrant was that it was too much at once.  I'm guessing Mattias and/or
Stefan would prefer to judge this change independently of the rest, but I
can wait for their feedback.

Lynn

[Message part 2 (text/html, inline)]

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Lynn Winebarger <owinebar <at> gmail.com>
Cc: bug-gnu-emacs <at> gnu.org,
 Mattias Engdegård <mattias.engdegard <at> gmail.com>
Subject: Re: Remove micro-optimization for Funintern for performance
Date: Thu, 17 Jul 2025 11:09:21 -0400

> (lread.c): Remove static variable oblookup_last_bucket_number.  Removal
> appears to reduce execution time of obarray functions by about 6%, while
> the variable was only used to avoid a redundant lookup in the rarely
> called Funintern.

[ Actually, it's not even a full lookup that's avoided, it's only the
  computation of the hash index.  ]

Yeah, this static var is ugly, I'm in favor of removing it.

BTW, looking at the code of `Funintern` I get the impression that
`oblookup_considering_shorthand` is wrong when the argument is a symbol:
we should take shorthand into account only when passed a string.


        Stefan

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

From: Mattias Engdegård <mattias.engdegard <at> gmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Emacs Bug Report <bug-gnu-emacs <at> gnu.org>,
 Lynn Winebarger <owinebar <at> gmail.com>
Subject: Re: Remove micro-optimization for Funintern for performance
Date: Thu, 17 Jul 2025 18:05:14 +0200

17 juli 2025 kl. 17.09 skrev Stefan Monnier <monnier <at> iro.umontreal.ca>:

> Yeah, this static var is ugly, I'm in favor of removing it.

Same here. I don't think it was ever meant as an optimisation, just a quick and lazy way to implement Funintern.

> BTW, looking at the code of `Funintern` I get the impression that
> `oblookup_considering_shorthand` is wrong when the argument is a symbol:
> we should take shorthand into account only when passed a string.

Right. Funintern isn't often used but we could do a better job here.

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

From: Lynn Winebarger <owinebar <at> gmail.com>
To: Mattias Engdegård <mattias.engdegard <at> gmail.com>
Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>, 79035 <at> debbugs.gnu.org
Subject: Re: bug#79035: Remove micro-optimization for Funintern for performance
Date: Thu, 17 Jul 2025 18:12:05 -0400

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

On Thu, Jul 17, 2025, 2:51 PM Mattias Engdegård <mattias.engdegard <at> gmail.com>
wrote:

> 17 juli 2025 kl. 17.09 skrev Stefan Monnier <monnier <at> iro.umontreal.ca>:
>
> > Yeah, this static var is ugly, I'm in favor of removing it.
>
> Same here. I don't think it was ever meant as an optimisation, just a
> quick and lazy way to implement Funintern.
>
> > BTW, looking at the code of `Funintern` I get the impression that
> > `oblookup_considering_shorthand` is wrong when the argument is a symbol:
> > we should take shorthand into account only when passed a string.
>
> Right. Funintern isn't often used but we could do a better job here.
>

Well, I certainly have no opinion on the finer points of the semantics. I
only tried to eliminate the unnecessary global state without altering the
behavior.
Since I've worked out (with Pip Cet's help) the undocumented contracts with
debug and elisp-mode, I'm going to be going through the rest of my patch to
rebase it on your recent commits in a more incremental series of changes.
There are some other random bits of detritus in lread that I plan to submit
separately from the main patch, just to simplify the larger task a bit.
Would you (Mattias and Stefan) prefer I submit these bits as a single bug
or one bug per change?

[Message part 2 (text/html, inline)]

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

From: Lynn Winebarger <owinebar <at> gmail.com>
To: Mattias Engdegård <mattias.engdegard <at> gmail.com>
Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>, 79035 <at> debbugs.gnu.org
Subject: Re: bug#79035: Remove micro-optimization for Funintern for performance
Date: Fri, 18 Jul 2025 17:43:56 -0400

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

On Thu, Jul 17, 2025 at 6:12 PM Lynn Winebarger <owinebar <at> gmail.com> wrote:
>
> On Thu, Jul 17, 2025, 2:51 PM Mattias Engdegård <mattias.engdegard <at> gmail.com> wrote:
>>
>> 17 juli 2025 kl. 17.09 skrev Stefan Monnier <monnier <at> iro.umontreal.ca>:
>>
>> > Yeah, this static var is ugly, I'm in favor of removing it.
>>
>> Same here. I don't think it was ever meant as an optimisation, just a quick and lazy way to implement Funintern.
>>
>> > BTW, looking at the code of `Funintern` I get the impression that
>> > `oblookup_considering_shorthand` is wrong when the argument is a symbol:
>> > we should take shorthand into account only when passed a string.
>>
>> Right. Funintern isn't often used but we could do a better job here.
>
>
> Well, I certainly have no opinion on the finer points of the semantics. I only tried to eliminate the unnecessary global state without altering the behavior.
> Since I've worked out (with Pip Cet's help) the undocumented contracts with debug and elisp-mode, I'm going to be going through the rest of my patch to rebase it on your recent commits in a more incremental series of changes.  There are some other random bits of detritus in lread that I plan to submit separately from the main patch, just to simplify the larger task a bit.
> Would you (Mattias and Stefan) prefer I submit these bits as a single bug or one bug per change?
>
Since I haven't heard back, I'm just going to revise the patch set
here.  One of the bits I thought was detritus actually depended on one
of the other changes I had made in lread.c, so I only have 2 small
additional changes.  The commit messages are detailed.
0002-Remove-unreferenced-internal-variable.patch eliminates the unused
variable load_convert_to_unibyte and associated unwind-protect.
0003-Move-read-char-and-friends-to-keyboard.c.patch cleans up lread.c
by grouping read-char et al with read-key-sequence in keyboard.c.

Running make check shows no additional failures compared to the baseline.

Lynn

[0002-Remove-unreferenced-internal-variable.patch (text/x-patch, attachment)]

[0001-Remove-micro-optimization-for-Funintern.patch (text/x-patch, attachment)]

[0003-Move-read-char-and-friends-to-keyboard.c.patch (text/x-patch, attachment)]

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

From: Mattias Engdegård <mattias.engdegard <at> gmail.com>
To: Lynn Winebarger <owinebar <at> gmail.com>
Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>, 79035 <at> debbugs.gnu.org
Subject: Re: bug#79035: Remove micro-optimization for Funintern for performance
Date: Sat, 19 Jul 2025 17:37:10 +0200

18 juli 2025 kl. 23.43 skrev Lynn Winebarger <owinebar <at> gmail.com>:

> Since I haven't heard back, I'm just going to revise the patch set
> here.

I pushed some patches for removing load-convert-to-unibyte (a good catch, thank you!) and fixing unintern.

Regarding read-char etc they indeed don't belong in lread.c but the same is true for some other things such as the obarray code. Maybe later.

Looks like this concludes this bug -- anything else?

For future contributions:

* House conventions for commit messages.
* No walls of text.
* No benchmarks with -O0.

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

From: Lynn Winebarger <owinebar <at> gmail.com>
To: Mattias Engdegård <mattias.engdegard <at> gmail.com>
Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>, 79035 <at> debbugs.gnu.org
Subject: Re: bug#79035: Remove micro-optimization for Funintern for performance
Date: Sat, 19 Jul 2025 12:13:07 -0400

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

On Sat, Jul 19, 2025, 11:37 AM Mattias Engdegård <
mattias.engdegard <at> gmail.com> wrote:

> 18 juli 2025 kl. 23.43 skrev Lynn Winebarger <owinebar <at> gmail.com>:
>
> > Since I haven't heard back, I'm just going to revise the patch set
> > here.
>
> I pushed some patches for removing load-convert-to-unibyte (a good catch,
> thank you!) and fixing unintern.
>
> Regarding read-char etc they indeed don't belong in lread.c but the same
> is true for some other things such as the obarray code. Maybe later.
>

Wasn't sure about obarrays since read0 might conceivably inline code
referencing the initial obarray for performance.


> Looks like this concludes this bug -- anything else?
>
> For future contributions:
>
> * House conventions for commit messages.
>
I'm trying to get there.

* No walls of text.
>
I don't know what that means.  Although I suspect your reaction to that is
something like "Obviously"....

* No benchmarks with -O0.
>

Ok.  Do you have any benchmarks you prefer to see?

Lynn

[Message part 2 (text/html, inline)]

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

From: Lynn Winebarger <owinebar <at> gmail.com>
To: Mattias Engdegård <mattias.engdegard <at> gmail.com>
Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>, 79035 <at> debbugs.gnu.org
Subject: Re: bug#79035: Remove micro-optimization for Funintern for performance
Date: Sat, 19 Jul 2025 13:31:14 -0400

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

On Sat, Jul 19, 2025, 12:13 PM Lynn Winebarger <owinebar <at> gmail.com> wrote:

> On Sat, Jul 19, 2025, 11:37 AM Mattias Engdegård <
> mattias.engdegard <at> gmail.com> wrote:
>
>>
>> Looks like this concludes this bug -- anything else?
>>
> No.

>
>> For future contributions:
>>
>> * House conventions for commit messages.
>>
> I'm trying to get there.
>

But unfortunately I see what you mean after review.

Lynn

[Message part 2 (text/html, inline)]

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

From: Mattias Engdegård <mattias.engdegard <at> gmail.com>
To: Lynn Winebarger <owinebar <at> gmail.com>
Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>, 79035 <at> debbugs.gnu.org
Subject: Re: bug#79035: Remove micro-optimization for Funintern for performance
Date: Wed, 30 Jul 2025 12:57:08 +0200

Reconsidering this, no harm comes from moving read-char from lread.c to keyboard.c; now done on master.

Previous Next

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