bug#53242: [PATCH] unify reads from local_var_alist

[Sergey Vinokurov <serg.foo@gmail.com>]

On 15/01/2022 16:02, Corwin Brust wrote:
> I tried to follow this conversation but it wasn't clear to me what out 
> motive is for this change.

The motive is that prior to change the alist with buffer-local variables 
was handled inconsistently. Sometimes with Fassoc, other times with 
Fassq and even assq_no_quit (the one that doesn't allow interrupts). 
Since the keys of alist are symbols (variable names), it doesn't make 
sense to use Fassoc which compares them with Fequal - an Fassq which 
does the comparison which simpler Feq would suffice.

> I had understood we typically make (especially in the c sources) our 
> changes to achieve specific, tangible improvement.  Is that the case 
> here?  is the particularly oppressive 'tech debt'?  In the latter case, 
> does history reflect consideration wrt the original selections in each 
> of the various cases we hereby change?

I don't know whether this is an oppressive tech debt, but from my 
perspective I have taken a look over handling of buffer-local variables 
during hacking some elisp code and saw the inconsistency. My patch is 
just an effort to reduce it and try to make Emacs a little bit better 
than it was before.

I don't know what the improvement will be, probably in will be pretty small.

My main consideration for selecting which function to use it to look at 
the types, notice that this in an associative list with symbols as keys 
and select the most appropriate function that would handle lookups in 
the list.

> Also (and especially if we must 'clean for the sake of cleanliness'), 
> could we prefer the (seeming more conservative of UX) interruptable 
> varient in this case?  (Is that very costly? How costly and how have we 
> measured that?)

Some parts before the change were already using uninterruptible variant.

The Fassq does more work than assq_no_quit because it's not only 
interruptible but also checks for circular lists whereas assq_no_quit 
does not handle them correctly and would just loop forever. It is safe 
to use assq_no_quit for buffer-local variables because this in Emacs 
internal structure not visible to the user, Emacs fully maintains it and 
does not make it into a circular list.

 > Please consider the case of a package developer who may be abusing 
buffer-local vars during experiments.  It seems this will cause much 
more ’oops, time to kill Emacs/grab a coffee'.

I think it's unrealistic to introduce, even accidentally, enough 
buffer-local variables that lack of interruptibility in these particular 
functions will start to show.

This is based on the following benchmark, which I encourage everyone to 
try out. It creates a list of length n and does one lookup into it. This 
corresponds to a buffer having n local variables and the lookup is the 
operation we're arguing about (Fassq vs assq_no_quit). The assq_no_quit 
is not exposed in elisp as it's not safe so the benchmark uses Fassq but 
assq_no_quit will be pretty close as it does roughly the same amount of 
work.

(defun mk-list (n)
   (let ((res nil))
     (dotimes (i n)
       (push (cons i nil) res))
     res))

(byte-compile #'mk-list)

(let* ((n 100000)
        (xs (mk-list n)))
   (benchmark-call
    (lambda ()
      (assq 'foo xs))
    1))

It takes pretty large n to get the lookup take significant amount of 
time (please note that list creation time is not included in the 
calculation as it has nothing to do with Fassq vs assq_no_quit, so look 
at what benchmark-call returns and not on how long it all subjectively 
takes).

On my machine I need 10 000 000 elements for lookup to take 110 ms, 
which is a noticeable amount of time (probably still bearable to work 
with). For Emacs to "freeze" over, say 10 second, the number of 
variables introduced has to be even higher.

Is having millions (or hundreds of millions...) of buffer-local 
variables a reasonable scenario to consider? Please keep in mind that 
even if this occurs, interruptibility will not make lookups finish 
faster (even probably slower because of additional work that Fassq does 
compared to assq_no_quit). Yes, user would be able to C-g out of an 
operation if we use Fassq. But the operation will still be as much slow 
the next time it's performed. User will do the 'oops, time to kill 
Emacs/grab a coffee' sequence anyway in this case since any commands 
looking at local variables will be affected. AND this is the case with 
Emacs prior to my patch. Please do a benchmark and define 100 000 000 
local variables and see whether Emacs works appropriately well in this case.

I argue that if we're really concerned what would happen when someone 
defines a few hundred million local variables then we should not be 
asking a question whether reads of said variables are interruptible but 
should be asking a question whether linked list is the appropriate data 
structure to store local variables in the first place (spoiler alert: 
it's not).