On Thu, Jul 17, 2025, 1:04 AM Eli Zaretskii <eliz@gnu.org> wrote:
> From: Lynn Winebarger <owinebar@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