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

[Stefan Monnier <monnier@iro.umontreal.ca>]

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

Maybe one way to figure it out is to magnify the impact: add extra
slowdown code to BVAR to make sure the slowdown is noticeable.
This might point to the place where a performance impact may be
measurable, or it might make it obvious that the impact is negligible.

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

While this might work for some vars, I suspect that it could be
detrimental for some.

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

100% agreement.  And modern branch predictors are quite sophisticated so
I expect they'll do a much better job than any hint we may be able
to provide.

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

Indeed, there are two concepts: one if the concept that corresponds to
the `permanent-local` symbol property (that's implemented via
buffer_permanent_local_flags) and the other is the concept of
buffer-local variables which are always buffer-local and don't even have
a default value (that's the one your patch modifies, tho it only
modifies the implementation, not the semantics).

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

BTW, one potentially unintended side effect of your change is that
`makunbound` on PER_BUFFER vars won't work quite as usual.  I guess we
could address that by using some other special value than Qunbound, tho
I'm not sure how important this is.

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

Agreed: I think we should get rid of this special category of "always
local" variables.  This is a concept that only applies to the following
variables:

    bset_filename (&buffer_local_flags, make_fixnum (-1));
    bset_directory (&buffer_local_flags, make_fixnum (-1));
    bset_backed_up (&buffer_local_flags, make_fixnum (-1));
    bset_save_length (&buffer_local_flags, make_fixnum (-1));
    bset_auto_save_file_name (&buffer_local_flags, make_fixnum (-1));
    bset_read_only (&buffer_local_flags, make_fixnum (-1));
    bset_major_mode (&buffer_local_flags, make_fixnum (-1));
    bset_mode_name (&buffer_local_flags, make_fixnum (-1));
    bset_undo_list (&buffer_local_flags, make_fixnum (-1));
    bset_mark_active (&buffer_local_flags, make_fixnum (-1));
    bset_point_before_scroll (&buffer_local_flags, make_fixnum (-1));
    bset_file_truename (&buffer_local_flags, make_fixnum (-1));
    bset_invisibility_spec (&buffer_local_flags, make_fixnum (-1));
    bset_file_format (&buffer_local_flags, make_fixnum (-1));
    bset_auto_save_file_format (&buffer_local_flags, make_fixnum (-1));
    bset_display_count (&buffer_local_flags, make_fixnum (-1));
    bset_display_time (&buffer_local_flags, make_fixnum (-1));
    bset_enable_multibyte_characters (&buffer_local_flags, make_fixnum (-1));

I don't see the benefit of forcing those vars to always be buffer-local.
We could just as well make them "normal" and they'll still have
a buffer-local value pretty much all the time anyway.


        Stefan