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