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

[Spencer Baugh]

Eli Zaretskii <eliz@gnu.org> writes:
>> === 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.

Yes, it's probably a trade-off.  My guess is that the extra overhead of
BVAR will not be significant, but benchmarking is the only way to know
for sure.  (Indeed, it's possible things will speed up by removing the
metadata, which reduces the size of the working set and lets more things
stay in cache)

>> 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?

Well, with a completely naive static branch-predictor which assumes the
Qunbound branch will never be taken, there will be no overhead for
fields which are never Qunbound, namely non-DEFVAR_PER_BUFFER fields and
permanent-buffer-locals.  We could encourage that prediction by marking
the Qunbound branch with an "unlikely()" macro using GCC's
__builtin_expect.

It's probably prematurely low-level for me to talk about such things
right now though - benchmarking is the only way to know.

>>        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?

No, we can still mark them as permanent-buffer-local.  The control flow
here is a bit weird. buffer_permanent_local_flags is not how
DEFVAR_PER_BUFFER variables are normally marked as
permanent-buffer-local.

A DEFVAR_PER_BUFFER variable is normally marked as
permanent-buffer-local by:

- in the old version, setting -1 in the corresponding field in
  buffer_local_flags (which is a `struct buffer' distinct from
  buffer_permanent_local_flags which is an array of chars)
- in the new version, setting the corresponding field in buffer_defaults
  to Qunbound (indicating there's no default value)

When such marking is done, all the normal behavior of
permanent-buffer-local variables happens:

- there's no default value,
- local-variable-p always returns t,
- kill-local-variable is a no-op, etc.

buffer_permanent_local_flags is a separate, orthogonal mechanism.
If a variable is marked in buffer_permanent_local_flags:

- it still has a default value which can be set,
- local-variable-p can return either nil or t,
- kill-local-variable kills the local binding, etc.

In summary, a variable marked in buffer_permanent_local_flags is in
general indistinguishable from any other non-permanent buffer-local
variable.  It's not actually permanent-buffer-local.

buffer_permanent_local_flags has only one effect, on the
reset_buffer_local_variables function.  reset_buffer_local_variables in
fact treats non-permanent and permanent buffer-local-variables
identically, and always resets all of them.  Its behavior only diverges
for variables marked with buffer_permanent_local_flags, which it only
resets if the confusingly named PERMANENT_TOO argument is true.

This is mentioned in the docstring of reset_buffer_local_variables:

   it does not treat permanent locals consistently.

All this is rather confusing and inconsistent; I don't think we should
add more of these "pseudo-permanent" variables.