Subject: Re: bug#79035: Remove micro-optimization for Funintern for performance

From: Lynn Winebarger <[email-masked]>
Date: Thu, 17 Jul 2025 01:24:03 -0400

On Thu, Jul 17, 2025, 1:04 AM Eli Zaretskii <[email-masked]> wrote:

> > From: Lynn Winebarger <[email-masked]>
> > 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