Re: [PATCH v2 00/16] Speeding up DEFVAR_PER_BUFFER

[Eli Zaretskii]

> From: Spencer Baugh <sbaugh@catern.com>
> Cc: emacs-devel@gnu.org, arnold@tdrhq.com, monnier@iro.umontreal.ca,
>  dgutov@yandex.ru, rms@gnu.org
> Date: Sun, 29 Nov 2020 12:41:59 -0500
> 
> > Thanks, but I would also like to hear more about patches 3 to 14, and
> > how they fit into the overall idea of the speedup change.  Could you
> > please elaborate on that?  I'm not an expert on this stuff, and I'd
> > like to understand the idea better, so that I could assess each part
> > in the context of its place in the overall scheme of this changeset.
> > I'm sure others could also benefit from hearing more details.
> 
> Sure, here is some overview of the change.
> 
> === Background on DEFVAR_PER_BUFFER variables ===

Thank you very much for this detailed description, it makes the idea
of the changes very clear.

I started reviewing the patches again with this in mind, but ran out
of time for today.  I will try very hard to respond by tomorrow, sorry
about the delay.

A couple of general comments/questions, though:

> === New implementation ===
> 
> In the new implementation, the BVAR macro actually does some work.
> Simplifying a bit:
> 
>   #define BVAR(buf, field) EQ ((buf)->field, Qunbound) ? 
> buffer_defaults.field : (buf)->field
> 
> We now use Qunbound as a sentinel to tell us whether the buffer has a
> buffer-local binding for this field or not.  If the field contains
> Qunbound, we should use the default value out of buffer_defaults; if
> not, there's a buffer-local binding, and we should use the per-buffer
> value.
> 
> We no longer need to iterate over all buffers whenever we change the
> default value.  So setting the default value is now fast.

But OTOH, referencing buffer-local values through BVAR is now slower,
about twice slower, I guess?  For example, the display code has 90
uses of BVAR, and they will now be slower.  I wonder what that means
for us: we are making let-binding of local variables much faster, but
"punish" the code that just accesses these variables in our inner
loops.

> We could do a mass change to all the uses of BVAR, so that accesses to
> non-DEFVAR_PER_BUFFER fields and permanent-buffer-local
> DEFVAR_PER_BUFFER fields don't go through the conditional.  But the
> additional check adds minimal overhead anyway; it will be easily
> branch-predicted by modern CPUs.

I don't think I understand the last part: how can branch-prediction
help here, when which variables do have local bindings and which don't
is not known in advance and can change a lot during a session.  What
am I missing here?

>        Get rid of buffer_permanent_local_flags array
> 
> This removes some usage of the buffer_local_flags indices in favor of a
> simpler special case for the two pseudo-permanent-local variables it
> applied to.

Does this mean we will no longer be able to mark built-in variables as
permanent-buffer-local?  If so, why would we want to lose this
capability?

Thanks again for working on this, and I hope to complete the review by
tomorrow.