ido-switch-buffer is slow with many buffers; others are fast

ido-switch-buffer is slow with many buffers; others are fast

[catern]

I recently became annoyed enough in my 1000-buffer Emacs instance to
start profiling things.

I was disappointed to find that the buffer-switching slowness I'd seen
is the fault of ido-switch-buffer.

ido-switch-buffer takes over a second before it starts accepting user
input, with this many buffers.

It appears that the slowness is completely due to ido building a list of
all buffers in ido-make-buffer-list-1. This function calls
ido-ignore-item-p on every buffer, which checks buffer names against a
regex to ignore ones with leading spaces, which seems to be fairly slow.

I tested a few other packages to see how they compared.

- switch-to-buffer, without modification: This is fast. Compared to ido,
  I believe this is because switch-to-buffer ultimately uses read-buffer
  and internal-complete-buffer to read the buffer, which are implemented
  in C and presumably have been well-optimized. ido doesn't use those.

- switch-to-buffer with icomplete-mode: This is also fast, I suppose
  because it also uses read-buffer and internal-complete-buffer, and
  just customizes completion, unlike ido-mode.

- switch-to-buffer with fido-mode: This is also fast, again for the same
  reasons as icomplete-mode.

- iswitchb-mode: This is fairly fast - faster than ido-mode. But, it
  still has a noticeable delay. Like ido, it doesn't use read-buffer,
  and it builds a list of all buffers. Relative to ido, it seems to have
  a more efficient (and less generic) implementation of ignoring
  irrelevant buffers, which explains its speed-up. This is a bit
  unfortunate since iswitchb-mode has been deprecated in favor of
  ido-mode.

I also tried some third-party packages.

- helm: Slow. Looks like it builds a list of all buffers. (Note that
  helm is not in GNU ELPA, but is quite popular.)

- ivy: Fast. It uses read-buffer and only customizes completion.
  Notably, ivy seems to have many, if not a superset, of the features of
  ido-mode. (ivy is in GNU ELPA)

What is the solution? It seems to me that there are two candidates:

1. Given that ivy has a similar feature set to ido, and uses
   read-buffer, and is fast, it seems that it should be possible to make
   ido use read-buffer without loss of functionality.

2. ido should be deprecated or discouraged in favor of ivy, possibly as
   part of including ivy into core Emacs.

Discovering this inefficiency of ido (which probably applies to other
ido functions, since they have similar implementations) is fairly
disappointing to me, because it's frequently recommended to new
users.

There have been many proposals to turn ido on by default in Emacs.
Alas, given the current state of ido's performance, it seems to me to be
clearly unacceptable to turn ido on by default.

Re: ido-switch-buffer is slow with many buffers; others are fast

[Stefan Monnier]

> I recently became annoyed enough in my 1000-buffer Emacs instance to
> start profiling things.
[...]
> It appears that the slowness is completely due to ido building a list of
> all buffers in ido-make-buffer-list-1. This function calls
> ido-ignore-item-p on every buffer, which checks buffer names against a
> regex to ignore ones with leading spaces, which seems to be fairly slow.

With current processing power, I find it strange that filtering with
a single regexp on a 1000-element list should take a whole second.
IOW, I suspect that there's more to it than that (e.g. an
inefficient regexp, or some other cost somewhere).

>   This is a bit unfortunate since iswitchb-mode has been deprecated in
>   favor of ido-mode.

Actually, it was deprecated in favor of icomplete-mode.

> There have been many proposals to turn ido on by default in Emacs.

I can't remember such proposals, at least not in the last 10 years or so.

> Alas, given the current state of ido's performance, it seems to me to be
> clearly unacceptable to turn ido on by default.

`ido-mode` is barely being maintained, I wouldn't recommend it.


        Stefan

Re: ido-switch-buffer is slow with many buffers; others are fast

[sbaugh]

> With current processing power, I find it strange that filtering with
> a single regexp on a 1000-element list should take a whole second.
> IOW, I suspect that there's more to it than that (e.g. an
> inefficient regexp, or some other cost somewhere).

Ah, you're right. After more investigation, it seems to be that
string-match is extremely inefficient for me for some
reason. Specifically, if case-fold-search is buffer-local or set with
let, (string-match "" "") is about 2000x slower (not exaggeration) than
if case-fold-search is global. (Doesn't matter what case-fold-search is
set to.)

That is,

(setq-default case-fold-search t) (benchmark-run 1000 (string-match "" ""))

is about 2000x faster than

(benchmark-run 1000 (let ((case-fold-search t)) (string-match "" "")))

It seems this isn't (directly) related to my configuration, because a
fresh Emacs doesn't have the issue. Instead, it's something about my
running, 1000-buffer Emacs... not sure what would cause this or how to
fix it.

Re: ido-switch-buffer is slow with many buffers; others are fast

[Stefan Monnier]

> (setq-default case-fold-search t) (benchmark-run 1000 (string-match "" ""))
>
> is about 2000x faster than
>
> (benchmark-run 1000 (let ((case-fold-search t)) (string-match "" "")))
>
> It seems this isn't (directly) related to my configuration, because a
> fresh Emacs doesn't have the issue. Instead, it's something about my
> running, 1000-buffer Emacs... not sure what would cause this or how to
> fix it.

Hmm... I think you'll need the use profiling to get to the bottom
of it.  I'd of course that with `profiler-start`, but there's a chance
that you'll have to do the profiling at the level of the C code to
better figure out what's going on.


        Stefan

Re: ido-switch-buffer is slow with many buffers; others are fast

[Dmitry Gutov]

On 14.11.2020 18:34, catern@catern.com wrote:
> I recently became annoyed enough in my 1000-buffer Emacs instance to
> start profiling things.
> 
> I was disappointed to find that the buffer-switching slowness I'd seen
> is the fault of ido-switch-buffer.
> 
> ido-switch-buffer takes over a second before it starts accepting user
> input, with this many buffers.

Which Emacs version are you using?

A fix that seems relevant has been added in df4991093b9 recently. But 
it's only in Emacs 28 thus far.

Re: ido-switch-buffer is slow with many buffers; others are fast

[Spencer Baugh]

Dmitry Gutov <dgutov@yandex.ru> writes:
> On 14.11.2020 18:34, catern@catern.com wrote:
>> I recently became annoyed enough in my 1000-buffer Emacs instance to
>> start profiling things.
>> 
>> I was disappointed to find that the buffer-switching slowness I'd seen
>> is the fault of ido-switch-buffer.
>> 
>> ido-switch-buffer takes over a second before it starts accepting user
>> input, with this many buffers.
>
> Which Emacs version are you using?
>
> A fix that seems relevant has been added in df4991093b9 recently. But 
> it's only in Emacs 28 thus far.

Oh, indeed, that seems perfect! So, forget everything I said before -
this fixes ido's performance to be quite fine with large numbers of
buffers. So that specific problem is solved.

The associated bug#41029 mentions:
>Many of the buffer locals (in particular I think it's the
>SYMBOL_FORWARDED types --- my terminoly may be off), appear to be O(n)
>to bind, where n is the number of live buffers.

That's quite worrying - that implies that pretty much any code will run
slower as the number of buffers increases, even if it doesn't have
anything to do with buffers! This explains the slow performance of
let-binding case-fold-search that I mentioned in another branch of this
thread.

Is this poor scaling of binding buffer locals a known problem? Has
anyone worked on fixing this?

Re: ido-switch-buffer is slow with many buffers; others are fast

[Stefan Monnier]

> The associated bug#41029 mentions:
>> Many of the buffer locals (in particular I think it's the
>> SYMBOL_FORWARDED types --- my terminoly may be off), appear to be O(n)
>> to bind, where n is the number of live buffers.
> That's quite worrying - that implies that pretty much any code will run
> slower as the number of buffers increases, even if it doesn't have
> anything to do with buffers!

Yup.
It affects all the vars defined via `DEFVAR_PER_BUFFER`.

> This explains the slow performance of let-binding case-fold-search
> that I mentioned in another branch of this thread.

Yes it's the source of that problem.

> Is this poor scaling of binding buffer locals a known problem?

At least since bug#41029, yes ;-)

> Has anyone worked on fixing this?

Not that I know, no.

It's a fairly tricky part of the code, but if you're interested, I'd be
very happy to help you along (I'm familiar with this code for having
reworked it somewhat around the time of Emacs-24).


        Stefan

Re: ido-switch-buffer is slow with many buffers; others are fast

[Spencer Baugh]

Stefan Monnier <monnier@iro.umontreal.ca> writes:
>> The associated bug#41029 mentions:
>>> Many of the buffer locals (in particular I think it's the
>>> SYMBOL_FORWARDED types --- my terminoly may be off), appear to be O(n)
>>> to bind, where n is the number of live buffers.
>> That's quite worrying - that implies that pretty much any code will run
>> slower as the number of buffers increases, even if it doesn't have
>> anything to do with buffers!
>
> Yup.
> It affects all the vars defined via `DEFVAR_PER_BUFFER`.
>
>> This explains the slow performance of let-binding case-fold-search
>> that I mentioned in another branch of this thread.
>
> Yes it's the source of that problem.
>
>> Is this poor scaling of binding buffer locals a known problem?
>
> At least since bug#41029, yes ;-)
>
>> Has anyone worked on fixing this?
>
> Not that I know, no.
>
> It's a fairly tricky part of the code, but if you're interested, I'd be
> very happy to help you along (I'm familiar with this code for having
> reworked it somewhat around the time of Emacs-24).

I'm not sure I have the requisite expertise, but I have the motivation,
so yes, I'll give fixing this a try.

If I'm guessing right, the problem is the NILP (Flocal_variable_p
(symbol, Qnil)) case of specbind, on line 3351 of eval.c. In that case,
the let-binding actually changes the "global" value of the variable.

So in turn, we call do_specbind, which calls set_default_internal, which
iterates over all buffers to set the buffer slot to the "global"
value. And then I guess we again reset the value in all live buffers
when unwinding the let?

(Actually, couldn't this be buggy if a new live buffer is created while
in the scope of the let? The new live buffer wouldn't have the value set
by the let-binding...)

It seems like maybe this could be fixed by having a true global value
for DEFVAR_PER_BUFFER which is checked on variable read when there's no
buffer-local binding. Then a let-binding which changes the "global"
value would just, change that value. I'm not sure why it doesn't already
work like that?

I added arnold@tdrhq.com to CC since they filed bug#41029 in the first
place. Arnold - you mentioned trying to optimize the binding process in
the bug. Do you have any more detail about what you tried that didn't
work out?

Re: ido-switch-buffer is slow with many buffers; others are fast

[Arnold Noronha]

On Sun, Nov 15, 2020 at 03:49:06PM -0500, Spencer Baugh wrote:
> I added arnold@tdrhq.com to CC since they filed bug#41029 in the first
> place. Arnold - you mentioned trying to optimize the binding process in
> the bug. Do you have any more detail about what you tried that didn't
> work out?

I wish I had more information to give you, but it's been a while. I
tried looking for old git branches or any notes I kept, but with no
luck. Happy to help if I can though, I'm sure there'll be things
that'll jolt my memory! It's also possible I gave up way too quickly,
looking for the low hanging fruit so I could get back to using
ido-switch-buffer.

--Arnold

[PATCH 00/10] Speeding up DEFVAR_PER_BUFFER (Was: ido-switch-buffer is slow)

[Spencer Baugh]

Here is my first attempt at speeding up DEFVAR_PER_BUFFER. I would
greatly appreciate review/comments. This is just a first attempt and
will certainly need some cleanup.

Here is a simple benchmark just for DEFVAR_PER_BUFFER:

(benchmark-run 500000 (let ((case-fold-search nil)) (string-match "" "")))
; unpatched emacs: (0.6872330339999999 0 0.0)
; patched emacs: (0.608068564 0 0.0)
(setq my-buffer-list nil)
(dotimes (idx 1000) (push (get-buffer-create (format "test-buffer:%s" idx)) my-buffer-list))

(benchmark-run 500000 (let ((case-fold-search nil)) (string-match "" "")))
; unpatched emacs: (18.591189848 0 0.0)
; patched emacs: (0.5885462539999999 0 0.0)

There's some measurement noise, but the patch does indeed eliminate
the performance overhead of let-binding DEFVAR_PER_BUFFER when there
are large numbers of buffers. A 40x speedup is not too shabby. :)

The last two patches are the core of the change.  Most of the rest of
the patch series is simple preparatory changes for changing the
definition of the BVAR macro.

Spencer Baugh (10):
  Take buffer field name in DEFVAR_PER_BUFFER
  Add bset_save_length and use it
  Use bset_last_selected_window everywhere
  Use bset_enable_multibyte_characters everywhere
  Add BVAR_DEFAULT for access to buffer defaults
  Disallow using BVAR as an lvalue
  Reorder buffer.h for upcoming rework of BVAR
  Make cache_long_scans buffer-local when setting it
  Access buffer_defaults in BVAR if there's no local binding
  Don't iterate over all buffers in set_default_internal

 src/buffer.c   | 243 +++++++++++++++++++-------------------
 src/buffer.h   | 313 +++++++++++++++++++++++++++----------------------
 src/category.h |   2 +-
 src/data.c     |  30 +----
 src/eval.c     |  18 ++-
 src/fileio.c   |  12 +-
 src/syntax.h   |   2 +-
 src/window.c   |   5 +-
 8 files changed, 320 insertions(+), 305 deletions(-)

-- 
2.28.0

[PATCH 01/10] Take buffer field name in DEFVAR_PER_BUFFER

[Spencer Baugh]

This removes some unnecessary usage of BVAR which would need to be
changed later anyway.

Also, this makes the comment above the define of DEFVAR_PER_BUFFER
correct - it already says that vname is the name of the buffer slot.
---
 src/buffer.c | 124 +++++++++++++++++++++++++--------------------------
 1 file changed, 61 insertions(+), 63 deletions(-)

diff --git a/src/buffer.c b/src/buffer.c
index 4fd2b0c8b1..64d0a74fb0 100644
--- a/src/buffer.c
+++ b/src/buffer.c
@@ -5462,18 +5462,16 @@ init_buffer (void)
 #define DEFVAR_PER_BUFFER(lname, vname, predicate, doc)		\
   do {								\
     static struct Lisp_Buffer_Objfwd bo_fwd;			\
-    defvar_per_buffer (&bo_fwd, lname, vname, predicate);	\
+    defvar_per_buffer (&bo_fwd, lname, PER_BUFFER_VAR_OFFSET (vname), predicate); \
   } while (0)
 
 static void
 defvar_per_buffer (struct Lisp_Buffer_Objfwd *bo_fwd, const char *namestring,
-		   Lisp_Object *address, Lisp_Object predicate)
+		   int offset, Lisp_Object predicate)
 {
   struct Lisp_Symbol *sym;
-  int offset;
 
   sym = XSYMBOL (intern (namestring));
-  offset = (char *)address - (char *)current_buffer;
 
   bo_fwd->type = Lisp_Fwd_Buffer_Obj;
   bo_fwd->offset = offset;
@@ -5538,20 +5536,20 @@ syms_of_buffer (void)
 	build_pure_c_string ("Attempt to modify a protected field"));
 
   DEFVAR_PER_BUFFER ("tab-line-format",
-		     &BVAR (current_buffer, tab_line_format),
+		     tab_line_format,
 		     Qnil,
 		     doc: /* Analogous to `mode-line-format', but controls the tab line.
 The tab line appears, optionally, at the top of a window;
 the mode line appears at the bottom.  */);
 
   DEFVAR_PER_BUFFER ("header-line-format",
-		     &BVAR (current_buffer, header_line_format),
+		     header_line_format,
 		     Qnil,
 		     doc: /* Analogous to `mode-line-format', but controls the header line.
 The header line appears, optionally, at the top of a window;
 the mode line appears at the bottom.  */);
 
-  DEFVAR_PER_BUFFER ("mode-line-format", &BVAR (current_buffer, mode_line_format),
+  DEFVAR_PER_BUFFER ("mode-line-format", mode_line_format,
 		     Qnil,
 		     doc: /* Template for displaying mode line for current buffer.
 
@@ -5617,32 +5615,32 @@ A string is printed verbatim in the mode line except for %-constructs:
   %% -- print %.   %- -- print infinitely many dashes.
 Decimal digits after the % specify field width to which to pad.  */);
 
-  DEFVAR_PER_BUFFER ("major-mode", &BVAR (current_buffer, major_mode),
+  DEFVAR_PER_BUFFER ("major-mode", major_mode,
 		     Qsymbolp,
 		     doc: /* Symbol for current buffer's major mode.
 The default value (normally `fundamental-mode') affects new buffers.
 A value of nil means to use the current buffer's major mode, provided
 it is not marked as "special".  */);
 
-  DEFVAR_PER_BUFFER ("mode-name", &BVAR (current_buffer, mode_name),
+  DEFVAR_PER_BUFFER ("mode-name", mode_name,
                      Qnil,
 		     doc: /* Pretty name of current buffer's major mode.
 Usually a string, but can use any of the constructs for `mode-line-format',
 which see.
 Format with `format-mode-line' to produce a string value.  */);
 
-  DEFVAR_PER_BUFFER ("local-abbrev-table", &BVAR (current_buffer, abbrev_table), Qnil,
+  DEFVAR_PER_BUFFER ("local-abbrev-table", abbrev_table, Qnil,
 		     doc: /* Local (mode-specific) abbrev table of current buffer.  */);
 
-  DEFVAR_PER_BUFFER ("abbrev-mode", &BVAR (current_buffer, abbrev_mode), Qnil,
+  DEFVAR_PER_BUFFER ("abbrev-mode", abbrev_mode, Qnil,
 		     doc: /*  Non-nil if Abbrev mode is enabled.
 Use the command `abbrev-mode' to change this variable.  */);
 
-  DEFVAR_PER_BUFFER ("case-fold-search", &BVAR (current_buffer, case_fold_search),
+  DEFVAR_PER_BUFFER ("case-fold-search", case_fold_search,
 		     Qnil,
 		     doc: /* Non-nil if searches and matches should ignore case.  */);
 
-  DEFVAR_PER_BUFFER ("fill-column", &BVAR (current_buffer, fill_column),
+  DEFVAR_PER_BUFFER ("fill-column", fill_column,
 		     Qintegerp,
 		     doc: /* Column beyond which automatic line-wrapping should happen.
 It is used by filling commands, such as `fill-region' and `fill-paragraph',
@@ -5650,26 +5648,26 @@ and by `auto-fill-mode', which see.
 See also `current-fill-column'.
 Interactively, you can set the buffer local value using \\[set-fill-column].  */);
 
-  DEFVAR_PER_BUFFER ("left-margin", &BVAR (current_buffer, left_margin),
+  DEFVAR_PER_BUFFER ("left-margin", left_margin,
 		     Qintegerp,
 		     doc: /* Column for the default `indent-line-function' to indent to.
 Linefeed indents to this column in Fundamental mode.  */);
 
-  DEFVAR_PER_BUFFER ("tab-width", &BVAR (current_buffer, tab_width),
+  DEFVAR_PER_BUFFER ("tab-width", tab_width,
 		     Qintegerp,
 		     doc: /* Distance between tab stops (for display of tab characters), in columns.
 NOTE: This controls the display width of a TAB character, and not
 the size of an indentation step.
 This should be an integer greater than zero.  */);
 
-  DEFVAR_PER_BUFFER ("ctl-arrow", &BVAR (current_buffer, ctl_arrow), Qnil,
+  DEFVAR_PER_BUFFER ("ctl-arrow", ctl_arrow, Qnil,
 		     doc: /* Non-nil means display control chars with uparrow.
 A value of nil means use backslash and octal digits.
 This variable does not apply to characters whose display is specified
 in the current display table (if there is one).  */);
 
   DEFVAR_PER_BUFFER ("enable-multibyte-characters",
-		     &BVAR (current_buffer, enable_multibyte_characters),
+		     enable_multibyte_characters,
 		     Qnil,
 		     doc: /* Non-nil means the buffer contents are regarded as multi-byte characters.
 Otherwise they are regarded as unibyte.  This affects the display,
@@ -5683,7 +5681,7 @@ See also Info node `(elisp)Text Representations'.  */);
   make_symbol_constant (intern_c_string ("enable-multibyte-characters"));
 
   DEFVAR_PER_BUFFER ("buffer-file-coding-system",
-		     &BVAR (current_buffer, buffer_file_coding_system), Qnil,
+		     buffer_file_coding_system, Qnil,
 		     doc: /* Coding system to be used for encoding the buffer contents on saving.
 This variable applies to saving the buffer, and also to `write-region'
 and other functions that use `write-region'.
@@ -5701,7 +5699,7 @@ The variable `coding-system-for-write', if non-nil, overrides this variable.
 This variable is never applied to a way of decoding a file while reading it.  */);
 
   DEFVAR_PER_BUFFER ("bidi-display-reordering",
-		     &BVAR (current_buffer, bidi_display_reordering), Qnil,
+		     bidi_display_reordering, Qnil,
 		     doc: /* Non-nil means reorder bidirectional text for display in the visual order.
 Setting this to nil is intended for use in debugging the display code.
 Don't set to nil in normal sessions, as that is not supported.
@@ -5709,7 +5707,7 @@ See also `bidi-paragraph-direction'; setting that non-nil might
 speed up redisplay.  */);
 
   DEFVAR_PER_BUFFER ("bidi-paragraph-start-re",
-		     &BVAR (current_buffer, bidi_paragraph_start_re), Qnil,
+		     bidi_paragraph_start_re, Qnil,
 		     doc: /* If non-nil, a regexp matching a line that starts OR separates paragraphs.
 
 The value of nil means to use empty lines as lines that start and
@@ -5731,7 +5729,7 @@ set both these variables to "^".
 See also `bidi-paragraph-direction'.  */);
 
   DEFVAR_PER_BUFFER ("bidi-paragraph-separate-re",
-		     &BVAR (current_buffer, bidi_paragraph_separate_re), Qnil,
+		     bidi_paragraph_separate_re, Qnil,
 		     doc: /* If non-nil, a regexp matching a line that separates paragraphs.
 
 The value of nil means to use empty lines as paragraph separators.
@@ -5752,7 +5750,7 @@ set both these variables to "^".
 See also `bidi-paragraph-direction'.  */);
 
   DEFVAR_PER_BUFFER ("bidi-paragraph-direction",
-		     &BVAR (current_buffer, bidi_paragraph_direction), Qnil,
+		     bidi_paragraph_direction, Qnil,
 		     doc: /* If non-nil, forces directionality of text paragraphs in the buffer.
 
 If this is nil (the default), the direction of each paragraph is
@@ -5763,7 +5761,7 @@ Any other value is treated as nil.
 This variable has no effect unless the buffer's value of
 `bidi-display-reordering' is non-nil.  */);
 
- DEFVAR_PER_BUFFER ("truncate-lines", &BVAR (current_buffer, truncate_lines), Qnil,
+ DEFVAR_PER_BUFFER ("truncate-lines", truncate_lines, Qnil,
 		     doc: /* Non-nil means do not display continuation lines.
 Instead, give each line of text just one screen line.
 
@@ -5773,7 +5771,7 @@ and this buffer is not full-frame width.
 
 Minibuffers set this variable to nil.  */);
 
-  DEFVAR_PER_BUFFER ("word-wrap", &BVAR (current_buffer, word_wrap), Qnil,
+  DEFVAR_PER_BUFFER ("word-wrap", word_wrap, Qnil,
 		     doc: /* Non-nil means to use word-wrapping for continuation lines.
 When word-wrapping is on, continuation lines are wrapped at the space
 or tab character nearest to the right window edge.
@@ -5791,14 +5789,14 @@ to t, and additionally redefines simple editing commands to act on
 visual lines rather than logical lines.  See the documentation of
 `visual-line-mode'.  */);
 
-  DEFVAR_PER_BUFFER ("default-directory", &BVAR (current_buffer, directory),
+  DEFVAR_PER_BUFFER ("default-directory", directory,
 		     Qstringp,
 		     doc: /* Name of default directory of current buffer.
 It should be an absolute directory name; on GNU and Unix systems,
 these names start with `/' or `~' and end with `/'.
 To interactively change the default directory, use command `cd'. */);
 
-  DEFVAR_PER_BUFFER ("auto-fill-function", &BVAR (current_buffer, auto_fill_function),
+  DEFVAR_PER_BUFFER ("auto-fill-function", auto_fill_function,
 		     Qnil,
 		     doc: /* Function called (if non-nil) to perform auto-fill.
 It is called after self-inserting any character specified in
@@ -5806,31 +5804,31 @@ the `auto-fill-chars' table.
 NOTE: This variable is not a hook;
 its value may not be a list of functions.  */);
 
-  DEFVAR_PER_BUFFER ("buffer-file-name", &BVAR (current_buffer, filename),
+  DEFVAR_PER_BUFFER ("buffer-file-name", filename,
 		     Qstringp,
 		     doc: /* Name of file visited in current buffer, or nil if not visiting a file.
 This should be an absolute file name.  */);
 
-  DEFVAR_PER_BUFFER ("buffer-file-truename", &BVAR (current_buffer, file_truename),
+  DEFVAR_PER_BUFFER ("buffer-file-truename", file_truename,
 		     Qstringp,
 		     doc: /* Abbreviated truename of file visited in current buffer, or nil if none.
 The truename of a file is calculated by `file-truename'
 and then abbreviated with `abbreviate-file-name'.  */);
 
   DEFVAR_PER_BUFFER ("buffer-auto-save-file-name",
-		     &BVAR (current_buffer, auto_save_file_name),
+		     auto_save_file_name,
 		     Qstringp,
 		     doc: /* Name of file for auto-saving current buffer.
 If it is nil, that means don't auto-save this buffer.  */);
 
-  DEFVAR_PER_BUFFER ("buffer-read-only", &BVAR (current_buffer, read_only), Qnil,
+  DEFVAR_PER_BUFFER ("buffer-read-only", read_only, Qnil,
 		     doc: /* Non-nil if this buffer is read-only.  */);
 
-  DEFVAR_PER_BUFFER ("buffer-backed-up", &BVAR (current_buffer, backed_up), Qnil,
+  DEFVAR_PER_BUFFER ("buffer-backed-up", backed_up, Qnil,
 		     doc: /* Non-nil if this buffer's file has been backed up.
 Backing up is done before the first time the file is saved.  */);
 
-  DEFVAR_PER_BUFFER ("buffer-saved-size", &BVAR (current_buffer, save_length),
+  DEFVAR_PER_BUFFER ("buffer-saved-size", save_length,
 		     Qintegerp,
 		     doc: /* Length of current buffer when last read in, saved or auto-saved.
 0 initially.
@@ -5840,7 +5838,7 @@ If you set this to -2, that means don't turn off auto-saving in this buffer
 if its text size shrinks.   If you use `buffer-swap-text' on a buffer,
 you probably should set this to -2 in that buffer.  */);
 
-  DEFVAR_PER_BUFFER ("selective-display", &BVAR (current_buffer, selective_display),
+  DEFVAR_PER_BUFFER ("selective-display", selective_display,
 		     Qnil,
 		     doc: /* Non-nil enables selective display.
 
@@ -5853,11 +5851,11 @@ in a file, save the ^M as a newline.  This usage is obsolete; use
 overlays or text properties instead.  */);
 
   DEFVAR_PER_BUFFER ("selective-display-ellipses",
-		     &BVAR (current_buffer, selective_display_ellipses),
+		     selective_display_ellipses,
 		     Qnil,
 		     doc: /* Non-nil means display ... on previous line when a line is invisible.  */);
 
-  DEFVAR_PER_BUFFER ("overwrite-mode", &BVAR (current_buffer, overwrite_mode),
+  DEFVAR_PER_BUFFER ("overwrite-mode", overwrite_mode,
 		     Qoverwrite_mode,
 		     doc: /* Non-nil if self-insertion should replace existing text.
 The value should be one of `overwrite-mode-textual',
@@ -5867,7 +5865,7 @@ inserts at the end of a line, and inserts when point is before a tab,
 until the tab is filled in.
 If `overwrite-mode-binary', self-insertion replaces newlines and tabs too.  */);
 
-  DEFVAR_PER_BUFFER ("buffer-display-table", &BVAR (current_buffer, display_table),
+  DEFVAR_PER_BUFFER ("buffer-display-table", display_table,
 		     Qnil,
 		     doc: /* Display table that controls display of the contents of current buffer.
 
@@ -5904,7 +5902,7 @@ In addition, a char-table has six extra slots to control the display of:
 
 See also the functions `display-table-slot' and `set-display-table-slot'.  */);
 
-  DEFVAR_PER_BUFFER ("left-margin-width", &BVAR (current_buffer, left_margin_cols),
+  DEFVAR_PER_BUFFER ("left-margin-width", left_margin_cols,
 		     Qintegerp,
 		     doc: /* Width in columns of left marginal area for display of a buffer.
 A value of nil means no marginal area.
@@ -5912,7 +5910,7 @@ A value of nil means no marginal area.
 Setting this variable does not take effect until a new buffer is displayed
 in a window.  To make the change take effect, call `set-window-buffer'.  */);
 
-  DEFVAR_PER_BUFFER ("right-margin-width", &BVAR (current_buffer, right_margin_cols),
+  DEFVAR_PER_BUFFER ("right-margin-width", right_margin_cols,
 		     Qintegerp,
 		     doc: /* Width in columns of right marginal area for display of a buffer.
 A value of nil means no marginal area.
@@ -5920,7 +5918,7 @@ A value of nil means no marginal area.
 Setting this variable does not take effect until a new buffer is displayed
 in a window.  To make the change take effect, call `set-window-buffer'.  */);
 
-  DEFVAR_PER_BUFFER ("left-fringe-width", &BVAR (current_buffer, left_fringe_width),
+  DEFVAR_PER_BUFFER ("left-fringe-width", left_fringe_width,
 		     Qintegerp,
 		     doc: /* Width of this buffer's left fringe (in pixels).
 A value of 0 means no left fringe is shown in this buffer's window.
@@ -5929,7 +5927,7 @@ A value of nil means to use the left fringe width from the window's frame.
 Setting this variable does not take effect until a new buffer is displayed
 in a window.  To make the change take effect, call `set-window-buffer'.  */);
 
-  DEFVAR_PER_BUFFER ("right-fringe-width", &BVAR (current_buffer, right_fringe_width),
+  DEFVAR_PER_BUFFER ("right-fringe-width", right_fringe_width,
 		     Qintegerp,
 		     doc: /* Width of this buffer's right fringe (in pixels).
 A value of 0 means no right fringe is shown in this buffer's window.
@@ -5938,7 +5936,7 @@ A value of nil means to use the right fringe width from the window's frame.
 Setting this variable does not take effect until a new buffer is displayed
 in a window.  To make the change take effect, call `set-window-buffer'.  */);
 
-  DEFVAR_PER_BUFFER ("fringes-outside-margins", &BVAR (current_buffer, fringes_outside_margins),
+  DEFVAR_PER_BUFFER ("fringes-outside-margins", fringes_outside_margins,
 		     Qnil,
 		     doc: /* Non-nil means to display fringes outside display margins.
 A value of nil means to display fringes between margins and buffer text.
@@ -5946,17 +5944,17 @@ A value of nil means to display fringes between margins and buffer text.
 Setting this variable does not take effect until a new buffer is displayed
 in a window.  To make the change take effect, call `set-window-buffer'.  */);
 
-  DEFVAR_PER_BUFFER ("scroll-bar-width", &BVAR (current_buffer, scroll_bar_width),
+  DEFVAR_PER_BUFFER ("scroll-bar-width", scroll_bar_width,
 		     Qintegerp,
 		     doc: /* Width of this buffer's vertical scroll bars in pixels.
 A value of nil means to use the scroll bar width from the window's frame.  */);
 
-  DEFVAR_PER_BUFFER ("scroll-bar-height", &BVAR (current_buffer, scroll_bar_height),
+  DEFVAR_PER_BUFFER ("scroll-bar-height", scroll_bar_height,
 		     Qintegerp,
 		     doc: /* Height of this buffer's horizontal scroll bars in pixels.
 A value of nil means to use the scroll bar height from the window's frame.  */);
 
-  DEFVAR_PER_BUFFER ("vertical-scroll-bar", &BVAR (current_buffer, vertical_scroll_bar_type),
+  DEFVAR_PER_BUFFER ("vertical-scroll-bar", vertical_scroll_bar_type,
 		     Qvertical_scroll_bar,
 		     doc: /* Position of this buffer's vertical scroll bar.
 The value takes effect whenever you tell a window to display this buffer;
@@ -5966,7 +5964,7 @@ A value of `left' or `right' means put the vertical scroll bar at that side
 of the window; a value of nil means don't show any vertical scroll bars.
 A value of t (the default) means do whatever the window's frame specifies.  */);
 
-  DEFVAR_PER_BUFFER ("horizontal-scroll-bar", &BVAR (current_buffer, horizontal_scroll_bar_type),
+  DEFVAR_PER_BUFFER ("horizontal-scroll-bar", horizontal_scroll_bar_type,
 		     Qnil,
 		     doc: /* Position of this buffer's horizontal scroll bar.
 The value takes effect whenever you tell a window to display this buffer;
@@ -5978,13 +5976,13 @@ A value of t (the default) means do whatever the window's frame
 specifies.  */);
 
   DEFVAR_PER_BUFFER ("indicate-empty-lines",
-		     &BVAR (current_buffer, indicate_empty_lines), Qnil,
+		     indicate_empty_lines, Qnil,
 		     doc: /* Visually indicate empty lines after the buffer end.
 If non-nil, a bitmap is displayed in the left fringe of a window on
 window-systems.  */);
 
   DEFVAR_PER_BUFFER ("indicate-buffer-boundaries",
-		     &BVAR (current_buffer, indicate_buffer_boundaries), Qnil,
+		     indicate_buffer_boundaries, Qnil,
 		     doc: /* Visually indicate buffer boundaries and scrolling.
 If non-nil, the first and last line of the buffer are marked in the fringe
 of a window on window-systems with angle bitmaps, or if the window can be
@@ -6009,7 +6007,7 @@ bitmaps in right fringe.  To show just the angle bitmaps in the left
 fringe, but no arrow bitmaps, use ((top .  left) (bottom . left)).  */);
 
   DEFVAR_PER_BUFFER ("fringe-indicator-alist",
-		     &BVAR (current_buffer, fringe_indicator_alist), Qnil,
+		     fringe_indicator_alist, Qnil,
 		     doc: /* Mapping from logical to physical fringe indicator bitmaps.
 The value is an alist where each element (INDICATOR . BITMAPS)
 specifies the fringe bitmaps used to display a specific logical
@@ -6028,7 +6026,7 @@ last (only) line has no final newline.  BITMAPS may also be a single
 symbol which is used in both left and right fringes.  */);
 
   DEFVAR_PER_BUFFER ("fringe-cursor-alist",
-		     &BVAR (current_buffer, fringe_cursor_alist), Qnil,
+		     fringe_cursor_alist, Qnil,
 		     doc: /* Mapping from logical to physical fringe cursor bitmaps.
 The value is an alist where each element (CURSOR . BITMAP)
 specifies the fringe bitmaps used to display a specific logical
@@ -6043,7 +6041,7 @@ BITMAP is the corresponding fringe bitmap shown for the logical
 cursor type.  */);
 
   DEFVAR_PER_BUFFER ("scroll-up-aggressively",
-		     &BVAR (current_buffer, scroll_up_aggressively), Qfraction,
+		     scroll_up_aggressively, Qfraction,
 		     doc: /* How far to scroll windows upward.
 If you move point off the bottom, the window scrolls automatically.
 This variable controls how far it scrolls.  The value nil, the default,
@@ -6056,7 +6054,7 @@ window scrolls by a full window height.  Meaningful values are
 between 0.0 and 1.0, inclusive.  */);
 
   DEFVAR_PER_BUFFER ("scroll-down-aggressively",
-		     &BVAR (current_buffer, scroll_down_aggressively), Qfraction,
+		     scroll_down_aggressively, Qfraction,
 		     doc: /* How far to scroll windows downward.
 If you move point off the top, the window scrolls automatically.
 This variable controls how far it scrolls.  The value nil, the default,
@@ -6107,7 +6105,7 @@ from happening repeatedly and making Emacs nonfunctional.  */);
 The functions are run using the `run-hooks' function.  */);
   Vfirst_change_hook = Qnil;
 
-  DEFVAR_PER_BUFFER ("buffer-undo-list", &BVAR (current_buffer, undo_list), Qnil,
+  DEFVAR_PER_BUFFER ("buffer-undo-list", undo_list, Qnil,
 		     doc: /* List of undo entries in current buffer.
 Recent changes come first; older changes follow newer.
 
@@ -6153,10 +6151,10 @@ the changes between two undo boundaries as a single step to be undone.
 
 If the value of the variable is t, undo information is not recorded.  */);
 
-  DEFVAR_PER_BUFFER ("mark-active", &BVAR (current_buffer, mark_active), Qnil,
+  DEFVAR_PER_BUFFER ("mark-active", mark_active, Qnil,
 		     doc: /* Non-nil means the mark and region are currently active in this buffer.  */);
 
-  DEFVAR_PER_BUFFER ("cache-long-scans", &BVAR (current_buffer, cache_long_scans), Qnil,
+  DEFVAR_PER_BUFFER ("cache-long-scans", cache_long_scans, Qnil,
 		     doc: /* Non-nil means that Emacs should use caches in attempt to speedup buffer scans.
 
 There is no reason to set this to nil except for debugging purposes.
@@ -6192,23 +6190,23 @@ maintained internally by the Emacs primitives.  Enabling or disabling
 the cache should not affect the behavior of any of the motion
 functions; it should only affect their performance.  */);
 
-  DEFVAR_PER_BUFFER ("point-before-scroll", &BVAR (current_buffer, point_before_scroll), Qnil,
+  DEFVAR_PER_BUFFER ("point-before-scroll", point_before_scroll, Qnil,
 		     doc: /* Value of point before the last series of scroll operations, or nil.  */);
 
-  DEFVAR_PER_BUFFER ("buffer-file-format", &BVAR (current_buffer, file_format), Qnil,
+  DEFVAR_PER_BUFFER ("buffer-file-format", file_format, Qnil,
 		     doc: /* List of formats to use when saving this buffer.
 Formats are defined by `format-alist'.  This variable is
 set when a file is visited.  */);
 
   DEFVAR_PER_BUFFER ("buffer-auto-save-file-format",
-		     &BVAR (current_buffer, auto_save_file_format), Qnil,
+		     auto_save_file_format, Qnil,
 		     doc: /* Format in which to write auto-save files.
 Should be a list of symbols naming formats that are defined in `format-alist'.
 If it is t, which is the default, auto-save files are written in the
 same format as a regular save would use.  */);
 
   DEFVAR_PER_BUFFER ("buffer-invisibility-spec",
-		     &BVAR (current_buffer, invisibility_spec), Qnil,
+		     invisibility_spec, Qnil,
 		     doc: /* Invisibility spec of this buffer.
 The default is t, which means that text is invisible if it has a non-nil
 `invisible' property.
@@ -6222,12 +6220,12 @@ Setting this variable is very fast, much faster than scanning all the text in
 the buffer looking for properties to change.  */);
 
   DEFVAR_PER_BUFFER ("buffer-display-count",
-		     &BVAR (current_buffer, display_count), Qintegerp,
+		     display_count, Qintegerp,
 		     doc: /* A number incremented each time this buffer is displayed in a window.
 The function `set-window-buffer' increments it.  */);
 
   DEFVAR_PER_BUFFER ("buffer-display-time",
-		     &BVAR (current_buffer, display_time), Qnil,
+		     display_time, Qnil,
 		     doc: /* Time stamp updated each time this buffer is displayed in a window.
 The function `set-window-buffer' updates this variable
 to the value obtained by calling `current-time'.
@@ -6263,7 +6261,7 @@ member of the list.  Any other non-nil value means disregard `buffer-read-only'
 and all `read-only' text properties.  */);
   Vinhibit_read_only = Qnil;
 
-  DEFVAR_PER_BUFFER ("cursor-type", &BVAR (current_buffer, cursor_type), Qnil,
+  DEFVAR_PER_BUFFER ("cursor-type", cursor_type, Qnil,
 		     doc: /* Cursor to use when this buffer is in the selected window.
 Values are interpreted as follows:
 
@@ -6287,7 +6285,7 @@ cursor's appearance is instead controlled by the variable
 `cursor-in-non-selected-windows'.  */);
 
   DEFVAR_PER_BUFFER ("line-spacing",
-		     &BVAR (current_buffer, extra_line_spacing), Qnumberp,
+		     extra_line_spacing, Qnumberp,
 		     doc: /* Additional space to put between lines when displaying a buffer.
 The space is measured in pixels, and put below lines on graphic displays,
 see `display-graphic-p'.
@@ -6295,7 +6293,7 @@ If value is a floating point number, it specifies the spacing relative
 to the default frame line height.  A value of nil means add no extra space.  */);
 
   DEFVAR_PER_BUFFER ("cursor-in-non-selected-windows",
-		     &BVAR (current_buffer, cursor_in_non_selected_windows), Qnil,
+		     cursor_in_non_selected_windows, Qnil,
 		     doc: /* Non-nil means show a cursor in non-selected windows.
 If nil, only shows a cursor in the selected window.
 If t, displays a cursor related to the usual cursor type
-- 
2.28.0

[PATCH 02/10] Add bset_save_length and use it

[Spencer Baugh]

This removes some usage of BVAR as an lvalue, necessary for changing
BVAR into a function call.
---
 src/buffer.c |  9 ++-------
 src/buffer.h |  5 +++++
 src/fileio.c | 12 ++++++------
 3 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/src/buffer.c b/src/buffer.c
index 64d0a74fb0..e88bf23fad 100644
--- a/src/buffer.c
+++ b/src/buffer.c
@@ -332,11 +332,6 @@ bset_right_fringe_width (struct buffer *b, Lisp_Object val)
   b->right_fringe_width_ = val;
 }
 static void
-bset_save_length (struct buffer *b, Lisp_Object val)
-{
-  b->save_length_ = val;
-}
-static void
 bset_scroll_bar_width (struct buffer *b, Lisp_Object val)
 {
   b->scroll_bar_width_ = val;
@@ -956,7 +951,7 @@ reset_buffer (register struct buffer *b)
   bset_directory (b, current_buffer ? BVAR (current_buffer, directory) : Qnil);
   b->modtime = make_timespec (0, UNKNOWN_MODTIME_NSECS);
   b->modtime_size = -1;
-  XSETFASTINT (BVAR (b, save_length), 0);
+  bset_save_length (b, make_fixed_natnum (0));
   b->last_window_start = 1;
   /* It is more conservative to start out "changed" than "unchanged".  */
   b->clip_changed = 0;
@@ -2282,7 +2277,7 @@ so the buffer is truly empty after this.  */)
   /* Prevent warnings, or suspension of auto saving, that would happen
      if future size is less than past size.  Use of erase-buffer
      implies that the future text is not really related to the past text.  */
-  XSETFASTINT (BVAR (current_buffer, save_length), 0);
+  bset_save_length (current_buffer, make_fixed_natnum (0));
   return Qnil;
 }
 
diff --git a/src/buffer.h b/src/buffer.h
index 3da49414bb..690593757e 100644
--- a/src/buffer.h
+++ b/src/buffer.h
@@ -824,6 +824,11 @@ bset_width_table (struct buffer *b, Lisp_Object val)
 {
   b->width_table_ = val;
 }
+INLINE void
+bset_save_length (struct buffer *b, Lisp_Object val)
+{
+  b->save_length_ = val;
+}
 
 /* BUFFER_CEILING_OF (resp. BUFFER_FLOOR_OF), when applied to n, return
    the max (resp. min) p such that
diff --git a/src/fileio.c b/src/fileio.c
index 283813ff89..ba094a5705 100644
--- a/src/fileio.c
+++ b/src/fileio.c
@@ -4697,7 +4697,7 @@ by calling `format-decode', which see.  */)
 
       SAVE_MODIFF = MODIFF;
       BUF_AUTOSAVE_MODIFF (current_buffer) = MODIFF;
-      XSETFASTINT (BVAR (current_buffer, save_length), Z - BEG);
+      bset_save_length (current_buffer, make_fixed_natnum (Z - BEG));
       if (NILP (handler))
 	{
 	  if (!NILP (BVAR (current_buffer, file_truename)))
@@ -5110,7 +5110,7 @@ write_region (Lisp_Object start, Lisp_Object end, Lisp_Object filename,
       if (visiting)
 	{
 	  SAVE_MODIFF = MODIFF;
-	  XSETFASTINT (BVAR (current_buffer, save_length), Z - BEG);
+          bset_save_length (current_buffer, make_fixed_natnum (Z - BEG));
 	  bset_filename (current_buffer, visit_file);
 	}
 
@@ -5361,7 +5361,7 @@ write_region (Lisp_Object start, Lisp_Object end, Lisp_Object filename,
   if (visiting)
     {
       SAVE_MODIFF = MODIFF;
-      XSETFASTINT (BVAR (current_buffer, save_length), Z - BEG);
+      bset_save_length (current_buffer, make_fixed_natnum (Z - BEG));
       bset_filename (current_buffer, visit_file);
       update_mode_lines = 14;
       if (auto_saving_into_visited_file)
@@ -5979,7 +5979,7 @@ A non-nil CURRENT-ONLY argument means save only current buffer.  */)
 		minibuffer_auto_raise = 0;
 		/* Turn off auto-saving until there's a real save,
 		   and prevent any more warnings.  */
-		XSETINT (BVAR (b, save_length), -1);
+                bset_save_length (current_buffer, make_fixed_natnum (-1));
 		Fsleep_for (make_fixnum (1), Qnil);
 		continue;
 	      }
@@ -5988,7 +5988,7 @@ A non-nil CURRENT-ONLY argument means save only current buffer.  */)
 	    internal_condition_case (auto_save_1, Qt, auto_save_error);
 	    auto_saved = 1;
 	    BUF_AUTOSAVE_MODIFF (b) = BUF_MODIFF (b);
-	    XSETFASTINT (BVAR (current_buffer, save_length), Z - BEG);
+            bset_save_length (current_buffer, make_fixed_natnum (Z - BEG));
 	    set_buffer_internal (old);
 
 	    after_time = current_timespec ();
@@ -6034,7 +6034,7 @@ No auto-save file will be written until the buffer changes again.  */)
   /* FIXME: This should not be called in indirect buffers, since
      they're not autosaved.  */
   BUF_AUTOSAVE_MODIFF (current_buffer) = MODIFF;
-  XSETFASTINT (BVAR (current_buffer, save_length), Z - BEG);
+  bset_save_length (current_buffer, make_fixed_natnum (Z - BEG));
   current_buffer->auto_save_failure_time = 0;
   return Qnil;
 }
-- 
2.28.0

[PATCH 03/10] Use bset_last_selected_window everywhere

[Spencer Baugh]

This function already existed; we just needed to use it in more
places.
---
 src/window.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/src/window.c b/src/window.c
index a6de34f3db..d24d1a546f 100644
--- a/src/window.c
+++ b/src/window.c
@@ -7111,9 +7111,8 @@ the return value is nil.  Otherwise the value is t.  */)
 	 Do not record the buffer here.  We do that in a separate call
 	 to select_window below.  See also Bug#16207.  */
       select_window (data->current_window, Qt, true);
-      BVAR (XBUFFER (XWINDOW (selected_window)->contents),
-	    last_selected_window)
-	= selected_window;
+      bset_last_selected_window (XBUFFER (XWINDOW (selected_window)->contents),
+                                 selected_window);
 
       if (NILP (data->focus_frame)
 	  || (FRAMEP (data->focus_frame)
-- 
2.28.0

[PATCH 04/10] Use bset_enable_multibyte_characters everywhere

[Spencer Baugh]

This function already existed; we just needed to use it in more
places.
---
 src/buffer.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/buffer.c b/src/buffer.c
index e88bf23fad..ba03fed3b9 100644
--- a/src/buffer.c
+++ b/src/buffer.c
@@ -2765,8 +2765,8 @@ current buffer is cleared.  */)
       struct buffer *o = XBUFFER (other);
       if (o->base_buffer == current_buffer && BUFFER_LIVE_P (o))
 	{
-	  BVAR (o, enable_multibyte_characters)
-	    = BVAR (current_buffer, enable_multibyte_characters);
+	  bset_enable_multibyte_characters (o,
+            BVAR (current_buffer, enable_multibyte_characters));
 	  o->prevent_redisplay_optimizations_p = true;
 	}
     }
-- 
2.28.0

[PATCH 05/10] Add BVAR_DEFAULT for access to buffer defaults

[Spencer Baugh]

This is more direct. This macro can stay as an lvalue even when we
change BVAR to be a function call - directly changing and accessing
buffer_defaults is always fine.
---
 src/buffer.h   | 3 +++
 src/category.h | 2 +-
 src/syntax.h   | 2 +-
 3 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/src/buffer.h b/src/buffer.h
index 690593757e..0b26c56e89 100644
--- a/src/buffer.h
+++ b/src/buffer.h
@@ -284,6 +284,9 @@ struct buffer_text
 
 #define BVAR(buf, field) ((buf)->field ## _)
 
+/* Access a BVAR from buffer_defaults */
+#define BVAR_DEFAULT(field) (buffer_defaults.field ## _)
+
 /* Max number of builtin per-buffer variables.  */
 enum { MAX_PER_BUFFER_VARS = 50 };
 
diff --git a/src/category.h b/src/category.h
index cc32990478..85e872a940 100644
--- a/src/category.h
+++ b/src/category.h
@@ -94,7 +94,7 @@ CHAR_HAS_CATEGORY (int ch, int category)
 
 /* The standard category table is stored where it will automatically
    be used in all new buffers.  */
-#define Vstandard_category_table BVAR (&buffer_defaults, category_table)
+#define Vstandard_category_table BVAR_DEFAULT (category_table)
 
 /* Return the doc string of CATEGORY in category table TABLE.  */
 #define CATEGORY_DOCSTRING(table, category)				\
diff --git a/src/syntax.h b/src/syntax.h
index a2ec3301ba..9c2bf32057 100644
--- a/src/syntax.h
+++ b/src/syntax.h
@@ -31,7 +31,7 @@ extern void update_syntax_table_forward (ptrdiff_t, bool, Lisp_Object);
 
 /* The standard syntax table is stored where it will automatically
    be used in all new buffers.  */
-#define Vstandard_syntax_table BVAR (&buffer_defaults, syntax_table)
+#define Vstandard_syntax_table BVAR_DEFAULT (syntax_table)
 
 /* A syntax table is a chartable whose elements are cons cells
    (CODE+FLAGS . MATCHING-CHAR).  MATCHING-CHAR can be nil if the char
-- 
2.28.0

[PATCH 06/10] Disallow using BVAR as an lvalue

[Spencer Baugh]

This is a prerequisite for later changing BVAR into a function call.

buffer.c keeps the ability to directly access struct buffer Lisp
fields; it has to change to using the new local macro BVAR_DIRECT for
this. buffer.c exclusively uses this ability to access the special
struct buffer instances: buffer_local_flags, buffer_local_symbols, and
buffer_defaults. None of those should fall back to looking up
variables in buffer_defaults.
---
 src/buffer.c | 104 ++++++++++++++++++++++++++-------------------------
 src/buffer.h |   2 +-
 2 files changed, 54 insertions(+), 52 deletions(-)

diff --git a/src/buffer.c b/src/buffer.c
index ba03fed3b9..6bcf1ad596 100644
--- a/src/buffer.c
+++ b/src/buffer.c
@@ -89,6 +89,8 @@ struct buffer buffer_local_symbols;
 #define PER_BUFFER_SYMBOL(OFFSET) \
       (*(Lisp_Object *)((OFFSET) + (char *) &buffer_local_symbols))
 
+#define BVAR_DIRECT(buf, field) ((buf)->field ## _)
+
 /* Maximum length of an overlay vector.  */
 #define OVERLAY_COUNT_MAX						\
   ((ptrdiff_t) min (MOST_POSITIVE_FIXNUM,				\
@@ -5183,53 +5185,53 @@ init_buffer_once (void)
   bset_last_selected_window (&buffer_local_flags, make_fixnum (0));
 
   idx = 1;
-  XSETFASTINT (BVAR (&buffer_local_flags, mode_line_format), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, abbrev_mode), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, overwrite_mode), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, case_fold_search), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, auto_fill_function), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, selective_display), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, selective_display_ellipses), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, tab_width), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, truncate_lines), idx);
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, mode_line_format), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, abbrev_mode), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, overwrite_mode), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, case_fold_search), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, auto_fill_function), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, selective_display), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, selective_display_ellipses), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, tab_width), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, truncate_lines), idx);
   /* Make this one a permanent local.  */
   buffer_permanent_local_flags[idx++] = 1;
-  XSETFASTINT (BVAR (&buffer_local_flags, word_wrap), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, ctl_arrow), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, fill_column), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, left_margin), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, abbrev_table), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, display_table), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, syntax_table), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, cache_long_scans), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, category_table), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, bidi_display_reordering), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, bidi_paragraph_direction), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, bidi_paragraph_separate_re), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, bidi_paragraph_start_re), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, buffer_file_coding_system), idx);
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, word_wrap), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, ctl_arrow), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, fill_column), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, left_margin), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, abbrev_table), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, display_table), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, syntax_table), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, cache_long_scans), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, category_table), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, bidi_display_reordering), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, bidi_paragraph_direction), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, bidi_paragraph_separate_re), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, bidi_paragraph_start_re), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, buffer_file_coding_system), idx);
   /* Make this one a permanent local.  */
   buffer_permanent_local_flags[idx++] = 1;
-  XSETFASTINT (BVAR (&buffer_local_flags, left_margin_cols), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, right_margin_cols), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, left_fringe_width), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, right_fringe_width), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, fringes_outside_margins), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, scroll_bar_width), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, scroll_bar_height), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, vertical_scroll_bar_type), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, horizontal_scroll_bar_type), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, indicate_empty_lines), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, indicate_buffer_boundaries), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, fringe_indicator_alist), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, fringe_cursor_alist), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, scroll_up_aggressively), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, scroll_down_aggressively), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, header_line_format), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, tab_line_format), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, cursor_type), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, extra_line_spacing), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, cursor_in_non_selected_windows), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, left_margin_cols), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, right_margin_cols), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, left_fringe_width), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, right_fringe_width), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, fringes_outside_margins), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, scroll_bar_width), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, scroll_bar_height), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, vertical_scroll_bar_type), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, horizontal_scroll_bar_type), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, indicate_empty_lines), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, indicate_buffer_boundaries), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, fringe_indicator_alist), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, fringe_cursor_alist), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, scroll_up_aggressively), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, scroll_down_aggressively), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, header_line_format), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, tab_line_format), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, cursor_type), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, extra_line_spacing), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, cursor_in_non_selected_windows), idx); ++idx;
 
   /* buffer_local_flags contains no pointers, so it's safe to treat it
      as a blob for pdumper.  */
@@ -5244,9 +5246,9 @@ init_buffer_once (void)
   /* Make sure all markable slots in buffer_defaults
      are initialized reasonably, so mark_buffer won't choke.  */
   reset_buffer (&buffer_defaults);
-  eassert (NILP (BVAR (&buffer_defaults, name)));
+  eassert (NILP (BVAR_DIRECT (&buffer_defaults, name)));
   reset_buffer_local_variables (&buffer_defaults, 1);
-  eassert (NILP (BVAR (&buffer_local_symbols, name)));
+  eassert (NILP (BVAR_DIRECT (&buffer_local_symbols, name)));
   reset_buffer (&buffer_local_symbols);
   reset_buffer_local_variables (&buffer_local_symbols, 1);
   /* Prevent GC from getting confused.  */
@@ -5289,7 +5291,7 @@ init_buffer_once (void)
   set_buffer_overlays_after (&buffer_defaults, NULL);
   buffer_defaults.overlay_center = BEG;
 
-  XSETFASTINT (BVAR (&buffer_defaults, tab_width), 8);
+  XSETFASTINT (BVAR_DIRECT (&buffer_defaults, tab_width), 8);
   bset_truncate_lines (&buffer_defaults, Qnil);
   bset_word_wrap (&buffer_defaults, Qnil);
   bset_ctl_arrow (&buffer_defaults, Qt);
@@ -5303,13 +5305,13 @@ init_buffer_once (void)
 
   bset_enable_multibyte_characters (&buffer_defaults, Qt);
   bset_buffer_file_coding_system (&buffer_defaults, Qnil);
-  XSETFASTINT (BVAR (&buffer_defaults, fill_column), 70);
-  XSETFASTINT (BVAR (&buffer_defaults, left_margin), 0);
+  XSETFASTINT (BVAR_DIRECT (&buffer_defaults, fill_column), 70);
+  XSETFASTINT (BVAR_DIRECT (&buffer_defaults, left_margin), 0);
   bset_cache_long_scans (&buffer_defaults, Qt);
   bset_file_truename (&buffer_defaults, Qnil);
-  XSETFASTINT (BVAR (&buffer_defaults, display_count), 0);
-  XSETFASTINT (BVAR (&buffer_defaults, left_margin_cols), 0);
-  XSETFASTINT (BVAR (&buffer_defaults, right_margin_cols), 0);
+  XSETFASTINT (BVAR_DIRECT (&buffer_defaults, display_count), 0);
+  XSETFASTINT (BVAR_DIRECT (&buffer_defaults, left_margin_cols), 0);
+  XSETFASTINT (BVAR_DIRECT (&buffer_defaults, right_margin_cols), 0);
   bset_left_fringe_width (&buffer_defaults, Qnil);
   bset_right_fringe_width (&buffer_defaults, Qnil);
   bset_fringes_outside_margins (&buffer_defaults, Qnil);
diff --git a/src/buffer.h b/src/buffer.h
index 0b26c56e89..ae2d785aff 100644
--- a/src/buffer.h
+++ b/src/buffer.h
@@ -282,7 +282,7 @@ struct buffer_text
 
 /* Most code should use this macro to access Lisp fields in struct buffer.  */
 
-#define BVAR(buf, field) ((buf)->field ## _)
+#define BVAR(buf, field) ((void)0, (buf)->field ## _)
 
 /* Access a BVAR from buffer_defaults */
 #define BVAR_DEFAULT(field) (buffer_defaults.field ## _)
-- 
2.28.0

[PATCH 07/10] Reorder buffer.h for upcoming rework of BVAR

[Spencer Baugh]

This is just rearranging these constants so that everything is
properly in scope for the new definition of BVAR.
---
 src/buffer.h | 288 +++++++++++++++++++++++++--------------------------
 1 file changed, 144 insertions(+), 144 deletions(-)

diff --git a/src/buffer.h b/src/buffer.h
index ae2d785aff..6e384640b7 100644
--- a/src/buffer.h
+++ b/src/buffer.h
@@ -280,13 +280,6 @@ struct buffer_text
     bool_bf redisplay : 1;
   };
 
-/* Most code should use this macro to access Lisp fields in struct buffer.  */
-
-#define BVAR(buf, field) ((void)0, (buf)->field ## _)
-
-/* Access a BVAR from buffer_defaults */
-#define BVAR_DEFAULT(field) (buffer_defaults.field ## _)
-
 /* Max number of builtin per-buffer variables.  */
 enum { MAX_PER_BUFFER_VARS = 50 };
 
@@ -695,6 +688,150 @@ struct buffer
   Lisp_Object undo_list_;
 };
 
+/* This structure holds the default values of the buffer-local variables
+   that have special slots in each buffer.
+   The default value occupies the same slot in this structure
+   as an individual buffer's value occupies in that buffer.
+   Setting the default value also goes through the alist of buffers
+   and stores into each buffer that does not say it has a local value.  */
+
+extern struct buffer buffer_defaults;
+
+/* This structure marks which slots in a buffer have corresponding
+   default values in buffer_defaults.
+   Each such slot has a nonzero value in this structure.
+   The value has only one nonzero bit.
+
+   When a buffer has its own local value for a slot,
+   the entry for that slot (found in the same slot in this structure)
+   is turned on in the buffer's local_flags array.
+
+   If a slot in this structure is zero, then even though there may
+   be a Lisp-level local variable for the slot, it has no default value,
+   and the corresponding slot in buffer_defaults is not used.  */
+
+
+extern struct buffer buffer_local_flags;
+
+/* For each buffer slot, this points to the Lisp symbol name
+   for that slot in the current buffer.  It is 0 for slots
+   that don't have such names.  */
+
+extern struct buffer buffer_local_symbols;
+
+/***********************************************************************
+			Buffer-local Variables
+ ***********************************************************************/
+
+/* Return the offset in bytes of member VAR of struct buffer
+   from the start of a buffer structure.  */
+
+#define PER_BUFFER_VAR_OFFSET(VAR) \
+  offsetof (struct buffer, VAR ## _)
+
+/* Used to iterate over normal Lisp_Object fields of struct buffer (all
+   Lisp_Objects except undo_list).  If you add, remove, or reorder
+   Lisp_Objects in a struct buffer, make sure that this is still correct.  */
+
+#define FOR_EACH_PER_BUFFER_OBJECT_AT(offset)				 \
+  for (offset = PER_BUFFER_VAR_OFFSET (name);				 \
+       offset <= PER_BUFFER_VAR_OFFSET (cursor_in_non_selected_windows); \
+       offset += word_size)
+
+/* Return the index of buffer-local variable VAR.  Each per-buffer
+   variable has an index > 0 associated with it, except when it always
+   has buffer-local values, in which case the index is -1.  If this is
+   0, this is a bug and means that the slot of VAR in
+   buffer_local_flags wasn't initialized.  */
+
+#define PER_BUFFER_VAR_IDX(VAR) \
+    PER_BUFFER_IDX (PER_BUFFER_VAR_OFFSET (VAR))
+
+extern bool valid_per_buffer_idx (int);
+
+/* Value is true if the variable with index IDX has a local value
+   in buffer B.  */
+
+INLINE bool
+PER_BUFFER_VALUE_P (struct buffer *b, int idx)
+{
+  eassert (valid_per_buffer_idx (idx));
+  return b->local_flags[idx];
+}
+
+/* Set whether per-buffer variable with index IDX has a buffer-local
+   value in buffer B.  VAL zero means it hasn't.  */
+
+INLINE void
+SET_PER_BUFFER_VALUE_P (struct buffer *b, int idx, bool val)
+{
+  eassert (valid_per_buffer_idx (idx));
+  b->local_flags[idx] = val;
+}
+
+/* Return the index value of the per-buffer variable at offset OFFSET
+   in the buffer structure.
+
+   If the slot OFFSET has a corresponding default value in
+   buffer_defaults, the index value is positive and has only one
+   nonzero bit.  When a buffer has its own local value for a slot, the
+   bit for that slot (found in the same slot in this structure) is
+   turned on in the buffer's local_flags array.
+
+   If the index value is -1, even though there may be a
+   DEFVAR_PER_BUFFER for the slot, there is no default value for it;
+   and the corresponding slot in buffer_defaults is not used.
+
+   If the index value is -2, then there is no DEFVAR_PER_BUFFER for
+   the slot, but there is a default value which is copied into each
+   new buffer.
+
+   If a slot in this structure corresponding to a DEFVAR_PER_BUFFER is
+   zero, that is a bug.  */
+
+INLINE int
+PER_BUFFER_IDX (ptrdiff_t offset)
+{
+  return XFIXNUM (*(Lisp_Object *) (offset + (char *) &buffer_local_flags));
+}
+
+/* Functions to get and set default value of the per-buffer
+   variable at offset OFFSET in the buffer structure.  */
+
+INLINE Lisp_Object
+per_buffer_default (int offset)
+{
+  return *(Lisp_Object *)(offset + (char *) &buffer_defaults);
+}
+
+INLINE void
+set_per_buffer_default (int offset, Lisp_Object value)
+{
+  *(Lisp_Object *)(offset + (char *) &buffer_defaults) = value;
+}
+
+/* Functions to get and set buffer-local value of the per-buffer
+   variable at offset OFFSET in the buffer structure.  */
+
+INLINE Lisp_Object
+per_buffer_value (struct buffer *b, int offset)
+{
+  return *(Lisp_Object *)(offset + (char *) b);
+}
+
+INLINE void
+set_per_buffer_value (struct buffer *b, int offset, Lisp_Object value)
+{
+  *(Lisp_Object *)(offset + (char *) b) = value;
+}
+
+/* Most code should use this macro to access Lisp fields in struct buffer.  */
+
+#define BVAR(buf, field) ((void)0, (buf)->field ## _)
+
+/* Access a BVAR from buffer_defaults */
+#define BVAR_DEFAULT(field) (buffer_defaults.field ## _)
+
 INLINE bool
 BUFFERP (Lisp_Object a)
 {
@@ -1109,37 +1246,6 @@ BUFFER_CHECK_INDIRECTION (struct buffer *b)
     }
 }
 
-/* This structure holds the default values of the buffer-local variables
-   that have special slots in each buffer.
-   The default value occupies the same slot in this structure
-   as an individual buffer's value occupies in that buffer.
-   Setting the default value also goes through the alist of buffers
-   and stores into each buffer that does not say it has a local value.  */
-
-extern struct buffer buffer_defaults;
-
-/* This structure marks which slots in a buffer have corresponding
-   default values in buffer_defaults.
-   Each such slot has a nonzero value in this structure.
-   The value has only one nonzero bit.
-
-   When a buffer has its own local value for a slot,
-   the entry for that slot (found in the same slot in this structure)
-   is turned on in the buffer's local_flags array.
-
-   If a slot in this structure is zero, then even though there may
-   be a Lisp-level local variable for the slot, it has no default value,
-   and the corresponding slot in buffer_defaults is not used.  */
-
-
-extern struct buffer buffer_local_flags;
-
-/* For each buffer slot, this points to the Lisp symbol name
-   for that slot in the current buffer.  It is 0 for slots
-   that don't have such names.  */
-
-extern struct buffer buffer_local_symbols;
-
 /* verify_interval_modification saves insertion hooks here
    to be run later by report_interval_modification.  */
 extern Lisp_Object interval_insert_behind_hooks;
@@ -1393,112 +1499,6 @@ OVERLAY_POSITION (Lisp_Object p)
 }
 
 \f
-/***********************************************************************
-			Buffer-local Variables
- ***********************************************************************/
-
-/* Return the offset in bytes of member VAR of struct buffer
-   from the start of a buffer structure.  */
-
-#define PER_BUFFER_VAR_OFFSET(VAR) \
-  offsetof (struct buffer, VAR ## _)
-
-/* Used to iterate over normal Lisp_Object fields of struct buffer (all
-   Lisp_Objects except undo_list).  If you add, remove, or reorder
-   Lisp_Objects in a struct buffer, make sure that this is still correct.  */
-
-#define FOR_EACH_PER_BUFFER_OBJECT_AT(offset)				 \
-  for (offset = PER_BUFFER_VAR_OFFSET (name);				 \
-       offset <= PER_BUFFER_VAR_OFFSET (cursor_in_non_selected_windows); \
-       offset += word_size)
-
-/* Return the index of buffer-local variable VAR.  Each per-buffer
-   variable has an index > 0 associated with it, except when it always
-   has buffer-local values, in which case the index is -1.  If this is
-   0, this is a bug and means that the slot of VAR in
-   buffer_local_flags wasn't initialized.  */
-
-#define PER_BUFFER_VAR_IDX(VAR) \
-    PER_BUFFER_IDX (PER_BUFFER_VAR_OFFSET (VAR))
-
-extern bool valid_per_buffer_idx (int);
-
-/* Value is true if the variable with index IDX has a local value
-   in buffer B.  */
-
-INLINE bool
-PER_BUFFER_VALUE_P (struct buffer *b, int idx)
-{
-  eassert (valid_per_buffer_idx (idx));
-  return b->local_flags[idx];
-}
-
-/* Set whether per-buffer variable with index IDX has a buffer-local
-   value in buffer B.  VAL zero means it hasn't.  */
-
-INLINE void
-SET_PER_BUFFER_VALUE_P (struct buffer *b, int idx, bool val)
-{
-  eassert (valid_per_buffer_idx (idx));
-  b->local_flags[idx] = val;
-}
-
-/* Return the index value of the per-buffer variable at offset OFFSET
-   in the buffer structure.
-
-   If the slot OFFSET has a corresponding default value in
-   buffer_defaults, the index value is positive and has only one
-   nonzero bit.  When a buffer has its own local value for a slot, the
-   bit for that slot (found in the same slot in this structure) is
-   turned on in the buffer's local_flags array.
-
-   If the index value is -1, even though there may be a
-   DEFVAR_PER_BUFFER for the slot, there is no default value for it;
-   and the corresponding slot in buffer_defaults is not used.
-
-   If the index value is -2, then there is no DEFVAR_PER_BUFFER for
-   the slot, but there is a default value which is copied into each
-   new buffer.
-
-   If a slot in this structure corresponding to a DEFVAR_PER_BUFFER is
-   zero, that is a bug.  */
-
-INLINE int
-PER_BUFFER_IDX (ptrdiff_t offset)
-{
-  return XFIXNUM (*(Lisp_Object *) (offset + (char *) &buffer_local_flags));
-}
-
-/* Functions to get and set default value of the per-buffer
-   variable at offset OFFSET in the buffer structure.  */
-
-INLINE Lisp_Object
-per_buffer_default (int offset)
-{
-  return *(Lisp_Object *)(offset + (char *) &buffer_defaults);
-}
-
-INLINE void
-set_per_buffer_default (int offset, Lisp_Object value)
-{
-  *(Lisp_Object *)(offset + (char *) &buffer_defaults) = value;
-}
-
-/* Functions to get and set buffer-local value of the per-buffer
-   variable at offset OFFSET in the buffer structure.  */
-
-INLINE Lisp_Object
-per_buffer_value (struct buffer *b, int offset)
-{
-  return *(Lisp_Object *)(offset + (char *) b);
-}
-
-INLINE void
-set_per_buffer_value (struct buffer *b, int offset, Lisp_Object value)
-{
-  *(Lisp_Object *)(offset + (char *) b) = value;
-}
-
 /* Downcase a character C, or make no change if that cannot be done.  */
 INLINE int
 downcase (int c)
-- 
2.28.0

[PATCH 08/10] Make cache_long_scans buffer-local when setting it

[Spencer Baugh]

If we set a value for a variable on some buffer, it necessarily means
that that value is buffer-local - it might be, and probably is,
different from what's in buffer_defaults.

Without further changes, I think this has no effect. But it's required
for correct behavior when we change BVAR to fall back to
buffer_defaults for non-buffer-local variables; without this change,
we would always use buffer_defaults for cache_long_scans even though
it's been set in a per-buffer way.
---
 src/buffer.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/buffer.h b/src/buffer.h
index 6e384640b7..077edb893a 100644
--- a/src/buffer.h
+++ b/src/buffer.h
@@ -863,6 +863,7 @@ INLINE void
 bset_cache_long_scans (struct buffer *b, Lisp_Object val)
 {
   b->cache_long_scans_ = val;
+  SET_PER_BUFFER_VALUE_P (b, PER_BUFFER_VAR_IDX(cache_long_scans), 1);
 }
 INLINE void
 bset_case_canon_table (struct buffer *b, Lisp_Object val)
-- 
2.28.0

[PATCH 09/10] Access buffer_defaults in BVAR if there's no local binding

[Spencer Baugh]

In SHOULD_USE_BUFFER_DEFAULTS, we have to extract the buffer field
index from buffer_local_flags.

If the buffer_local_flags Lisp_Object is 0, though, it means this
field in buffer_local_flags wasn't initialized, which means this field
is always buffer-local (and therefore doesn't use buffer_defaults).

Otherwise, it should be a fixnum. If it's less than 1, it also means
that this field is always buffer-local, and we shouldn't use
buffer_defaults.

Finally, if it's 1 or greater, it's a valid index into local_flags. If
local_flags is 0, that means there's no local value for this buffer
variable. Which means we should use buffer_defaults.
---
 src/buffer.h | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/src/buffer.h b/src/buffer.h
index 077edb893a..d0e44c35bb 100644
--- a/src/buffer.h
+++ b/src/buffer.h
@@ -825,9 +825,31 @@ set_per_buffer_value (struct buffer *b, int offset, Lisp_Object value)
   *(Lisp_Object *)(offset + (char *) b) = value;
 }
 
+INLINE bool
+SHOULD_USE_BUFFER_DEFAULTS(struct buffer *b, ptrdiff_t offset)
+{
+  Lisp_Object obj = *((Lisp_Object *) (offset + (char *) &buffer_local_flags));
+  if (obj.i == 0)
+    return false;
+  int idx = XFIXNUM(obj);
+  if (idx < 1)
+    return false;
+  return b->local_flags[idx] == 0;
+}
+
+INLINE Lisp_Object
+bvar_get(struct buffer *b, ptrdiff_t offset)
+{
+  if (SHOULD_USE_BUFFER_DEFAULTS(b, offset)) {
+    return per_buffer_value(&buffer_defaults, offset);
+  } else {
+    return per_buffer_value(b, offset);
+  }
+}
+
 /* Most code should use this macro to access Lisp fields in struct buffer.  */
 
-#define BVAR(buf, field) ((void)0, (buf)->field ## _)
+#define BVAR(buf, field) bvar_get(buf, PER_BUFFER_VAR_OFFSET (field))
 
 /* Access a BVAR from buffer_defaults */
 #define BVAR_DEFAULT(field) (buffer_defaults.field ## _)
-- 
2.28.0

[PATCH 10/10] Don't iterate over all buffers in set_default_internal

[Spencer Baugh]

The main change here is in set_default_internal, where we remove the
loop over all live buffers.  Instead, we only set the default value.

There are some followup changes implied by that.

When looking up a BUFFER_OBJFWDP variable, we now need to call
bvar_get, so that we look at the default value, instead of calling
per_buffer_value, which now won't be updated by LET_DEFAULT.

More complex is the change to set_internal.
Previously, if we were in the following scenario, which we'll refer to
as scenario A:
- We are in the scope of a let_default binding for some variable,
- the current buffer is the buffer which created that let binding,
- and we call setq for that variable.
The variable would not become buffer-local after the setq, because of
this check in set_internal.  But, nevertheless, the value we'd see for
the variable would still appear different in the current buffer from
other buffers. See the first, minor bug mentioned in bug#44733.

This wouldn't work with these changes, because merely setting a value
in struct buffer is not enough to change the value visible to Lisp:
the variable also must become buffer-local.

By removing this check in set_internal, the variable will become
buffer-local when set in scenario A, so its binding will be visible.

Finally, the change to do_one_unbind is required by the change to
set_internal, to preserve the old unwinding behavior of let_default
bindings.

The old behavior of let_default is that if we setq in a buffer other
than the one which created the let_default binding, our binding
wouldn't be unwound after the let_default was done.  But if we setq in
the buffer that created the let_default binding (that is, in scenario
A) then that binding would be unwound.

Previously, this unwinding behavior was implemented by
set_default_internal.  Now, this change to do_one_unbind makes it
perform this unwinding in the buffer that created the let_default
binding.
---
 src/buffer.c |  2 +-
 src/data.c   | 30 +++---------------------------
 src/eval.c   | 18 ++++++++++++++++--
 3 files changed, 20 insertions(+), 30 deletions(-)

diff --git a/src/buffer.c b/src/buffer.c
index 6bcf1ad596..a8e00ca876 100644
--- a/src/buffer.c
+++ b/src/buffer.c
@@ -1251,7 +1251,7 @@ buffer_local_value (Lisp_Object variable, Lisp_Object buffer)
       {
 	lispfwd fwd = SYMBOL_FWD (sym);
 	if (BUFFER_OBJFWDP (fwd))
-	  result = per_buffer_value (buf, XBUFFER_OBJFWD (fwd)->offset);
+          result = bvar_get (buf, XBUFFER_OBJFWD (fwd)->offset);
 	else
 	  result = Fdefault_value (variable);
 	break;
diff --git a/src/data.c b/src/data.c
index 6558985668..49a29092f9 100644
--- a/src/data.c
+++ b/src/data.c
@@ -1004,8 +1004,8 @@ do_symval_forwarding (lispfwd valcontents)
       return *XOBJFWD (valcontents)->objvar;
 
     case Lisp_Fwd_Buffer_Obj:
-      return per_buffer_value (current_buffer,
-			       XBUFFER_OBJFWD (valcontents)->offset);
+      return bvar_get (current_buffer,
+                       XBUFFER_OBJFWD (valcontents)->offset);
 
     case Lisp_Fwd_Kboard_Obj:
       /* We used to simply use current_kboard here, but from Lisp
@@ -1441,8 +1441,7 @@ set_internal (Lisp_Object symbol, Lisp_Object newval, Lisp_Object where,
 	    int offset = XBUFFER_OBJFWD (innercontents)->offset;
 	    int idx = PER_BUFFER_IDX (offset);
 	    if (idx > 0
-                && bindflag == SET_INTERNAL_SET
-		&& !let_shadows_buffer_binding_p (sym))
+                && bindflag == SET_INTERNAL_SET)
 	      SET_PER_BUFFER_VALUE_P (buf, idx, 1);
 	  }
 
@@ -1716,30 +1715,7 @@ set_default_internal (Lisp_Object symbol, Lisp_Object value,
 	if (BUFFER_OBJFWDP (valcontents))
 	  {
 	    int offset = XBUFFER_OBJFWD (valcontents)->offset;
-	    int idx = PER_BUFFER_IDX (offset);
-
 	    set_per_buffer_default (offset, value);
-
-	    /* If this variable is not always local in all buffers,
-	       set it in the buffers that don't nominally have a local value.  */
-	    if (idx > 0)
-	      {
-		Lisp_Object buf, tail;
-
-		/* Do this only in live buffers, so that if there are
-		   a lot of buffers which are dead, that doesn't slow
-		   down let-binding of variables that are
-		   automatically local when set, like
-		   case-fold-search.  This is for Lisp programs that
-		   let-bind such variables in their inner loops.  */
-		FOR_EACH_LIVE_BUFFER (tail, buf)
-		  {
-		    struct buffer *b = XBUFFER (buf);
-
-		    if (!PER_BUFFER_VALUE_P (b, idx))
-		      set_per_buffer_value (b, offset, value);
-		  }
-	      }
 	  }
 	else
           set_internal (symbol, value, Qnil, bindflag);
diff --git a/src/eval.c b/src/eval.c
index 76708e6e7e..7a8e314550 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -3497,12 +3497,26 @@ do_one_unbind (union specbinding *this_binding, bool unwinding,
       }
       /* Come here only if make_local_foo was used for the first time
 	 on this var within this let.  */
-      FALLTHROUGH;
-    case SPECPDL_LET_DEFAULT:
       set_default_internal (specpdl_symbol (this_binding),
                             specpdl_old_value (this_binding),
                             bindflag);
       break;
+    case SPECPDL_LET_DEFAULT:
+      {
+	Lisp_Object symbol = specpdl_symbol (this_binding);
+	Lisp_Object where = specpdl_where (this_binding);
+	Lisp_Object old_value = specpdl_old_value (this_binding);
+	eassert (BUFFERP (where));
+
+	/* If this was a local binding, reset the value in the appropriate
+	   buffer, but only if that buffer's binding still exists.  */
+	if (!NILP (Flocal_variable_p (symbol, where)))
+          set_internal (symbol, old_value, where, bindflag);
+        set_default_internal (specpdl_symbol (this_binding),
+                              specpdl_old_value (this_binding),
+                              bindflag);
+      }
+      break;
     case SPECPDL_LET_LOCAL:
       {
 	Lisp_Object symbol = specpdl_symbol (this_binding);
-- 
2.28.0

Re: [PATCH 00/10] Speeding up DEFVAR_PER_BUFFER

[Stefan Monnier]

> Here is my first attempt at speeding up DEFVAR_PER_BUFFER. I would
> greatly appreciate review/comments. This is just a first attempt and
> will certainly need some cleanup.

Thanks, this looks pretty good, thanks.

My first comment is about the commit messages: they explain the
motivation very well, but they lack the itemized description in
ChangeLog style that we like to add.
For example, for the patch 01/10 instead of

    Take buffer field name in DEFVAR_PER_BUFFER

    This removes some unnecessary usage of BVAR which would need to be
    changed later anyway.
    
    Also, this makes the comment above the define of DEFVAR_PER_BUFFER
    correct - it already says that vname is the name of the buffer slot.

We'd want something like:

    src/buffer.c: Take buffer field name in DEFVAR_PER_BUFFER

    This removes some unnecessary usage of BVAR which would need to be
    changed later anyway.
    
    Also, this makes the comment above the define of DEFVAR_PER_BUFFER
    correct - it already says that vname is the name of the buffer slot.

    (DEFVAR_PER_BUFFER): `vname` is now a field name.
    (defvar_per_buffer): Replace `address` arg with `offset`.
    (syms_of_buffer): Adjust accordingly.

I'll send further comments on some specific patches.


        Stefan

Re: [PATCH 09/10] Access buffer_defaults in BVAR if there's no local binding

[Stefan Monnier]

> If the buffer_local_flags Lisp_Object is 0, though, it means this
> field in buffer_local_flags wasn't initialized,

Actually, no it means it was explicitly set (otherwise it would be Qnil).
0 means that this slot isn't exposed as a lisp variable (and those
fields don't have a "default value").

> which means this field is always buffer-local (and therefore doesn't
> use buffer_defaults).

That's right.

> @@ -825,9 +825,31 @@ set_per_buffer_value (struct buffer *b, int offset, Lisp_Object value)
>    *(Lisp_Object *)(offset + (char *) b) = value;
>  }
>  
> +INLINE bool
> +SHOULD_USE_BUFFER_DEFAULTS(struct buffer *b, ptrdiff_t offset)
                            ^^
                            SPC

Remember to use a space before opening parens (there are other
occurrences in your patches).

> +{
> +  Lisp_Object obj = *((Lisp_Object *) (offset + (char *) &buffer_local_flags));

I think you want to use `PER_BUFFER_IDX` here.

> +  if (obj.i == 0)
> +    return false;

Please don't use `obj.i`, which relies on the internal representation of `Lisp_Object`.
Also, this test is not needed: a zero value here (i.e. obj == Qnil)
would be a bug.
We already test this elsewhere, but even if we don't test it here,
`XFIXNUM` could signal an error if `obj` is not a fixnum, so this test
shouldn't be needed even for debugging purposes.

> +  int idx = XFIXNUM(obj);
> +  if (idx < 1)
> +    return false;
> +  return b->local_flags[idx] == 0;

I'd write it as

    return idx > 0 && !PER_BUFFER_VALUE_P (b, idx);

Largely because I dislike writing the constant `true` or `false`,
especially within `if` branches.

> +INLINE Lisp_Object
> +bvar_get(struct buffer *b, ptrdiff_t offset)
> +{
> +  if (SHOULD_USE_BUFFER_DEFAULTS(b, offset)) {
> +    return per_buffer_value(&buffer_defaults, offset);
> +  } else {
> +    return per_buffer_value(b, offset);
> +  }

Remember also that our coding style puts braces on their own lines like:

    if (SHOULD_USE_BUFFER_DEFAULTS (b, offset))
      {
        return per_buffer_value (&buffer_defaults, offset);
      }
    else
      {
        return per_buffer_value (b, offset);
      }

But here you can just drop the braces (and add the spaces and use
`per_buffer_default`):

    if (SHOULD_USE_BUFFER_DEFAULTS (b, offset))
      return per_buffer_default (offset);
    else
      return per_buffer_value (b, offset);

aka

    return SHOULD_USE_BUFFER_DEFAULTS (b, offset))
           ? per_buffer_value (&buffer_defaults, offset)
           : per_buffer_value (b, offset);

> -#define BVAR(buf, field) ((void)0, (buf)->field ## _)
> +#define BVAR(buf, field) bvar_get(buf, PER_BUFFER_VAR_OFFSET (field))

This will unnecessarily slow down access to those fields which have
an `idx < 1` since I can't imagine the compiler will be able to figure
out that `SHOULD_USE_BUFFER_DEFAULTS` will always return false.

So maybe those fields should be accessed via BVAR_DIRECT (which could
include an `eassert` to make sure that `SHOULD_USE_BUFFER_DEFAULTS`
indeed is false).

Your patch series will also presumably slow down BVAR access to the
other fields since they need to test `SHOULD_USE_BUFFER_DEFAULTS` now,
but I think this is the price to pay.  It should be negligible for
accesses to those vars from Elisp code, but of the 600 or so uses of
BVAR in the current code, I wonder if some of them could show
a measurable speed penalty.


        Stefan

Re: [PATCH 09/10] Access buffer_defaults in BVAR if there's no local binding

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Thu, 19 Nov 2020 13:08:16 -0500
> Cc: Arnold Noronha <arnold@tdrhq.com>, Dmitry Gutov <dgutov@yandex.ru>,
>  emacs-devel@gnu.org
> 
> Your patch series will also presumably slow down BVAR access to the
> other fields since they need to test `SHOULD_USE_BUFFER_DEFAULTS` now,
> but I think this is the price to pay.  It should be negligible for
> accesses to those vars from Elisp code, but of the 600 or so uses of
> BVAR in the current code, I wonder if some of them could show
> a measurable speed penalty.

There are 85 uses of BVAR in xdisp.c, something to look into, I guess.

Re: [PATCH 10/10] Don't iterate over all buffers in set_default_internal

[Stefan Monnier]

> More complex is the change to set_internal.
> Previously, if we were in the following scenario, which we'll refer to
> as scenario A:
> - We are in the scope of a let_default binding for some variable,
> - the current buffer is the buffer which created that let binding,
> - and we call setq for that variable.
> The variable would not become buffer-local after the setq, because of
> this check in set_internal.  But, nevertheless, the value we'd see for
> the variable would still appear different in the current buffer from
> other buffers. See the first, minor bug mentioned in bug#44733.

See my comment there.
The bug is real, but I think your patch "fixes" in an incorrect way.

> The old behavior of let_default is that if we setq in a buffer other
> than the one which created the let_default binding, our binding
> wouldn't be unwound after the let_default was done.  But if we setq in
> the buffer that created the let_default binding (that is, in scenario
> A) then that binding would be unwound.

If you can add tests for these subtleties at the same time as (or
before) these patches, that would be very helpful.


        Stefan

[PATCH v2 00/16] Speeding up DEFVAR_PER_BUFFER

[Spencer Baugh]

This patch series reworks access to Lisp_Object fields in struct
buffer. The primary motivation is performance of DEFVAR_PER_BUFFER
Lisp variables, but this also removes several now-unnecessary pieces
of metadata and generally simplifies things.

Relative to v1, this patch series now performs a larger refactoring of
struct buffer field access, which should provide for good performance
while keeping the access method uniform.  I've also corrected style
issues and bugs pointed out in review, and added some testing for
buffer-local let variables.  (Thanks Stefan for the review, and the
tests you added in the fix for bug#44733, both were quite helpful.)

It's possible to provide further performance improvements, although
the current state of the patch series has minimal overhead, and the
uniformity is somewhat appealing.

If we do need to completely remove the overhead, we could do a mass
change to all the uses of BVAR, to introduce separate, maybe,
BVAR_LISP and BVAR_C macros, for fields which need fallback and fields
which don't.  I wasn't sure whether that would be acceptable.

I have an idea which would allow avoiding the mass change, if that's
desired, which also has some additional benefits.  We could define
getter functions (marked as inline) for each Lisp_Object field, and
have BVAR call those field-specific getter functions by forming the
name of the getter through token pasting.  The boilerplate of the
getters would be kept under control by using an X macro to define both
the fields in struct buffer and the getters. Something like:

#define FIELD_LIST \
  C_FIELD(name) \
  C_FIELD(filename) \
  ...
  LISP_FIELD(mode_line_format) \
  ...

And then "instantiating" FIELD_LIST twice in two different contexts to
define the Lisp_Object fields and the getters.  This would have some
additional benefits:
- it would make it a lot easier to tell which fields are C-only and
  which fields are accessed from Lisp
- we can attach more metadata to the individual fields
- we could also generate the existing hand-written setters from these
  macros

Regardless of what approach we take, at least some level of
benchmarking will be necessary.  Is there an established benchmark
suite for these kinds of changes?

(My commit messages are still not in ChangeLog style - sorry - with 16
patches in the series right now, and having not yet personally figured
out a good magit-integrated way to generate ChangeLog-appropriate
messages, I figured I'm better off adjusting the commit messages at
the end of the review process, rather than continually throughout. If
anyone has a recommendation for ChangeLog generation that is
integrated with magit, that would be welcome - I haven't found
anything yet...)

Spencer Baugh (16):
  Add a test for let-binding unwinding
  Assert not local-variable-p after setq in let_default binding
  Stop checking the constant default for enable_multibyte_characters
  Take buffer field name in DEFVAR_PER_BUFFER
  Add BVAR_DEFAULT for access to buffer defaults
  Use bset_* functions instead of BVAR
  Take offset not idx in PER_BUFFER_VALUE_P
  Combine unnecessarily separate loops in buffer.c
  Add and use BUFFER_DEFAULT_VALUE_P
  Add and use KILL_PER_BUFFER_VALUE
  Assert that PER_BUFFER_IDX for Lisp variables is not 0
  Rearrange set_internal for buffer forwarded symbols
  Get rid of buffer_permanent_local_flags array
  Remove unnecessary Qunbound check
  Remove local_flags array in struct buffer
  Remove usage of buffer_local_flags

 lisp/bindings.el       |   3 +-
 src/buffer.c           | 417 ++++++++++++-----------------------------
 src/buffer.h           | 145 ++++++--------
 src/category.c         |   4 -
 src/category.h         |   2 +-
 src/data.c             |  82 ++------
 src/fileio.c           |  12 +-
 src/pdumper.c          |   3 -
 src/print.c            |   6 +-
 src/process.c          |  15 +-
 src/syntax.c           |   4 -
 src/syntax.h           |   2 +-
 src/window.c           |   5 +-
 test/src/data-tests.el |  24 ++-
 14 files changed, 228 insertions(+), 496 deletions(-)

-- 
2.28.0

[PATCH v2 01/16] Add a test for let-binding unwinding

[Spencer Baugh]

Bindings in other buffers are not un-set when we unwind a let-binding
which set the default value.  There doesn't seem to be an existing
test which covers this, so here's one.
---
 test/src/data-tests.el | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/test/src/data-tests.el b/test/src/data-tests.el
index 1312683c84..7e69b9c331 100644
--- a/test/src/data-tests.el
+++ b/test/src/data-tests.el
@@ -364,6 +364,28 @@ comparing the subr with a much slower lisp implementation."
           (should (equal (default-value var) (symbol-value var))))
         (should (equal (default-value var) def))))))
 
+(ert-deftest data-tests--let-buffer-local-no-unwind-other-buffers ()
+  "Test that a let-binding for a buffer-local unwinds only current-buffer"
+  (let ((blvar (make-symbol "blvar")))
+    (set-default blvar 0)
+    (make-variable-buffer-local blvar)
+    (dolist (var (list blvar 'left-margin))
+      (let* ((def (default-value var))
+             (newdef (+ def 1))
+             (otherbuf (generate-new-buffer "otherbuf")))
+        (with-temp-buffer
+          (cl-progv (list var) (list newdef)
+            (with-current-buffer otherbuf
+              (set var 123)
+              (should (local-variable-p var))
+              (should (equal (symbol-value var) 123))
+              (should (equal (default-value var) newdef))))
+          (with-current-buffer otherbuf
+            (should (local-variable-p var))
+            (should (equal (symbol-value var) 123))
+            (should (equal (default-value var) def)))
+          )))))
+
 (ert-deftest binding-test-makunbound ()
   "Tests of makunbound, from the manual."
   (with-current-buffer binding-test-buffer-B
-- 
2.28.0

[PATCH v2 02/16] Assert not local-variable-p after setq in let_default binding

[Spencer Baugh]

Breaking this is a likely way to break this test, so this saves a bit
of time in debugging.
---
 test/src/data-tests.el | 1 +
 1 file changed, 1 insertion(+)

diff --git a/test/src/data-tests.el b/test/src/data-tests.el
index 7e69b9c331..7c403870f1 100644
--- a/test/src/data-tests.el
+++ b/test/src/data-tests.el
@@ -358,6 +358,7 @@ comparing the subr with a much slower lisp implementation."
             (should (equal (symbol-value var) 42))
             (should (equal (default-value var) (symbol-value var)))
             (set var 123)
+            (should (not (local-variable-p var)))
             (should (equal (symbol-value var) 123))
             (should (equal (default-value var) (symbol-value var)))) ;bug#44733
           (should (equal (symbol-value var) def))
-- 
2.28.0

[PATCH v2 03/16] Stop checking the constant default for enable_multibyte_characters

[Spencer Baugh]

The default is a constant "t", and can't be changed. So we don't need
to check it.
---
 src/buffer.c  |  5 +----
 src/print.c   |  6 ++----
 src/process.c | 15 ++++-----------
 3 files changed, 7 insertions(+), 19 deletions(-)

diff --git a/src/buffer.c b/src/buffer.c
index 360dd348e0..33b64e1010 100644
--- a/src/buffer.c
+++ b/src/buffer.c
@@ -971,8 +971,7 @@ reset_buffer (register struct buffer *b)
   bset_last_selected_window (b, Qnil);
   bset_display_count (b, make_fixnum (0));
   bset_display_time (b, Qnil);
-  bset_enable_multibyte_characters
-    (b, BVAR (&buffer_defaults, enable_multibyte_characters));
+  bset_enable_multibyte_characters (b, Qt);
   bset_cursor_type (b, BVAR (&buffer_defaults, cursor_type));
   bset_extra_line_spacing (b, BVAR (&buffer_defaults, extra_line_spacing));
 
@@ -5397,8 +5396,6 @@ init_buffer (void)
 
   AUTO_STRING (scratch, "*scratch*");
   Fset_buffer (Fget_buffer_create (scratch));
-  if (NILP (BVAR (&buffer_defaults, enable_multibyte_characters)))
-    Fset_buffer_multibyte (Qnil);
 
   char const *pwd = emacs_wd;
 
diff --git a/src/print.c b/src/print.c
index 008bf5e639..ac8e641bea 100644
--- a/src/print.c
+++ b/src/print.c
@@ -453,8 +453,7 @@ print_string (Lisp_Object string, Lisp_Object printcharfun)
 	chars = SCHARS (string);
       else if (! print_escape_nonascii
 	       && (EQ (printcharfun, Qt)
-		   ? ! NILP (BVAR (&buffer_defaults, enable_multibyte_characters))
-		   : ! NILP (BVAR (current_buffer, enable_multibyte_characters))))
+		   || ! NILP (BVAR (current_buffer, enable_multibyte_characters))))
 	{
 	  /* If unibyte string STRING contains 8-bit codes, we must
 	     convert STRING to a multibyte string containing the same
@@ -572,8 +571,7 @@ temp_output_buffer_setup (const char *bufname)
   bset_undo_list (current_buffer, Qt);
   eassert (current_buffer->overlays_before == NULL);
   eassert (current_buffer->overlays_after == NULL);
-  bset_enable_multibyte_characters
-    (current_buffer, BVAR (&buffer_defaults, enable_multibyte_characters));
+  bset_enable_multibyte_characters (current_buffer, Qt);
   specbind (Qinhibit_read_only, Qt);
   specbind (Qinhibit_modification_hooks, Qt);
   Ferase_buffer ();
diff --git a/src/process.c b/src/process.c
index bf64ead24e..a6062e34c6 100644
--- a/src/process.c
+++ b/src/process.c
@@ -2484,8 +2484,7 @@ usage:  (make-pipe-process &rest ARGS)  */)
       }
     else if (!NILP (Vcoding_system_for_read))
       val = Vcoding_system_for_read;
-    else if ((!NILP (buffer) && NILP (BVAR (XBUFFER (buffer), enable_multibyte_characters)))
-	     || (NILP (buffer) && NILP (BVAR (&buffer_defaults, enable_multibyte_characters))))
+    else if (!NILP (buffer) && NILP (BVAR (XBUFFER (buffer), enable_multibyte_characters)))
       /* We dare not decode end-of-line format by setting VAL to
 	 Qraw_text, because the existing Emacs Lisp libraries
 	 assume that they receive bare code including a sequence of
@@ -2510,8 +2509,6 @@ usage:  (make-pipe-process &rest ARGS)  */)
       }
     else if (!NILP (Vcoding_system_for_write))
       val = Vcoding_system_for_write;
-    else if (NILP (BVAR (current_buffer, enable_multibyte_characters)))
-      val = Qnil;
     else
       {
 	if (CONSP (coding_systems))
@@ -3204,8 +3201,7 @@ usage:  (make-serial-process &rest ARGS)  */)
     }
   else if (!NILP (Vcoding_system_for_read))
     val = Vcoding_system_for_read;
-  else if ((!NILP (buffer) && NILP (BVAR (XBUFFER (buffer), enable_multibyte_characters)))
-	   || (NILP (buffer) && NILP (BVAR (&buffer_defaults, enable_multibyte_characters))))
+  else if (!NILP (buffer) && NILP (BVAR (XBUFFER (buffer), enable_multibyte_characters)))
     val = Qnil;
   pset_decode_coding_system (p, val);
 
@@ -3218,8 +3214,7 @@ usage:  (make-serial-process &rest ARGS)  */)
     }
   else if (!NILP (Vcoding_system_for_write))
     val = Vcoding_system_for_write;
-  else if ((!NILP (buffer) && NILP (BVAR (XBUFFER (buffer), enable_multibyte_characters)))
-	   || (NILP (buffer) && NILP (BVAR (&buffer_defaults, enable_multibyte_characters))))
+  else if (!NILP (buffer) && NILP (BVAR (XBUFFER (buffer), enable_multibyte_characters)))
     val = Qnil;
   pset_encode_coding_system (p, val);
 
@@ -3261,9 +3256,7 @@ set_network_socket_coding_system (Lisp_Object proc, Lisp_Object host,
   else if (!NILP (Vcoding_system_for_read))
     val = Vcoding_system_for_read;
   else if ((!NILP (p->buffer)
-	    && NILP (BVAR (XBUFFER (p->buffer), enable_multibyte_characters)))
-	   || (NILP (p->buffer)
-	       && NILP (BVAR (&buffer_defaults, enable_multibyte_characters))))
+	    && NILP (BVAR (XBUFFER (p->buffer), enable_multibyte_characters))))
     /* We dare not decode end-of-line format by setting VAL to
        Qraw_text, because the existing Emacs Lisp libraries
        assume that they receive bare code including a sequence of
-- 
2.28.0

[PATCH v2 04/16] Take buffer field name in DEFVAR_PER_BUFFER

[Spencer Baugh]

This removes some unnecessary usage of BVAR which would need to be
changed later anyway.

Also, this makes the comment above the define of DEFVAR_PER_BUFFER
correct - it already says that vname is the name of the buffer slot.
---
 src/buffer.c | 124 +++++++++++++++++++++++++--------------------------
 1 file changed, 61 insertions(+), 63 deletions(-)

diff --git a/src/buffer.c b/src/buffer.c
index 33b64e1010..defc4ac746 100644
--- a/src/buffer.c
+++ b/src/buffer.c
@@ -5452,18 +5452,16 @@ init_buffer (void)
 #define DEFVAR_PER_BUFFER(lname, vname, predicate, doc)		\
   do {								\
     static struct Lisp_Buffer_Objfwd bo_fwd;			\
-    defvar_per_buffer (&bo_fwd, lname, vname, predicate);	\
+    defvar_per_buffer (&bo_fwd, lname, PER_BUFFER_VAR_OFFSET (vname), predicate); \
   } while (0)
 
 static void
 defvar_per_buffer (struct Lisp_Buffer_Objfwd *bo_fwd, const char *namestring,
-		   Lisp_Object *address, Lisp_Object predicate)
+		   int offset, Lisp_Object predicate)
 {
   struct Lisp_Symbol *sym;
-  int offset;
 
   sym = XSYMBOL (intern (namestring));
-  offset = (char *)address - (char *)current_buffer;
 
   bo_fwd->type = Lisp_Fwd_Buffer_Obj;
   bo_fwd->offset = offset;
@@ -5528,20 +5526,20 @@ syms_of_buffer (void)
 	build_pure_c_string ("Attempt to modify a protected field"));
 
   DEFVAR_PER_BUFFER ("tab-line-format",
-		     &BVAR (current_buffer, tab_line_format),
+		     tab_line_format,
 		     Qnil,
 		     doc: /* Analogous to `mode-line-format', but controls the tab line.
 The tab line appears, optionally, at the top of a window;
 the mode line appears at the bottom.  */);
 
   DEFVAR_PER_BUFFER ("header-line-format",
-		     &BVAR (current_buffer, header_line_format),
+		     header_line_format,
 		     Qnil,
 		     doc: /* Analogous to `mode-line-format', but controls the header line.
 The header line appears, optionally, at the top of a window;
 the mode line appears at the bottom.  */);
 
-  DEFVAR_PER_BUFFER ("mode-line-format", &BVAR (current_buffer, mode_line_format),
+  DEFVAR_PER_BUFFER ("mode-line-format", mode_line_format,
 		     Qnil,
 		     doc: /* Template for displaying mode line for current buffer.
 
@@ -5607,32 +5605,32 @@ A string is printed verbatim in the mode line except for %-constructs:
   %% -- print %.   %- -- print infinitely many dashes.
 Decimal digits after the % specify field width to which to pad.  */);
 
-  DEFVAR_PER_BUFFER ("major-mode", &BVAR (current_buffer, major_mode),
+  DEFVAR_PER_BUFFER ("major-mode", major_mode,
 		     Qsymbolp,
 		     doc: /* Symbol for current buffer's major mode.
 The default value (normally `fundamental-mode') affects new buffers.
 A value of nil means to use the current buffer's major mode, provided
 it is not marked as "special".  */);
 
-  DEFVAR_PER_BUFFER ("mode-name", &BVAR (current_buffer, mode_name),
+  DEFVAR_PER_BUFFER ("mode-name", mode_name,
                      Qnil,
 		     doc: /* Pretty name of current buffer's major mode.
 Usually a string, but can use any of the constructs for `mode-line-format',
 which see.
 Format with `format-mode-line' to produce a string value.  */);
 
-  DEFVAR_PER_BUFFER ("local-abbrev-table", &BVAR (current_buffer, abbrev_table), Qnil,
+  DEFVAR_PER_BUFFER ("local-abbrev-table", abbrev_table, Qnil,
 		     doc: /* Local (mode-specific) abbrev table of current buffer.  */);
 
-  DEFVAR_PER_BUFFER ("abbrev-mode", &BVAR (current_buffer, abbrev_mode), Qnil,
+  DEFVAR_PER_BUFFER ("abbrev-mode", abbrev_mode, Qnil,
 		     doc: /*  Non-nil if Abbrev mode is enabled.
 Use the command `abbrev-mode' to change this variable.  */);
 
-  DEFVAR_PER_BUFFER ("case-fold-search", &BVAR (current_buffer, case_fold_search),
+  DEFVAR_PER_BUFFER ("case-fold-search", case_fold_search,
 		     Qnil,
 		     doc: /* Non-nil if searches and matches should ignore case.  */);
 
-  DEFVAR_PER_BUFFER ("fill-column", &BVAR (current_buffer, fill_column),
+  DEFVAR_PER_BUFFER ("fill-column", fill_column,
 		     Qintegerp,
 		     doc: /* Column beyond which automatic line-wrapping should happen.
 It is used by filling commands, such as `fill-region' and `fill-paragraph',
@@ -5640,26 +5638,26 @@ and by `auto-fill-mode', which see.
 See also `current-fill-column'.
 Interactively, you can set the buffer local value using \\[set-fill-column].  */);
 
-  DEFVAR_PER_BUFFER ("left-margin", &BVAR (current_buffer, left_margin),
+  DEFVAR_PER_BUFFER ("left-margin", left_margin,
 		     Qintegerp,
 		     doc: /* Column for the default `indent-line-function' to indent to.
 Linefeed indents to this column in Fundamental mode.  */);
 
-  DEFVAR_PER_BUFFER ("tab-width", &BVAR (current_buffer, tab_width),
+  DEFVAR_PER_BUFFER ("tab-width", tab_width,
 		     Qintegerp,
 		     doc: /* Distance between tab stops (for display of tab characters), in columns.
 NOTE: This controls the display width of a TAB character, and not
 the size of an indentation step.
 This should be an integer greater than zero.  */);
 
-  DEFVAR_PER_BUFFER ("ctl-arrow", &BVAR (current_buffer, ctl_arrow), Qnil,
+  DEFVAR_PER_BUFFER ("ctl-arrow", ctl_arrow, Qnil,
 		     doc: /* Non-nil means display control chars with uparrow.
 A value of nil means use backslash and octal digits.
 This variable does not apply to characters whose display is specified
 in the current display table (if there is one).  */);
 
   DEFVAR_PER_BUFFER ("enable-multibyte-characters",
-		     &BVAR (current_buffer, enable_multibyte_characters),
+		     enable_multibyte_characters,
 		     Qnil,
 		     doc: /* Non-nil means the buffer contents are regarded as multi-byte characters.
 Otherwise they are regarded as unibyte.  This affects the display,
@@ -5673,7 +5671,7 @@ See also Info node `(elisp)Text Representations'.  */);
   make_symbol_constant (intern_c_string ("enable-multibyte-characters"));
 
   DEFVAR_PER_BUFFER ("buffer-file-coding-system",
-		     &BVAR (current_buffer, buffer_file_coding_system), Qnil,
+		     buffer_file_coding_system, Qnil,
 		     doc: /* Coding system to be used for encoding the buffer contents on saving.
 This variable applies to saving the buffer, and also to `write-region'
 and other functions that use `write-region'.
@@ -5691,7 +5689,7 @@ The variable `coding-system-for-write', if non-nil, overrides this variable.
 This variable is never applied to a way of decoding a file while reading it.  */);
 
   DEFVAR_PER_BUFFER ("bidi-display-reordering",
-		     &BVAR (current_buffer, bidi_display_reordering), Qnil,
+		     bidi_display_reordering, Qnil,
 		     doc: /* Non-nil means reorder bidirectional text for display in the visual order.
 Setting this to nil is intended for use in debugging the display code.
 Don't set to nil in normal sessions, as that is not supported.
@@ -5699,7 +5697,7 @@ See also `bidi-paragraph-direction'; setting that non-nil might
 speed up redisplay.  */);
 
   DEFVAR_PER_BUFFER ("bidi-paragraph-start-re",
-		     &BVAR (current_buffer, bidi_paragraph_start_re), Qnil,
+		     bidi_paragraph_start_re, Qnil,
 		     doc: /* If non-nil, a regexp matching a line that starts OR separates paragraphs.
 
 The value of nil means to use empty lines as lines that start and
@@ -5721,7 +5719,7 @@ set both these variables to "^".
 See also `bidi-paragraph-direction'.  */);
 
   DEFVAR_PER_BUFFER ("bidi-paragraph-separate-re",
-		     &BVAR (current_buffer, bidi_paragraph_separate_re), Qnil,
+		     bidi_paragraph_separate_re, Qnil,
 		     doc: /* If non-nil, a regexp matching a line that separates paragraphs.
 
 The value of nil means to use empty lines as paragraph separators.
@@ -5742,7 +5740,7 @@ set both these variables to "^".
 See also `bidi-paragraph-direction'.  */);
 
   DEFVAR_PER_BUFFER ("bidi-paragraph-direction",
-		     &BVAR (current_buffer, bidi_paragraph_direction), Qnil,
+		     bidi_paragraph_direction, Qnil,
 		     doc: /* If non-nil, forces directionality of text paragraphs in the buffer.
 
 If this is nil (the default), the direction of each paragraph is
@@ -5753,7 +5751,7 @@ Any other value is treated as nil.
 This variable has no effect unless the buffer's value of
 `bidi-display-reordering' is non-nil.  */);
 
- DEFVAR_PER_BUFFER ("truncate-lines", &BVAR (current_buffer, truncate_lines), Qnil,
+ DEFVAR_PER_BUFFER ("truncate-lines", truncate_lines, Qnil,
 		     doc: /* Non-nil means do not display continuation lines.
 Instead, give each line of text just one screen line.
 
@@ -5763,7 +5761,7 @@ and this buffer is not full-frame width.
 
 Minibuffers set this variable to nil.  */);
 
-  DEFVAR_PER_BUFFER ("word-wrap", &BVAR (current_buffer, word_wrap), Qnil,
+  DEFVAR_PER_BUFFER ("word-wrap", word_wrap, Qnil,
 		     doc: /* Non-nil means to use word-wrapping for continuation lines.
 When word-wrapping is on, continuation lines are wrapped at the space
 or tab character nearest to the right window edge.
@@ -5781,14 +5779,14 @@ to t, and additionally redefines simple editing commands to act on
 visual lines rather than logical lines.  See the documentation of
 `visual-line-mode'.  */);
 
-  DEFVAR_PER_BUFFER ("default-directory", &BVAR (current_buffer, directory),
+  DEFVAR_PER_BUFFER ("default-directory", directory,
 		     Qstringp,
 		     doc: /* Name of default directory of current buffer.
 It should be an absolute directory name; on GNU and Unix systems,
 these names start with `/' or `~' and end with `/'.
 To interactively change the default directory, use command `cd'. */);
 
-  DEFVAR_PER_BUFFER ("auto-fill-function", &BVAR (current_buffer, auto_fill_function),
+  DEFVAR_PER_BUFFER ("auto-fill-function", auto_fill_function,
 		     Qnil,
 		     doc: /* Function called (if non-nil) to perform auto-fill.
 It is called after self-inserting any character specified in
@@ -5796,31 +5794,31 @@ the `auto-fill-chars' table.
 NOTE: This variable is not a hook;
 its value may not be a list of functions.  */);
 
-  DEFVAR_PER_BUFFER ("buffer-file-name", &BVAR (current_buffer, filename),
+  DEFVAR_PER_BUFFER ("buffer-file-name", filename,
 		     Qstringp,
 		     doc: /* Name of file visited in current buffer, or nil if not visiting a file.
 This should be an absolute file name.  */);
 
-  DEFVAR_PER_BUFFER ("buffer-file-truename", &BVAR (current_buffer, file_truename),
+  DEFVAR_PER_BUFFER ("buffer-file-truename", file_truename,
 		     Qstringp,
 		     doc: /* Abbreviated truename of file visited in current buffer, or nil if none.
 The truename of a file is calculated by `file-truename'
 and then abbreviated with `abbreviate-file-name'.  */);
 
   DEFVAR_PER_BUFFER ("buffer-auto-save-file-name",
-		     &BVAR (current_buffer, auto_save_file_name),
+		     auto_save_file_name,
 		     Qstringp,
 		     doc: /* Name of file for auto-saving current buffer.
 If it is nil, that means don't auto-save this buffer.  */);
 
-  DEFVAR_PER_BUFFER ("buffer-read-only", &BVAR (current_buffer, read_only), Qnil,
+  DEFVAR_PER_BUFFER ("buffer-read-only", read_only, Qnil,
 		     doc: /* Non-nil if this buffer is read-only.  */);
 
-  DEFVAR_PER_BUFFER ("buffer-backed-up", &BVAR (current_buffer, backed_up), Qnil,
+  DEFVAR_PER_BUFFER ("buffer-backed-up", backed_up, Qnil,
 		     doc: /* Non-nil if this buffer's file has been backed up.
 Backing up is done before the first time the file is saved.  */);
 
-  DEFVAR_PER_BUFFER ("buffer-saved-size", &BVAR (current_buffer, save_length),
+  DEFVAR_PER_BUFFER ("buffer-saved-size", save_length,
 		     Qintegerp,
 		     doc: /* Length of current buffer when last read in, saved or auto-saved.
 0 initially.
@@ -5830,7 +5828,7 @@ If you set this to -2, that means don't turn off auto-saving in this buffer
 if its text size shrinks.   If you use `buffer-swap-text' on a buffer,
 you probably should set this to -2 in that buffer.  */);
 
-  DEFVAR_PER_BUFFER ("selective-display", &BVAR (current_buffer, selective_display),
+  DEFVAR_PER_BUFFER ("selective-display", selective_display,
 		     Qnil,
 		     doc: /* Non-nil enables selective display.
 
@@ -5843,11 +5841,11 @@ in a file, save the ^M as a newline.  This usage is obsolete; use
 overlays or text properties instead.  */);
 
   DEFVAR_PER_BUFFER ("selective-display-ellipses",
-		     &BVAR (current_buffer, selective_display_ellipses),
+		     selective_display_ellipses,
 		     Qnil,
 		     doc: /* Non-nil means display ... on previous line when a line is invisible.  */);
 
-  DEFVAR_PER_BUFFER ("overwrite-mode", &BVAR (current_buffer, overwrite_mode),
+  DEFVAR_PER_BUFFER ("overwrite-mode", overwrite_mode,
 		     Qoverwrite_mode,
 		     doc: /* Non-nil if self-insertion should replace existing text.
 The value should be one of `overwrite-mode-textual',
@@ -5857,7 +5855,7 @@ inserts at the end of a line, and inserts when point is before a tab,
 until the tab is filled in.
 If `overwrite-mode-binary', self-insertion replaces newlines and tabs too.  */);
 
-  DEFVAR_PER_BUFFER ("buffer-display-table", &BVAR (current_buffer, display_table),
+  DEFVAR_PER_BUFFER ("buffer-display-table", display_table,
 		     Qnil,
 		     doc: /* Display table that controls display of the contents of current buffer.
 
@@ -5894,7 +5892,7 @@ In addition, a char-table has six extra slots to control the display of:
 
 See also the functions `display-table-slot' and `set-display-table-slot'.  */);
 
-  DEFVAR_PER_BUFFER ("left-margin-width", &BVAR (current_buffer, left_margin_cols),
+  DEFVAR_PER_BUFFER ("left-margin-width", left_margin_cols,
 		     Qintegerp,
 		     doc: /* Width in columns of left marginal area for display of a buffer.
 A value of nil means no marginal area.
@@ -5902,7 +5900,7 @@ A value of nil means no marginal area.
 Setting this variable does not take effect until a new buffer is displayed
 in a window.  To make the change take effect, call `set-window-buffer'.  */);
 
-  DEFVAR_PER_BUFFER ("right-margin-width", &BVAR (current_buffer, right_margin_cols),
+  DEFVAR_PER_BUFFER ("right-margin-width", right_margin_cols,
 		     Qintegerp,
 		     doc: /* Width in columns of right marginal area for display of a buffer.
 A value of nil means no marginal area.
@@ -5910,7 +5908,7 @@ A value of nil means no marginal area.
 Setting this variable does not take effect until a new buffer is displayed
 in a window.  To make the change take effect, call `set-window-buffer'.  */);
 
-  DEFVAR_PER_BUFFER ("left-fringe-width", &BVAR (current_buffer, left_fringe_width),
+  DEFVAR_PER_BUFFER ("left-fringe-width", left_fringe_width,
 		     Qintegerp,
 		     doc: /* Width of this buffer's left fringe (in pixels).
 A value of 0 means no left fringe is shown in this buffer's window.
@@ -5919,7 +5917,7 @@ A value of nil means to use the left fringe width from the window's frame.
 Setting this variable does not take effect until a new buffer is displayed
 in a window.  To make the change take effect, call `set-window-buffer'.  */);
 
-  DEFVAR_PER_BUFFER ("right-fringe-width", &BVAR (current_buffer, right_fringe_width),
+  DEFVAR_PER_BUFFER ("right-fringe-width", right_fringe_width,
 		     Qintegerp,
 		     doc: /* Width of this buffer's right fringe (in pixels).
 A value of 0 means no right fringe is shown in this buffer's window.
@@ -5928,7 +5926,7 @@ A value of nil means to use the right fringe width from the window's frame.
 Setting this variable does not take effect until a new buffer is displayed
 in a window.  To make the change take effect, call `set-window-buffer'.  */);
 
-  DEFVAR_PER_BUFFER ("fringes-outside-margins", &BVAR (current_buffer, fringes_outside_margins),
+  DEFVAR_PER_BUFFER ("fringes-outside-margins", fringes_outside_margins,
 		     Qnil,
 		     doc: /* Non-nil means to display fringes outside display margins.
 A value of nil means to display fringes between margins and buffer text.
@@ -5936,17 +5934,17 @@ A value of nil means to display fringes between margins and buffer text.
 Setting this variable does not take effect until a new buffer is displayed
 in a window.  To make the change take effect, call `set-window-buffer'.  */);
 
-  DEFVAR_PER_BUFFER ("scroll-bar-width", &BVAR (current_buffer, scroll_bar_width),
+  DEFVAR_PER_BUFFER ("scroll-bar-width", scroll_bar_width,
 		     Qintegerp,
 		     doc: /* Width of this buffer's vertical scroll bars in pixels.
 A value of nil means to use the scroll bar width from the window's frame.  */);
 
-  DEFVAR_PER_BUFFER ("scroll-bar-height", &BVAR (current_buffer, scroll_bar_height),
+  DEFVAR_PER_BUFFER ("scroll-bar-height", scroll_bar_height,
 		     Qintegerp,
 		     doc: /* Height of this buffer's horizontal scroll bars in pixels.
 A value of nil means to use the scroll bar height from the window's frame.  */);
 
-  DEFVAR_PER_BUFFER ("vertical-scroll-bar", &BVAR (current_buffer, vertical_scroll_bar_type),
+  DEFVAR_PER_BUFFER ("vertical-scroll-bar", vertical_scroll_bar_type,
 		     Qvertical_scroll_bar,
 		     doc: /* Position of this buffer's vertical scroll bar.
 The value takes effect whenever you tell a window to display this buffer;
@@ -5956,7 +5954,7 @@ A value of `left' or `right' means put the vertical scroll bar at that side
 of the window; a value of nil means don't show any vertical scroll bars.
 A value of t (the default) means do whatever the window's frame specifies.  */);
 
-  DEFVAR_PER_BUFFER ("horizontal-scroll-bar", &BVAR (current_buffer, horizontal_scroll_bar_type),
+  DEFVAR_PER_BUFFER ("horizontal-scroll-bar", horizontal_scroll_bar_type,
 		     Qnil,
 		     doc: /* Position of this buffer's horizontal scroll bar.
 The value takes effect whenever you tell a window to display this buffer;
@@ -5968,13 +5966,13 @@ A value of t (the default) means do whatever the window's frame
 specifies.  */);
 
   DEFVAR_PER_BUFFER ("indicate-empty-lines",
-		     &BVAR (current_buffer, indicate_empty_lines), Qnil,
+		     indicate_empty_lines, Qnil,
 		     doc: /* Visually indicate empty lines after the buffer end.
 If non-nil, a bitmap is displayed in the left fringe of a window on
 window-systems.  */);
 
   DEFVAR_PER_BUFFER ("indicate-buffer-boundaries",
-		     &BVAR (current_buffer, indicate_buffer_boundaries), Qnil,
+		     indicate_buffer_boundaries, Qnil,
 		     doc: /* Visually indicate buffer boundaries and scrolling.
 If non-nil, the first and last line of the buffer are marked in the fringe
 of a window on window-systems with angle bitmaps, or if the window can be
@@ -5999,7 +5997,7 @@ bitmaps in right fringe.  To show just the angle bitmaps in the left
 fringe, but no arrow bitmaps, use ((top .  left) (bottom . left)).  */);
 
   DEFVAR_PER_BUFFER ("fringe-indicator-alist",
-		     &BVAR (current_buffer, fringe_indicator_alist), Qnil,
+		     fringe_indicator_alist, Qnil,
 		     doc: /* Mapping from logical to physical fringe indicator bitmaps.
 The value is an alist where each element (INDICATOR . BITMAPS)
 specifies the fringe bitmaps used to display a specific logical
@@ -6018,7 +6016,7 @@ last (only) line has no final newline.  BITMAPS may also be a single
 symbol which is used in both left and right fringes.  */);
 
   DEFVAR_PER_BUFFER ("fringe-cursor-alist",
-		     &BVAR (current_buffer, fringe_cursor_alist), Qnil,
+		     fringe_cursor_alist, Qnil,
 		     doc: /* Mapping from logical to physical fringe cursor bitmaps.
 The value is an alist where each element (CURSOR . BITMAP)
 specifies the fringe bitmaps used to display a specific logical
@@ -6033,7 +6031,7 @@ BITMAP is the corresponding fringe bitmap shown for the logical
 cursor type.  */);
 
   DEFVAR_PER_BUFFER ("scroll-up-aggressively",
-		     &BVAR (current_buffer, scroll_up_aggressively), Qfraction,
+		     scroll_up_aggressively, Qfraction,
 		     doc: /* How far to scroll windows upward.
 If you move point off the bottom, the window scrolls automatically.
 This variable controls how far it scrolls.  The value nil, the default,
@@ -6046,7 +6044,7 @@ window scrolls by a full window height.  Meaningful values are
 between 0.0 and 1.0, inclusive.  */);
 
   DEFVAR_PER_BUFFER ("scroll-down-aggressively",
-		     &BVAR (current_buffer, scroll_down_aggressively), Qfraction,
+		     scroll_down_aggressively, Qfraction,
 		     doc: /* How far to scroll windows downward.
 If you move point off the top, the window scrolls automatically.
 This variable controls how far it scrolls.  The value nil, the default,
@@ -6097,7 +6095,7 @@ from happening repeatedly and making Emacs nonfunctional.  */);
 The functions are run using the `run-hooks' function.  */);
   Vfirst_change_hook = Qnil;
 
-  DEFVAR_PER_BUFFER ("buffer-undo-list", &BVAR (current_buffer, undo_list), Qnil,
+  DEFVAR_PER_BUFFER ("buffer-undo-list", undo_list, Qnil,
 		     doc: /* List of undo entries in current buffer.
 Recent changes come first; older changes follow newer.
 
@@ -6143,10 +6141,10 @@ the changes between two undo boundaries as a single step to be undone.
 
 If the value of the variable is t, undo information is not recorded.  */);
 
-  DEFVAR_PER_BUFFER ("mark-active", &BVAR (current_buffer, mark_active), Qnil,
+  DEFVAR_PER_BUFFER ("mark-active", mark_active, Qnil,
 		     doc: /* Non-nil means the mark and region are currently active in this buffer.  */);
 
-  DEFVAR_PER_BUFFER ("cache-long-scans", &BVAR (current_buffer, cache_long_scans), Qnil,
+  DEFVAR_PER_BUFFER ("cache-long-scans", cache_long_scans, Qnil,
 		     doc: /* Non-nil means that Emacs should use caches in attempt to speedup buffer scans.
 
 There is no reason to set this to nil except for debugging purposes.
@@ -6182,23 +6180,23 @@ maintained internally by the Emacs primitives.  Enabling or disabling
 the cache should not affect the behavior of any of the motion
 functions; it should only affect their performance.  */);
 
-  DEFVAR_PER_BUFFER ("point-before-scroll", &BVAR (current_buffer, point_before_scroll), Qnil,
+  DEFVAR_PER_BUFFER ("point-before-scroll", point_before_scroll, Qnil,
 		     doc: /* Value of point before the last series of scroll operations, or nil.  */);
 
-  DEFVAR_PER_BUFFER ("buffer-file-format", &BVAR (current_buffer, file_format), Qnil,
+  DEFVAR_PER_BUFFER ("buffer-file-format", file_format, Qnil,
 		     doc: /* List of formats to use when saving this buffer.
 Formats are defined by `format-alist'.  This variable is
 set when a file is visited.  */);
 
   DEFVAR_PER_BUFFER ("buffer-auto-save-file-format",
-		     &BVAR (current_buffer, auto_save_file_format), Qnil,
+		     auto_save_file_format, Qnil,
 		     doc: /* Format in which to write auto-save files.
 Should be a list of symbols naming formats that are defined in `format-alist'.
 If it is t, which is the default, auto-save files are written in the
 same format as a regular save would use.  */);
 
   DEFVAR_PER_BUFFER ("buffer-invisibility-spec",
-		     &BVAR (current_buffer, invisibility_spec), Qnil,
+		     invisibility_spec, Qnil,
 		     doc: /* Invisibility spec of this buffer.
 The default is t, which means that text is invisible if it has a non-nil
 `invisible' property.
@@ -6212,12 +6210,12 @@ Setting this variable is very fast, much faster than scanning all the text in
 the buffer looking for properties to change.  */);
 
   DEFVAR_PER_BUFFER ("buffer-display-count",
-		     &BVAR (current_buffer, display_count), Qintegerp,
+		     display_count, Qintegerp,
 		     doc: /* A number incremented each time this buffer is displayed in a window.
 The function `set-window-buffer' increments it.  */);
 
   DEFVAR_PER_BUFFER ("buffer-display-time",
-		     &BVAR (current_buffer, display_time), Qnil,
+		     display_time, Qnil,
 		     doc: /* Time stamp updated each time this buffer is displayed in a window.
 The function `set-window-buffer' updates this variable
 to the value obtained by calling `current-time'.
@@ -6253,7 +6251,7 @@ member of the list.  Any other non-nil value means disregard `buffer-read-only'
 and all `read-only' text properties.  */);
   Vinhibit_read_only = Qnil;
 
-  DEFVAR_PER_BUFFER ("cursor-type", &BVAR (current_buffer, cursor_type), Qnil,
+  DEFVAR_PER_BUFFER ("cursor-type", cursor_type, Qnil,
 		     doc: /* Cursor to use when this buffer is in the selected window.
 Values are interpreted as follows:
 
@@ -6277,7 +6275,7 @@ cursor's appearance is instead controlled by the variable
 `cursor-in-non-selected-windows'.  */);
 
   DEFVAR_PER_BUFFER ("line-spacing",
-		     &BVAR (current_buffer, extra_line_spacing), Qnumberp,
+		     extra_line_spacing, Qnumberp,
 		     doc: /* Additional space to put between lines when displaying a buffer.
 The space is measured in pixels, and put below lines on graphic displays,
 see `display-graphic-p'.
@@ -6285,7 +6283,7 @@ If value is a floating point number, it specifies the spacing relative
 to the default frame line height.  A value of nil means add no extra space.  */);
 
   DEFVAR_PER_BUFFER ("cursor-in-non-selected-windows",
-		     &BVAR (current_buffer, cursor_in_non_selected_windows), Qnil,
+		     cursor_in_non_selected_windows, Qnil,
 		     doc: /* Non-nil means show a cursor in non-selected windows.
 If nil, only shows a cursor in the selected window.
 If t, displays a cursor related to the usual cursor type
-- 
2.28.0

[PATCH v2 05/16] Add BVAR_DEFAULT for access to buffer defaults

[Spencer Baugh]

This is more direct. This macro can stay as an lvalue even when we
change BVAR to be a function call - directly changing and accessing
buffer_defaults is always fine.
---
 src/buffer.h   | 3 +++
 src/category.h | 2 +-
 src/syntax.h   | 2 +-
 3 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/src/buffer.h b/src/buffer.h
index fe549c5dac..2999036300 100644
--- a/src/buffer.h
+++ b/src/buffer.h
@@ -284,6 +284,9 @@ struct buffer_text
 
 #define BVAR(buf, field) ((buf)->field ## _)
 
+/* Access a BVAR from buffer_defaults */
+#define BVAR_DEFAULT(field) (buffer_defaults.field ## _)
+
 /* Max number of builtin per-buffer variables.  */
 enum { MAX_PER_BUFFER_VARS = 50 };
 
diff --git a/src/category.h b/src/category.h
index cc32990478..85e872a940 100644
--- a/src/category.h
+++ b/src/category.h
@@ -94,7 +94,7 @@ CHAR_HAS_CATEGORY (int ch, int category)
 
 /* The standard category table is stored where it will automatically
    be used in all new buffers.  */
-#define Vstandard_category_table BVAR (&buffer_defaults, category_table)
+#define Vstandard_category_table BVAR_DEFAULT (category_table)
 
 /* Return the doc string of CATEGORY in category table TABLE.  */
 #define CATEGORY_DOCSTRING(table, category)				\
diff --git a/src/syntax.h b/src/syntax.h
index a2ec3301ba..9c2bf32057 100644
--- a/src/syntax.h
+++ b/src/syntax.h
@@ -31,7 +31,7 @@ extern void update_syntax_table_forward (ptrdiff_t, bool, Lisp_Object);
 
 /* The standard syntax table is stored where it will automatically
    be used in all new buffers.  */
-#define Vstandard_syntax_table BVAR (&buffer_defaults, syntax_table)
+#define Vstandard_syntax_table BVAR_DEFAULT (syntax_table)
 
 /* A syntax table is a chartable whose elements are cons cells
    (CODE+FLAGS . MATCHING-CHAR).  MATCHING-CHAR can be nil if the char
-- 
2.28.0

[PATCH v2 06/16] Use bset_* functions instead of BVAR

[Spencer Baugh]

We move bset_save_length to buffer.h, and expand usage of the
pre-existing bset_last_selected_window and
bset_enable_multibyte_characters functions.

This removes some usage of BVAR as an lvalue, necessary for changing
BVAR into a function call.
---
 src/buffer.c | 13 ++++---------
 src/buffer.h |  5 +++++
 src/fileio.c | 12 ++++++------
 src/window.c |  5 ++---
 4 files changed, 17 insertions(+), 18 deletions(-)

diff --git a/src/buffer.c b/src/buffer.c
index defc4ac746..aa951ca78f 100644
--- a/src/buffer.c
+++ b/src/buffer.c
@@ -327,11 +327,6 @@ bset_right_fringe_width (struct buffer *b, Lisp_Object val)
   b->right_fringe_width_ = val;
 }
 static void
-bset_save_length (struct buffer *b, Lisp_Object val)
-{
-  b->save_length_ = val;
-}
-static void
 bset_scroll_bar_width (struct buffer *b, Lisp_Object val)
 {
   b->scroll_bar_width_ = val;
@@ -951,7 +946,7 @@ reset_buffer (register struct buffer *b)
   bset_directory (b, current_buffer ? BVAR (current_buffer, directory) : Qnil);
   b->modtime = make_timespec (0, UNKNOWN_MODTIME_NSECS);
   b->modtime_size = -1;
-  XSETFASTINT (BVAR (b, save_length), 0);
+  bset_save_length (b, make_fixed_natnum (0));
   b->last_window_start = 1;
   /* It is more conservative to start out "changed" than "unchanged".  */
   b->clip_changed = 0;
@@ -2275,7 +2270,7 @@ so the buffer is truly empty after this.  */)
   /* Prevent warnings, or suspension of auto saving, that would happen
      if future size is less than past size.  Use of erase-buffer
      implies that the future text is not really related to the past text.  */
-  XSETFASTINT (BVAR (current_buffer, save_length), 0);
+  bset_save_length (current_buffer, make_fixed_natnum (0));
   return Qnil;
 }
 
@@ -2763,8 +2758,8 @@ current buffer is cleared.  */)
       struct buffer *o = XBUFFER (other);
       if (o->base_buffer == current_buffer && BUFFER_LIVE_P (o))
 	{
-	  BVAR (o, enable_multibyte_characters)
-	    = BVAR (current_buffer, enable_multibyte_characters);
+	  bset_enable_multibyte_characters (o,
+            BVAR (current_buffer, enable_multibyte_characters));
 	  o->prevent_redisplay_optimizations_p = true;
 	}
     }
diff --git a/src/buffer.h b/src/buffer.h
index 2999036300..770a5d03e6 100644
--- a/src/buffer.h
+++ b/src/buffer.h
@@ -824,6 +824,11 @@ bset_width_table (struct buffer *b, Lisp_Object val)
 {
   b->width_table_ = val;
 }
+INLINE void
+bset_save_length (struct buffer *b, Lisp_Object val)
+{
+  b->save_length_ = val;
+}
 
 /* BUFFER_CEILING_OF (resp. BUFFER_FLOOR_OF), when applied to n, return
    the max (resp. min) p such that
diff --git a/src/fileio.c b/src/fileio.c
index 283813ff89..ba094a5705 100644
--- a/src/fileio.c
+++ b/src/fileio.c
@@ -4697,7 +4697,7 @@ by calling `format-decode', which see.  */)
 
       SAVE_MODIFF = MODIFF;
       BUF_AUTOSAVE_MODIFF (current_buffer) = MODIFF;
-      XSETFASTINT (BVAR (current_buffer, save_length), Z - BEG);
+      bset_save_length (current_buffer, make_fixed_natnum (Z - BEG));
       if (NILP (handler))
 	{
 	  if (!NILP (BVAR (current_buffer, file_truename)))
@@ -5110,7 +5110,7 @@ write_region (Lisp_Object start, Lisp_Object end, Lisp_Object filename,
       if (visiting)
 	{
 	  SAVE_MODIFF = MODIFF;
-	  XSETFASTINT (BVAR (current_buffer, save_length), Z - BEG);
+          bset_save_length (current_buffer, make_fixed_natnum (Z - BEG));
 	  bset_filename (current_buffer, visit_file);
 	}
 
@@ -5361,7 +5361,7 @@ write_region (Lisp_Object start, Lisp_Object end, Lisp_Object filename,
   if (visiting)
     {
       SAVE_MODIFF = MODIFF;
-      XSETFASTINT (BVAR (current_buffer, save_length), Z - BEG);
+      bset_save_length (current_buffer, make_fixed_natnum (Z - BEG));
       bset_filename (current_buffer, visit_file);
       update_mode_lines = 14;
       if (auto_saving_into_visited_file)
@@ -5979,7 +5979,7 @@ A non-nil CURRENT-ONLY argument means save only current buffer.  */)
 		minibuffer_auto_raise = 0;
 		/* Turn off auto-saving until there's a real save,
 		   and prevent any more warnings.  */
-		XSETINT (BVAR (b, save_length), -1);
+                bset_save_length (current_buffer, make_fixed_natnum (-1));
 		Fsleep_for (make_fixnum (1), Qnil);
 		continue;
 	      }
@@ -5988,7 +5988,7 @@ A non-nil CURRENT-ONLY argument means save only current buffer.  */)
 	    internal_condition_case (auto_save_1, Qt, auto_save_error);
 	    auto_saved = 1;
 	    BUF_AUTOSAVE_MODIFF (b) = BUF_MODIFF (b);
-	    XSETFASTINT (BVAR (current_buffer, save_length), Z - BEG);
+            bset_save_length (current_buffer, make_fixed_natnum (Z - BEG));
 	    set_buffer_internal (old);
 
 	    after_time = current_timespec ();
@@ -6034,7 +6034,7 @@ No auto-save file will be written until the buffer changes again.  */)
   /* FIXME: This should not be called in indirect buffers, since
      they're not autosaved.  */
   BUF_AUTOSAVE_MODIFF (current_buffer) = MODIFF;
-  XSETFASTINT (BVAR (current_buffer, save_length), Z - BEG);
+  bset_save_length (current_buffer, make_fixed_natnum (Z - BEG));
   current_buffer->auto_save_failure_time = 0;
   return Qnil;
 }
diff --git a/src/window.c b/src/window.c
index 6cd3122b43..ee435e395e 100644
--- a/src/window.c
+++ b/src/window.c
@@ -7117,9 +7117,8 @@ the return value is nil.  Otherwise the value is t.  */)
 	 Do not record the buffer here.  We do that in a separate call
 	 to select_window below.  See also Bug#16207.  */
       select_window (data->current_window, Qt, true);
-      BVAR (XBUFFER (XWINDOW (selected_window)->contents),
-	    last_selected_window)
-	= selected_window;
+      bset_last_selected_window (XBUFFER (XWINDOW (selected_window)->contents),
+                                 selected_window);
 
       if (NILP (data->focus_frame)
 	  || (FRAMEP (data->focus_frame)
-- 
2.28.0

[PATCH v2 07/16] Take offset not idx in PER_BUFFER_VALUE_P

[Spencer Baugh]

This also moves the common "idx == -1" check into PER_BUFFER_VALUE_P.
If idx == -1, it's guaranteed that the corresponding buffer field has
a per-buffer value.  We do this check everywhere we call
PER_BUFFER_VALUE_P, so let's put it in the common code.

This is motivated by later commits which will get rid of idx.
---
 src/buffer.c |  3 +--
 src/buffer.h | 21 +++++++++++----------
 src/data.c   | 11 +++++------
 3 files changed, 17 insertions(+), 18 deletions(-)

diff --git a/src/buffer.c b/src/buffer.c
index aa951ca78f..a0bcbf38e5 100644
--- a/src/buffer.c
+++ b/src/buffer.c
@@ -1297,8 +1297,7 @@ buffer_lisp_local_variables (struct buffer *buf, bool clone)
 static Lisp_Object
 buffer_local_variables_1 (struct buffer *buf, int offset, Lisp_Object sym)
 {
-  int idx = PER_BUFFER_IDX (offset);
-  if ((idx == -1 || PER_BUFFER_VALUE_P (buf, idx))
+  if (PER_BUFFER_VALUE_P (buf, offset)
       && SYMBOLP (PER_BUFFER_SYMBOL (offset)))
     {
       sym = NILP (sym) ? PER_BUFFER_SYMBOL (offset) : sym;
diff --git a/src/buffer.h b/src/buffer.h
index 770a5d03e6..69df791b53 100644
--- a/src/buffer.h
+++ b/src/buffer.h
@@ -1420,16 +1420,6 @@ OVERLAY_POSITION (Lisp_Object p)
 
 extern bool valid_per_buffer_idx (int);
 
-/* Value is true if the variable with index IDX has a local value
-   in buffer B.  */
-
-INLINE bool
-PER_BUFFER_VALUE_P (struct buffer *b, int idx)
-{
-  eassert (valid_per_buffer_idx (idx));
-  return b->local_flags[idx];
-}
-
 /* Set whether per-buffer variable with index IDX has a buffer-local
    value in buffer B.  VAL zero means it hasn't.  */
 
@@ -1496,6 +1486,17 @@ set_per_buffer_value (struct buffer *b, int offset, Lisp_Object value)
   *(Lisp_Object *)(offset + (char *) b) = value;
 }
 
+/* Value is true if the variable with offset OFFSET has a local value
+   in buffer B.  */
+
+INLINE bool
+PER_BUFFER_VALUE_P (struct buffer *b, int offset)
+{
+  int idx = PER_BUFFER_IDX (offset);
+  eassert (idx == -1 || valid_per_buffer_idx (idx));
+  return idx == -1 || b->local_flags[idx];
+}
+
 /* Downcase a character C, or make no change if that cannot be done.  */
 INLINE int
 downcase (int c)
diff --git a/src/data.c b/src/data.c
index 384c259220..47a1a6640f 100644
--- a/src/data.c
+++ b/src/data.c
@@ -1123,7 +1123,7 @@ store_symval_forwarding (lispfwd valcontents, Lisp_Object newval,
 	    {
 	      struct buffer *b = XBUFFER (buf);
 
-	      if (! PER_BUFFER_VALUE_P (b, idx))
+	      if (! PER_BUFFER_VALUE_P (b, offset))
 		set_per_buffer_value (b, offset, newval);
 	    }
 	}
@@ -1440,8 +1440,8 @@ set_internal (Lisp_Object symbol, Lisp_Object newval, Lisp_Object where,
 	  {
 	    int offset = XBUFFER_OBJFWD (innercontents)->offset;
 	    int idx = PER_BUFFER_IDX (offset);
-	    if (idx > 0 && bindflag == SET_INTERNAL_SET
-	        && !PER_BUFFER_VALUE_P (buf, idx))
+	    if (bindflag == SET_INTERNAL_SET
+	        && !PER_BUFFER_VALUE_P (buf, offset))
 	      {
 		if (let_shadows_buffer_binding_p (sym))
 		  set_default_internal (symbol, newval, bindflag);
@@ -1740,7 +1740,7 @@ set_default_internal (Lisp_Object symbol, Lisp_Object value,
 		  {
 		    struct buffer *b = XBUFFER (buf);
 
-		    if (!PER_BUFFER_VALUE_P (b, idx))
+		    if (!PER_BUFFER_VALUE_P (b, offset))
 		      set_per_buffer_value (b, offset, value);
 		  }
 	      }
@@ -2077,8 +2077,7 @@ BUFFER defaults to the current buffer.  */)
 	if (BUFFER_OBJFWDP (valcontents))
 	  {
 	    int offset = XBUFFER_OBJFWD (valcontents)->offset;
-	    int idx = PER_BUFFER_IDX (offset);
-	    if (idx == -1 || PER_BUFFER_VALUE_P (buf, idx))
+	    if (PER_BUFFER_VALUE_P (buf, offset))
 	      return Qt;
 	  }
 	return Qnil;
-- 
2.28.0

[PATCH v2 08/16] Combine unnecessarily separate loops in buffer.c

[Spencer Baugh]

These loops iterate over the same things with the same check.
---
 src/buffer.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/src/buffer.c b/src/buffer.c
index a0bcbf38e5..5333db73bd 100644
--- a/src/buffer.c
+++ b/src/buffer.c
@@ -984,7 +984,7 @@ reset_buffer (register struct buffer *b)
 static void
 reset_buffer_local_variables (struct buffer *b, bool permanent_too)
 {
-  int offset, i;
+  int offset;
 
   /* Reset the major mode to Fundamental, together with all the
      things that depend on the major mode.
@@ -1078,10 +1078,6 @@ reset_buffer_local_variables (struct buffer *b, bool permanent_too)
         }
     }
 
-  for (i = 0; i < last_per_buffer_idx; ++i)
-    if (permanent_too || buffer_permanent_local_flags[i] == 0)
-      SET_PER_BUFFER_VALUE_P (b, i, 0);
-
   /* For each slot that has a default value, copy that into the slot.  */
   FOR_EACH_PER_BUFFER_OBJECT_AT (offset)
     {
@@ -1089,7 +1085,10 @@ reset_buffer_local_variables (struct buffer *b, bool permanent_too)
       if ((idx > 0
 	   && (permanent_too
 	       || buffer_permanent_local_flags[idx] == 0)))
-	set_per_buffer_value (b, offset, per_buffer_default (offset));
+        {
+          SET_PER_BUFFER_VALUE_P (b, idx, 0);
+          set_per_buffer_value (b, offset, per_buffer_default (offset));
+        }
     }
 }
 
-- 
2.28.0

[PATCH v2 09/16] Add and use BUFFER_DEFAULT_VALUE_P

[Spencer Baugh]

This makes the code more clear, IMO.  And this gets rid of some more
usage of idx, which we'll remove later.
---
 src/buffer.c |  2 +-
 src/buffer.h | 10 ++++++++++
 src/data.c   |  5 ++---
 3 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/src/buffer.c b/src/buffer.c
index 5333db73bd..47a40a4284 100644
--- a/src/buffer.c
+++ b/src/buffer.c
@@ -1082,7 +1082,7 @@ reset_buffer_local_variables (struct buffer *b, bool permanent_too)
   FOR_EACH_PER_BUFFER_OBJECT_AT (offset)
     {
       int idx = PER_BUFFER_IDX (offset);
-      if ((idx > 0
+      if ((BUFFER_DEFAULT_VALUE_P (offset)
 	   && (permanent_too
 	       || buffer_permanent_local_flags[idx] == 0)))
         {
diff --git a/src/buffer.h b/src/buffer.h
index 69df791b53..16a5f223f8 100644
--- a/src/buffer.h
+++ b/src/buffer.h
@@ -1486,6 +1486,16 @@ set_per_buffer_value (struct buffer *b, int offset, Lisp_Object value)
   *(Lisp_Object *)(offset + (char *) b) = value;
 }
 
+/* Value is true if the variable with offset OFFSET has a default
+   value; false if the variable has no default, and is therefore
+   always local. */
+
+INLINE bool
+BUFFER_DEFAULT_VALUE_P (int offset)
+{
+  return PER_BUFFER_IDX (offset) > 0;
+}
+
 /* Value is true if the variable with offset OFFSET has a local value
    in buffer B.  */
 
diff --git a/src/data.c b/src/data.c
index 47a1a6640f..bfb7decc09 100644
--- a/src/data.c
+++ b/src/data.c
@@ -1720,13 +1720,12 @@ set_default_internal (Lisp_Object symbol, Lisp_Object value,
 	if (BUFFER_OBJFWDP (valcontents))
 	  {
 	    int offset = XBUFFER_OBJFWD (valcontents)->offset;
-	    int idx = PER_BUFFER_IDX (offset);
 
 	    set_per_buffer_default (offset, value);
 
 	    /* If this variable is not always local in all buffers,
 	       set it in the buffers that don't nominally have a local value.  */
-	    if (idx > 0)
+	    if (BUFFER_DEFAULT_VALUE_P (offset))
 	      {
 		Lisp_Object buf, tail;
 
@@ -1996,7 +1995,7 @@ From now on the default value will apply in this buffer.  Return VARIABLE.  */)
 	    int offset = XBUFFER_OBJFWD (valcontents)->offset;
 	    int idx = PER_BUFFER_IDX (offset);
 
-	    if (idx > 0)
+	    if (BUFFER_DEFAULT_VALUE_P (offset))
 	      {
 		SET_PER_BUFFER_VALUE_P (current_buffer, idx, 0);
 		set_per_buffer_value (current_buffer, offset,
-- 
2.28.0

[PATCH v2 10/16] Add and use KILL_PER_BUFFER_VALUE

[Spencer Baugh]

This improves clarity and removes usage of idx.
---
 src/buffer.c |  5 +----
 src/buffer.h | 10 ++++++++++
 src/data.c   |  8 +-------
 3 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/src/buffer.c b/src/buffer.c
index 47a40a4284..b05fdbd9b2 100644
--- a/src/buffer.c
+++ b/src/buffer.c
@@ -1085,10 +1085,7 @@ reset_buffer_local_variables (struct buffer *b, bool permanent_too)
       if ((BUFFER_DEFAULT_VALUE_P (offset)
 	   && (permanent_too
 	       || buffer_permanent_local_flags[idx] == 0)))
-        {
-          SET_PER_BUFFER_VALUE_P (b, idx, 0);
-          set_per_buffer_value (b, offset, per_buffer_default (offset));
-        }
+        KILL_PER_BUFFER_VALUE (b, offset);
     }
 }
 
diff --git a/src/buffer.h b/src/buffer.h
index 16a5f223f8..b95379db82 100644
--- a/src/buffer.h
+++ b/src/buffer.h
@@ -1507,6 +1507,16 @@ PER_BUFFER_VALUE_P (struct buffer *b, int offset)
   return idx == -1 || b->local_flags[idx];
 }
 
+/* Kill the per-buffer binding for this value, if there is one. */
+
+INLINE void
+KILL_PER_BUFFER_VALUE (struct buffer *b, int offset)
+{
+  int idx = PER_BUFFER_IDX (offset);
+  SET_PER_BUFFER_VALUE_P (b, idx, 0);
+  set_per_buffer_value (b, offset, per_buffer_default (offset));
+}
+
 /* Downcase a character C, or make no change if that cannot be done.  */
 INLINE int
 downcase (int c)
diff --git a/src/data.c b/src/data.c
index bfb7decc09..b7ad4ef1db 100644
--- a/src/data.c
+++ b/src/data.c
@@ -1993,14 +1993,8 @@ From now on the default value will apply in this buffer.  Return VARIABLE.  */)
 	if (BUFFER_OBJFWDP (valcontents))
 	  {
 	    int offset = XBUFFER_OBJFWD (valcontents)->offset;
-	    int idx = PER_BUFFER_IDX (offset);
-
 	    if (BUFFER_DEFAULT_VALUE_P (offset))
-	      {
-		SET_PER_BUFFER_VALUE_P (current_buffer, idx, 0);
-		set_per_buffer_value (current_buffer, offset,
-				      per_buffer_default (offset));
-	      }
+              KILL_PER_BUFFER_VALUE (current_buffer, offset);
 	  }
 	return variable;
       }
-- 
2.28.0

[PATCH v2 11/16] Assert that PER_BUFFER_IDX for Lisp variables is not 0

[Spencer Baugh]

PER_BUFFER_IDX can't be 0 for Lisp variables - so this if-check was
always pointless.
---
 src/data.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/data.c b/src/data.c
index b7ad4ef1db..f472127599 100644
--- a/src/data.c
+++ b/src/data.c
@@ -1626,8 +1626,8 @@ default_value (Lisp_Object symbol)
 	if (BUFFER_OBJFWDP (valcontents))
 	  {
 	    int offset = XBUFFER_OBJFWD (valcontents)->offset;
-	    if (PER_BUFFER_IDX (offset) != 0)
-	      return per_buffer_default (offset);
+	    eassert (PER_BUFFER_IDX (offset) != 0);
+            return per_buffer_default (offset);
 	  }
 
 	/* For other variables, get the current value.  */
-- 
2.28.0

[PATCH v2 12/16] Rearrange set_internal for buffer forwarded symbols

[Spencer Baugh]

Previously, we unnecessarily called store_symval_forwarding even after
calling set_default_internal.  store_symval_forwarding would have no
additional effect after a call to set_default_internal.

Now, we don't call store_symval_forwarding if we've called
set_default_internal.  As a consequence, we can also move the call to
SET_PER_BUFFER_VALUE_P into store_symval_forwarding, where it's closer
to the per-buffer-value actually being set, which improves clarity.

We'll later move the effect of SET_PER_BUFFER_VALUE_P into
set_per_buffer_value, so they need to be together.

Still, this change is a little ugly, there's maybe some other way to
do it that is a little more clear...
---
 src/data.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/src/data.c b/src/data.c
index f472127599..53b08a1aa4 100644
--- a/src/data.c
+++ b/src/data.c
@@ -1163,6 +1163,9 @@ store_symval_forwarding (lispfwd valcontents, Lisp_Object newval,
 	if (buf == NULL)
 	  buf = current_buffer;
 	set_per_buffer_value (buf, offset, newval);
+        int idx = PER_BUFFER_IDX (offset);
+        if (idx > 0)
+          SET_PER_BUFFER_VALUE_P (buf, idx, 1);
       }
       break;
 
@@ -1436,17 +1439,16 @@ set_internal (Lisp_Object symbol, Lisp_Object newval, Lisp_Object where,
 	struct buffer *buf
 	  = BUFFERP (where) ? XBUFFER (where) : current_buffer;
 	lispfwd innercontents = SYMBOL_FWD (sym);
+        bool should_store = true;
 	if (BUFFER_OBJFWDP (innercontents))
 	  {
 	    int offset = XBUFFER_OBJFWD (innercontents)->offset;
-	    int idx = PER_BUFFER_IDX (offset);
 	    if (bindflag == SET_INTERNAL_SET
-	        && !PER_BUFFER_VALUE_P (buf, offset))
+	        && !PER_BUFFER_VALUE_P (buf, offset)
+                && let_shadows_buffer_binding_p (sym))
 	      {
-		if (let_shadows_buffer_binding_p (sym))
-		  set_default_internal (symbol, newval, bindflag);
-		else
-		  SET_PER_BUFFER_VALUE_P (buf, idx, 1);
+                set_default_internal (symbol, newval, bindflag);
+		should_store = false;
 	      }
 	  }
 
@@ -1456,7 +1458,7 @@ set_internal (Lisp_Object symbol, Lisp_Object newval, Lisp_Object where,
 	    sym->u.s.redirect = SYMBOL_PLAINVAL;
 	    SET_SYMBOL_VAL (sym, newval);
 	  }
-	else
+	else if (should_store)
 	  store_symval_forwarding (/* sym, */ innercontents, newval, buf);
 	break;
       }
-- 
2.28.0

[PATCH v2 13/16] Get rid of buffer_permanent_local_flags array

[Spencer Baugh]

This is only used in one place, for two variables; we can just special
case those variables in the place it's used.
---
 lisp/bindings.el |  3 ++-
 src/buffer.c     | 28 +++++++++-------------------
 2 files changed, 11 insertions(+), 20 deletions(-)

diff --git a/lisp/bindings.el b/lisp/bindings.el
index 250234e94c..ffa1c94584 100644
--- a/lisp/bindings.el
+++ b/lisp/bindings.el
@@ -771,7 +771,8 @@ okay.  See `mode-line-format'.")
 ;; `kill-all-local-variables', because they have no default value.
 ;; For consistency, we give them the `permanent-local' property, even
 ;; though `kill-all-local-variables' does not actually consult it.
-;; See init_buffer_once in buffer.c for the origins of this list.
+;; See init_buffer_once and reset_buffer_local_variables in buffer.c
+;; for the origins of this list.
 
 (mapc (lambda (sym) (put sym 'permanent-local t))
       '(buffer-file-name default-directory buffer-backed-up
diff --git a/src/buffer.c b/src/buffer.c
index b05fdbd9b2..e7f9573eb5 100644
--- a/src/buffer.c
+++ b/src/buffer.c
@@ -94,10 +94,6 @@ struct buffer buffer_local_symbols;
   ((ptrdiff_t) min (MOST_POSITIVE_FIXNUM,				\
 		    min (PTRDIFF_MAX, SIZE_MAX) / word_size))
 
-/* Flags indicating which built-in buffer-local variables
-   are permanent locals.  */
-static char buffer_permanent_local_flags[MAX_PER_BUFFER_VARS];
-
 /* Number of per-buffer variables used.  */
 
 static int last_per_buffer_idx;
@@ -1081,10 +1077,14 @@ reset_buffer_local_variables (struct buffer *b, bool permanent_too)
   /* For each slot that has a default value, copy that into the slot.  */
   FOR_EACH_PER_BUFFER_OBJECT_AT (offset)
     {
-      int idx = PER_BUFFER_IDX (offset);
       if ((BUFFER_DEFAULT_VALUE_P (offset)
-	   && (permanent_too
-	       || buffer_permanent_local_flags[idx] == 0)))
+           && (permanent_too
+               /* Special case these two for backwards-compat; they're
+                  flagged as permanent-locals in bindings.el, even
+                  though they do have default values. */
+               || (offset != PER_BUFFER_VAR_OFFSET (truncate_lines)
+                   && offset !=
+                   PER_BUFFER_VAR_OFFSET (buffer_file_coding_system)))))
         KILL_PER_BUFFER_VALUE (b, offset);
     }
 }
@@ -5119,7 +5119,6 @@ init_buffer_once (void)
 
      buffer_defaults: default values of buffer-locals
      buffer_local_flags: metadata
-     buffer_permanent_local_flags: metadata
      buffer_local_symbols: metadata
 
      There must be a simpler way to store the metadata.
@@ -5127,11 +5126,6 @@ init_buffer_once (void)
 
   int idx;
 
-  /* Items flagged permanent get an explicit permanent-local property
-     added in bindings.el, for clarity.  */
-  PDUMPER_REMEMBER_SCALAR (buffer_permanent_local_flags);
-  memset (buffer_permanent_local_flags, 0, sizeof buffer_permanent_local_flags);
-
   /* 0 means not a lisp var, -1 means always local, else mask.  */
   memset (&buffer_local_flags, 0, sizeof buffer_local_flags);
   bset_filename (&buffer_local_flags, make_fixnum (-1));
@@ -5178,9 +5172,7 @@ init_buffer_once (void)
   XSETFASTINT (BVAR (&buffer_local_flags, selective_display), idx); ++idx;
   XSETFASTINT (BVAR (&buffer_local_flags, selective_display_ellipses), idx); ++idx;
   XSETFASTINT (BVAR (&buffer_local_flags, tab_width), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, truncate_lines), idx);
-  /* Make this one a permanent local.  */
-  buffer_permanent_local_flags[idx++] = 1;
+  XSETFASTINT (BVAR (&buffer_local_flags, truncate_lines), idx); ++idx;
   XSETFASTINT (BVAR (&buffer_local_flags, word_wrap), idx); ++idx;
   XSETFASTINT (BVAR (&buffer_local_flags, ctl_arrow), idx); ++idx;
   XSETFASTINT (BVAR (&buffer_local_flags, fill_column), idx); ++idx;
@@ -5194,9 +5186,7 @@ init_buffer_once (void)
   XSETFASTINT (BVAR (&buffer_local_flags, bidi_paragraph_direction), idx); ++idx;
   XSETFASTINT (BVAR (&buffer_local_flags, bidi_paragraph_separate_re), idx); ++idx;
   XSETFASTINT (BVAR (&buffer_local_flags, bidi_paragraph_start_re), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, buffer_file_coding_system), idx);
-  /* Make this one a permanent local.  */
-  buffer_permanent_local_flags[idx++] = 1;
+  XSETFASTINT (BVAR (&buffer_local_flags, buffer_file_coding_system), idx); ++idx;
   XSETFASTINT (BVAR (&buffer_local_flags, left_margin_cols), idx); ++idx;
   XSETFASTINT (BVAR (&buffer_local_flags, right_margin_cols), idx); ++idx;
   XSETFASTINT (BVAR (&buffer_local_flags, left_fringe_width), idx); ++idx;
-- 
2.28.0

[PATCH v2 14/16] Remove unnecessary Qunbound check

[Spencer Baugh]

This can never be the case.
---
 src/buffer.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/src/buffer.c b/src/buffer.c
index e7f9573eb5..7e0b19e19a 100644
--- a/src/buffer.c
+++ b/src/buffer.c
@@ -1297,8 +1297,7 @@ buffer_local_variables_1 (struct buffer *buf, int offset, Lisp_Object sym)
       && SYMBOLP (PER_BUFFER_SYMBOL (offset)))
     {
       sym = NILP (sym) ? PER_BUFFER_SYMBOL (offset) : sym;
-      Lisp_Object val = per_buffer_value (buf, offset);
-      return EQ (val, Qunbound) ? sym : Fcons (sym, val);
+      return Fcons (sym, per_buffer_value (buf, offset));
     }
   return Qnil;
 }
-- 
2.28.0

[PATCH v2 15/16] Remove local_flags array in struct buffer

[Spencer Baugh]

Previously, we used the local_flags array in each struct buffer to
know which field was buffer-local.  Now, we just set the field to
Qunbound if it's not buffer-local, and fall back to the corresponding
field in buffer_defaults.  So we can delete the local_flags array.

Because of this fallback logic, we now no longer need to update all
struct buffer objects whenever we set a value in buffer_defaults,
which provides a substantial performance gain.

The fallback logic applies for all accesses to Lisp_Object fields in
struct buffer, but it should now be very fast: It's one branch on
already-loaded data to check if it's equal to the Qunbound constant.

In these changes, we just add the Qunbound fall back logic, delete
local_flags, and delete several loops which updated all buffers when
buffer_defaults changed.

We also delete some calls to reset_buffer_local_variables on
buffer_defaults and buffer_local_symbols.  Since resetting a
buffer-local now requires setting it to Qunbound, those would set
fields on buffer_defaults and buffer_local_symbols.  Fortunately, the
calls are not necessary anymore.
---
 src/buffer.c           | 119 +++++++++++++++++++----------------------
 src/buffer.h           |  54 ++++++++-----------
 src/category.c         |   4 --
 src/data.c             |  53 +-----------------
 src/pdumper.c          |   3 --
 src/syntax.c           |   4 --
 test/src/data-tests.el |   1 -
 7 files changed, 79 insertions(+), 159 deletions(-)

diff --git a/src/buffer.c b/src/buffer.c
index 7e0b19e19a..b565d232f6 100644
--- a/src/buffer.c
+++ b/src/buffer.c
@@ -55,8 +55,7 @@ along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.  */
    defined with DEFVAR_PER_BUFFER, that have special slots in each buffer.
    The default value occupies the same slot in this structure
    as an individual buffer's value occupies in that buffer.
-   Setting the default value also goes through the alist of buffers
-   and stores into each buffer that does not say it has a local value.  */
+*/
 
 struct buffer buffer_defaults;
 
@@ -89,6 +88,8 @@ struct buffer buffer_local_symbols;
 #define PER_BUFFER_SYMBOL(OFFSET) \
       (*(Lisp_Object *)((OFFSET) + (char *) &buffer_local_symbols))
 
+#define BVAR_DIRECT(buf, field) ((buf)->field ## _)
+
 /* Maximum length of an overlay vector.  */
 #define OVERLAY_COUNT_MAX						\
   ((ptrdiff_t) min (MOST_POSITIVE_FIXNUM,				\
@@ -535,8 +536,6 @@ even if it is dead.  The return value is never nil.  */)
   /* No one shows us now.  */
   b->window_count = 0;
 
-  memset (&b->local_flags, 0, sizeof (b->local_flags));
-
   BUF_GAP_SIZE (b) = 20;
   block_input ();
   /* We allocate extra 1-byte at the tail and keep it always '\0' for
@@ -697,8 +696,6 @@ clone_per_buffer_values (struct buffer *from, struct buffer *to)
       set_per_buffer_value (to, offset, obj);
     }
 
-  memcpy (to->local_flags, from->local_flags, sizeof to->local_flags);
-
   set_buffer_overlays_before (to, copy_overlays (to, from->overlays_before));
   set_buffer_overlays_after (to, copy_overlays (to, from->overlays_after));
 
@@ -800,8 +797,6 @@ CLONE nil means the indirect buffer's state is reset to default values.  */)
   /* Always -1 for an indirect buffer.  */
   b->window_count = -1;
 
-  memset (&b->local_flags, 0, sizeof (b->local_flags));
-
   b->pt = b->base_buffer->pt;
   b->begv = b->base_buffer->begv;
   b->zv = b->base_buffer->zv;
@@ -963,8 +958,6 @@ reset_buffer (register struct buffer *b)
   bset_display_count (b, make_fixnum (0));
   bset_display_time (b, Qnil);
   bset_enable_multibyte_characters (b, Qt);
-  bset_cursor_type (b, BVAR (&buffer_defaults, cursor_type));
-  bset_extra_line_spacing (b, BVAR (&buffer_defaults, extra_line_spacing));
 
   b->display_error_modiff = 0;
 }
@@ -1238,7 +1231,7 @@ buffer_local_value (Lisp_Object variable, Lisp_Object buffer)
       {
 	lispfwd fwd = SYMBOL_FWD (sym);
 	if (BUFFER_OBJFWDP (fwd))
-	  result = per_buffer_value (buf, XBUFFER_OBJFWD (fwd)->offset);
+          result = bvar_get (buf, XBUFFER_OBJFWD (fwd)->offset);
 	else
 	  result = Fdefault_value (variable);
 	break;
@@ -5163,49 +5156,49 @@ init_buffer_once (void)
   bset_last_selected_window (&buffer_local_flags, make_fixnum (0));
 
   idx = 1;
-  XSETFASTINT (BVAR (&buffer_local_flags, mode_line_format), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, abbrev_mode), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, overwrite_mode), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, case_fold_search), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, auto_fill_function), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, selective_display), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, selective_display_ellipses), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, tab_width), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, truncate_lines), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, word_wrap), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, ctl_arrow), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, fill_column), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, left_margin), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, abbrev_table), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, display_table), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, syntax_table), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, cache_long_scans), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, category_table), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, bidi_display_reordering), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, bidi_paragraph_direction), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, bidi_paragraph_separate_re), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, bidi_paragraph_start_re), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, buffer_file_coding_system), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, left_margin_cols), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, right_margin_cols), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, left_fringe_width), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, right_fringe_width), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, fringes_outside_margins), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, scroll_bar_width), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, scroll_bar_height), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, vertical_scroll_bar_type), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, horizontal_scroll_bar_type), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, indicate_empty_lines), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, indicate_buffer_boundaries), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, fringe_indicator_alist), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, fringe_cursor_alist), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, scroll_up_aggressively), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, scroll_down_aggressively), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, header_line_format), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, tab_line_format), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, cursor_type), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, extra_line_spacing), idx); ++idx;
-  XSETFASTINT (BVAR (&buffer_local_flags, cursor_in_non_selected_windows), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, mode_line_format), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, abbrev_mode), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, overwrite_mode), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, case_fold_search), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, auto_fill_function), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, selective_display), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, selective_display_ellipses), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, tab_width), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, truncate_lines), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, word_wrap), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, ctl_arrow), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, fill_column), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, left_margin), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, abbrev_table), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, display_table), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, syntax_table), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, cache_long_scans), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, category_table), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, bidi_display_reordering), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, bidi_paragraph_direction), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, bidi_paragraph_separate_re), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, bidi_paragraph_start_re), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, buffer_file_coding_system), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, left_margin_cols), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, right_margin_cols), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, left_fringe_width), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, right_fringe_width), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, fringes_outside_margins), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, scroll_bar_width), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, scroll_bar_height), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, vertical_scroll_bar_type), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, horizontal_scroll_bar_type), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, indicate_empty_lines), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, indicate_buffer_boundaries), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, fringe_indicator_alist), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, fringe_cursor_alist), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, scroll_up_aggressively), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, scroll_down_aggressively), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, header_line_format), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, tab_line_format), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, cursor_type), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, extra_line_spacing), idx); ++idx;
+  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, cursor_in_non_selected_windows), idx); ++idx;
 
   /* buffer_local_flags contains no pointers, so it's safe to treat it
      as a blob for pdumper.  */
@@ -5220,11 +5213,9 @@ init_buffer_once (void)
   /* Make sure all markable slots in buffer_defaults
      are initialized reasonably, so mark_buffer won't choke.  */
   reset_buffer (&buffer_defaults);
-  eassert (NILP (BVAR (&buffer_defaults, name)));
-  reset_buffer_local_variables (&buffer_defaults, 1);
-  eassert (NILP (BVAR (&buffer_local_symbols, name)));
+  eassert (NILP (BVAR_DIRECT (&buffer_defaults, name)));
+  eassert (NILP (BVAR_DIRECT (&buffer_local_symbols, name)));
   reset_buffer (&buffer_local_symbols);
-  reset_buffer_local_variables (&buffer_local_symbols, 1);
   /* Prevent GC from getting confused.  */
   buffer_defaults.text = &buffer_defaults.own_text;
   buffer_local_symbols.text = &buffer_local_symbols.own_text;
@@ -5265,7 +5256,7 @@ init_buffer_once (void)
   set_buffer_overlays_after (&buffer_defaults, NULL);
   buffer_defaults.overlay_center = BEG;
 
-  XSETFASTINT (BVAR (&buffer_defaults, tab_width), 8);
+  XSETFASTINT (BVAR_DIRECT (&buffer_defaults, tab_width), 8);
   bset_truncate_lines (&buffer_defaults, Qnil);
   bset_word_wrap (&buffer_defaults, Qnil);
   bset_ctl_arrow (&buffer_defaults, Qt);
@@ -5279,13 +5270,13 @@ init_buffer_once (void)
 
   bset_enable_multibyte_characters (&buffer_defaults, Qt);
   bset_buffer_file_coding_system (&buffer_defaults, Qnil);
-  XSETFASTINT (BVAR (&buffer_defaults, fill_column), 70);
-  XSETFASTINT (BVAR (&buffer_defaults, left_margin), 0);
+  XSETFASTINT (BVAR_DIRECT (&buffer_defaults, fill_column), 70);
+  XSETFASTINT (BVAR_DIRECT (&buffer_defaults, left_margin), 0);
   bset_cache_long_scans (&buffer_defaults, Qt);
   bset_file_truename (&buffer_defaults, Qnil);
-  XSETFASTINT (BVAR (&buffer_defaults, display_count), 0);
-  XSETFASTINT (BVAR (&buffer_defaults, left_margin_cols), 0);
-  XSETFASTINT (BVAR (&buffer_defaults, right_margin_cols), 0);
+  XSETFASTINT (BVAR_DIRECT (&buffer_defaults, display_count), 0);
+  XSETFASTINT (BVAR_DIRECT (&buffer_defaults, left_margin_cols), 0);
+  XSETFASTINT (BVAR_DIRECT (&buffer_defaults, right_margin_cols), 0);
   bset_left_fringe_width (&buffer_defaults, Qnil);
   bset_right_fringe_width (&buffer_defaults, Qnil);
   bset_fringes_outside_margins (&buffer_defaults, Qnil);
diff --git a/src/buffer.h b/src/buffer.h
index b95379db82..ef44da0787 100644
--- a/src/buffer.h
+++ b/src/buffer.h
@@ -280,9 +280,12 @@ struct buffer_text
     bool_bf redisplay : 1;
   };
 
+INLINE Lisp_Object
+bvar_get (struct buffer *b, ptrdiff_t offset);
+
 /* Most code should use this macro to access Lisp fields in struct buffer.  */
 
-#define BVAR(buf, field) ((buf)->field ## _)
+#define BVAR(buf, field) bvar_get (buf, PER_BUFFER_VAR_OFFSET (field))
 
 /* Access a BVAR from buffer_defaults */
 #define BVAR_DEFAULT(field) (buffer_defaults.field ## _)
@@ -601,13 +604,6 @@ struct buffer
      an indirect buffer since it counts as its base buffer.  */
   int window_count;
 
-  /* A non-zero value in slot IDX means that per-buffer variable
-     with index IDX has a local value in this buffer.  The index IDX
-     for a buffer-local variable is stored in that variable's slot
-     in buffer_local_flags as a Lisp integer.  If the index is -1,
-     this means the variable is always local in all buffers.  */
-  char local_flags[MAX_PER_BUFFER_VARS];
-
   /* Set to the modtime of the visited file when read or written.
      modtime.tv_nsec == NONEXISTENT_MODTIME_NSECS means
      visited file was nonexistent.  modtime.tv_nsec ==
@@ -692,6 +688,12 @@ struct buffer
   Lisp_Object undo_list_;
 };
 
+/* Return the offset in bytes of member VAR of struct buffer
+   from the start of a buffer structure.  */
+
+#define PER_BUFFER_VAR_OFFSET(VAR) \
+  offsetof (struct buffer, VAR ## _)
+
 INLINE bool
 BUFFERP (Lisp_Object a)
 {
@@ -1110,8 +1112,7 @@ BUFFER_CHECK_INDIRECTION (struct buffer *b)
    that have special slots in each buffer.
    The default value occupies the same slot in this structure
    as an individual buffer's value occupies in that buffer.
-   Setting the default value also goes through the alist of buffers
-   and stores into each buffer that does not say it has a local value.  */
+*/
 
 extern struct buffer buffer_defaults;
 
@@ -1394,12 +1395,6 @@ OVERLAY_POSITION (Lisp_Object p)
 			Buffer-local Variables
  ***********************************************************************/
 
-/* Return the offset in bytes of member VAR of struct buffer
-   from the start of a buffer structure.  */
-
-#define PER_BUFFER_VAR_OFFSET(VAR) \
-  offsetof (struct buffer, VAR ## _)
-
 /* Used to iterate over normal Lisp_Object fields of struct buffer (all
    Lisp_Objects except undo_list).  If you add, remove, or reorder
    Lisp_Objects in a struct buffer, make sure that this is still correct.  */
@@ -1420,16 +1415,6 @@ OVERLAY_POSITION (Lisp_Object p)
 
 extern bool valid_per_buffer_idx (int);
 
-/* Set whether per-buffer variable with index IDX has a buffer-local
-   value in buffer B.  VAL zero means it hasn't.  */
-
-INLINE void
-SET_PER_BUFFER_VALUE_P (struct buffer *b, int idx, bool val)
-{
-  eassert (valid_per_buffer_idx (idx));
-  b->local_flags[idx] = val;
-}
-
 /* Return the index value of the per-buffer variable at offset OFFSET
    in the buffer structure.
 
@@ -1502,9 +1487,16 @@ BUFFER_DEFAULT_VALUE_P (int offset)
 INLINE bool
 PER_BUFFER_VALUE_P (struct buffer *b, int offset)
 {
-  int idx = PER_BUFFER_IDX (offset);
-  eassert (idx == -1 || valid_per_buffer_idx (idx));
-  return idx == -1 || b->local_flags[idx];
+  return !EQ (per_buffer_value (b, offset), Qunbound);
+}
+
+INLINE Lisp_Object
+bvar_get (struct buffer *b, ptrdiff_t offset)
+{
+  Lisp_Object val = per_buffer_value (b, offset);
+  return EQ (val, Qunbound)
+    ? per_buffer_default (offset)
+    : val;
 }
 
 /* Kill the per-buffer binding for this value, if there is one. */
@@ -1512,9 +1504,7 @@ PER_BUFFER_VALUE_P (struct buffer *b, int offset)
 INLINE void
 KILL_PER_BUFFER_VALUE (struct buffer *b, int offset)
 {
-  int idx = PER_BUFFER_IDX (offset);
-  SET_PER_BUFFER_VALUE_P (b, idx, 0);
-  set_per_buffer_value (b, offset, per_buffer_default (offset));
+  set_per_buffer_value (b, offset, Qunbound);
 }
 
 /* Downcase a character C, or make no change if that cannot be done.  */
diff --git a/src/category.c b/src/category.c
index c80571ecd4..657a2b2b65 100644
--- a/src/category.c
+++ b/src/category.c
@@ -268,12 +268,8 @@ DEFUN ("set-category-table", Fset_category_table, Sset_category_table, 1, 1, 0,
 Return TABLE.  */)
   (Lisp_Object table)
 {
-  int idx;
   table = check_category_table (table);
   bset_category_table (current_buffer, table);
-  /* Indicate that this buffer now has a specified category table.  */
-  idx = PER_BUFFER_VAR_IDX (category_table);
-  SET_PER_BUFFER_VALUE_P (current_buffer, idx, 1);
   return table;
 }
 
diff --git a/src/data.c b/src/data.c
index 53b08a1aa4..2e466ad207 100644
--- a/src/data.c
+++ b/src/data.c
@@ -1004,8 +1004,8 @@ do_symval_forwarding (lispfwd valcontents)
       return *XOBJFWD (valcontents)->objvar;
 
     case Lisp_Fwd_Buffer_Obj:
-      return per_buffer_value (current_buffer,
-			       XBUFFER_OBJFWD (valcontents)->offset);
+      return bvar_get (current_buffer,
+                       XBUFFER_OBJFWD (valcontents)->offset);
 
     case Lisp_Fwd_Kboard_Obj:
       /* We used to simply use current_kboard here, but from Lisp
@@ -1102,31 +1102,6 @@ store_symval_forwarding (lispfwd valcontents, Lisp_Object newval,
 
     case Lisp_Fwd_Obj:
       *XOBJFWD (valcontents)->objvar = newval;
-
-      /* If this variable is a default for something stored
-	 in the buffer itself, such as default-fill-column,
-	 find the buffers that don't have local values for it
-	 and update them.  */
-      if (XOBJFWD (valcontents)->objvar > (Lisp_Object *) &buffer_defaults
-	  && XOBJFWD (valcontents)->objvar < (Lisp_Object *) (&buffer_defaults + 1))
-	{
-	  int offset = ((char *) XOBJFWD (valcontents)->objvar
-			- (char *) &buffer_defaults);
-	  int idx = PER_BUFFER_IDX (offset);
-
-	  Lisp_Object tail, buf;
-
-	  if (idx <= 0)
-	    break;
-
-	  FOR_EACH_LIVE_BUFFER (tail, buf)
-	    {
-	      struct buffer *b = XBUFFER (buf);
-
-	      if (! PER_BUFFER_VALUE_P (b, offset))
-		set_per_buffer_value (b, offset, newval);
-	    }
-	}
       break;
 
     case Lisp_Fwd_Buffer_Obj:
@@ -1163,9 +1138,6 @@ store_symval_forwarding (lispfwd valcontents, Lisp_Object newval,
 	if (buf == NULL)
 	  buf = current_buffer;
 	set_per_buffer_value (buf, offset, newval);
-        int idx = PER_BUFFER_IDX (offset);
-        if (idx > 0)
-          SET_PER_BUFFER_VALUE_P (buf, idx, 1);
       }
       break;
 
@@ -1724,27 +1696,6 @@ set_default_internal (Lisp_Object symbol, Lisp_Object value,
 	    int offset = XBUFFER_OBJFWD (valcontents)->offset;
 
 	    set_per_buffer_default (offset, value);
-
-	    /* If this variable is not always local in all buffers,
-	       set it in the buffers that don't nominally have a local value.  */
-	    if (BUFFER_DEFAULT_VALUE_P (offset))
-	      {
-		Lisp_Object buf, tail;
-
-		/* Do this only in live buffers, so that if there are
-		   a lot of buffers which are dead, that doesn't slow
-		   down let-binding of variables that are
-		   automatically local when set, like
-		   case-fold-search.  This is for Lisp programs that
-		   let-bind such variables in their inner loops.  */
-		FOR_EACH_LIVE_BUFFER (tail, buf)
-		  {
-		    struct buffer *b = XBUFFER (buf);
-
-		    if (!PER_BUFFER_VALUE_P (b, offset))
-		      set_per_buffer_value (b, offset, value);
-		  }
-	      }
 	  }
 	else
           set_internal (symbol, value, Qnil, bindflag);
diff --git a/src/pdumper.c b/src/pdumper.c
index b5b4050b93..e722abda96 100644
--- a/src/pdumper.c
+++ b/src/pdumper.c
@@ -2785,9 +2785,6 @@ dump_buffer (struct dump_context *ctx, const struct buffer *in_buffer)
   DUMP_FIELD_COPY (out, buffer, indirections);
   DUMP_FIELD_COPY (out, buffer, window_count);
 
-  memcpy (out->local_flags,
-          &buffer->local_flags,
-          sizeof (out->local_flags));
   DUMP_FIELD_COPY (out, buffer, modtime);
   DUMP_FIELD_COPY (out, buffer, modtime_size);
   DUMP_FIELD_COPY (out, buffer, auto_save_modified);
diff --git a/src/syntax.c b/src/syntax.c
index df07809aaa..f20d5542a9 100644
--- a/src/syntax.c
+++ b/src/syntax.c
@@ -1041,12 +1041,8 @@ DEFUN ("set-syntax-table", Fset_syntax_table, Sset_syntax_table, 1, 1, 0,
 One argument, a syntax table.  */)
   (Lisp_Object table)
 {
-  int idx;
   check_syntax_table (table);
   bset_syntax_table (current_buffer, table);
-  /* Indicate that this buffer now has a specified syntax table.  */
-  idx = PER_BUFFER_VAR_IDX (syntax_table);
-  SET_PER_BUFFER_VALUE_P (current_buffer, idx, 1);
   return table;
 }
 \f
diff --git a/test/src/data-tests.el b/test/src/data-tests.el
index 7c403870f1..cd39e6e0a4 100644
--- a/test/src/data-tests.el
+++ b/test/src/data-tests.el
@@ -424,7 +424,6 @@ comparing the subr with a much slower lisp implementation."
   (with-no-warnings (should (setq :keyword :keyword))))
 
 (ert-deftest data-tests--set-default-per-buffer ()
-  :expected-result t ;; Not fixed yet!
   ;; FIXME: Performance tests are inherently unreliable.
   ;; Using wall-clock time makes it even worse, so don't bother unless
   ;; we have the primitive to measure cpu-time.
-- 
2.28.0

[PATCH v2 16/16] Remove usage of buffer_local_flags

[Spencer Baugh]

Previously, we used the buffer_local_flags structure to track which
fields in buffer_defaults contain usable defaults, as well as the
index into local_flags to use for each struct buffer field.

After the immediate previous commit, we don't need the index into
local_flags anymore.  So all we need is to know whether the fields in
buffer_defaults contain usable defaults.  We can do that by just
setting unusable and unused buffer_defaults fields to Qunbound. Then
we can delete buffer_local_flags.

In these changes, we delete buffer_local_flags and ensure that
buffer_defaults is initialized appropriately for this new usage.
---
 src/buffer.c | 208 ++++++++-------------------------------------------
 src/buffer.h |  58 +-------------
 src/data.c   |   1 -
 3 files changed, 34 insertions(+), 233 deletions(-)

diff --git a/src/buffer.c b/src/buffer.c
index b565d232f6..338624f5a4 100644
--- a/src/buffer.c
+++ b/src/buffer.c
@@ -55,28 +55,11 @@ along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.  */
    defined with DEFVAR_PER_BUFFER, that have special slots in each buffer.
    The default value occupies the same slot in this structure
    as an individual buffer's value occupies in that buffer.
-*/
+   Slots in this structure which are set to Qunbound are permanently
+   buffer-local. */
 
 struct buffer buffer_defaults;
 
-/* This structure marks which slots in a buffer have corresponding
-   default values in buffer_defaults.
-   Each such slot has a nonzero value in this structure.
-   The value has only one nonzero bit.
-
-   When a buffer has its own local value for a slot,
-   the entry for that slot (found in the same slot in this structure)
-   is turned on in the buffer's local_flags array.
-
-   If a slot in this structure is -1, then even though there may
-   be a DEFVAR_PER_BUFFER for the slot, there is no default value for it;
-   and the corresponding slot in buffer_defaults is not used.
-
-   If a slot in this structure corresponding to a DEFVAR_PER_BUFFER is
-   zero, that is a bug.  */
-
-struct buffer buffer_local_flags;
-
 /* This structure holds the names of symbols whose values may be
    buffer-local.  It is indexed and accessed in the same way as the above.  */
 
@@ -95,10 +78,6 @@ struct buffer buffer_local_symbols;
   ((ptrdiff_t) min (MOST_POSITIVE_FIXNUM,				\
 		    min (PTRDIFF_MAX, SIZE_MAX) / word_size))
 
-/* Number of per-buffer variables used.  */
-
-static int last_per_buffer_idx;
-
 static void call_overlay_mod_hooks (Lisp_Object list, Lisp_Object overlay,
                                     bool after, Lisp_Object arg1,
                                     Lisp_Object arg2, Lisp_Object arg3);
@@ -658,12 +637,6 @@ set_buffer_overlays_after (struct buffer *b, struct Lisp_Overlay *o)
   b->overlays_after = o;
 }
 
-bool
-valid_per_buffer_idx (int idx)
-{
-  return 0 <= idx && idx < last_per_buffer_idx;
-}
-
 /* Clone per-buffer values of buffer FROM.
 
    Buffer TO gets the same per-buffer values as FROM, with the
@@ -5106,135 +5079,20 @@ free_buffer_text (struct buffer *b)
 void
 init_buffer_once (void)
 {
-  /* TODO: clean up the buffer-local machinery.  Right now,
-     we have:
-
-     buffer_defaults: default values of buffer-locals
-     buffer_local_flags: metadata
-     buffer_local_symbols: metadata
-
-     There must be a simpler way to store the metadata.
-  */
-
-  int idx;
-
-  /* 0 means not a lisp var, -1 means always local, else mask.  */
-  memset (&buffer_local_flags, 0, sizeof buffer_local_flags);
-  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));
-
-  /* These used to be stuck at 0 by default, but now that the all-zero value
-     means Qnil, we have to initialize them explicitly.  */
-  bset_name (&buffer_local_flags, make_fixnum (0));
-  bset_mark (&buffer_local_flags, make_fixnum (0));
-  bset_local_var_alist (&buffer_local_flags, make_fixnum (0));
-  bset_keymap (&buffer_local_flags, make_fixnum (0));
-  bset_downcase_table (&buffer_local_flags, make_fixnum (0));
-  bset_upcase_table (&buffer_local_flags, make_fixnum (0));
-  bset_case_canon_table (&buffer_local_flags, make_fixnum (0));
-  bset_case_eqv_table (&buffer_local_flags, make_fixnum (0));
-  bset_width_table (&buffer_local_flags, make_fixnum (0));
-  bset_pt_marker (&buffer_local_flags, make_fixnum (0));
-  bset_begv_marker (&buffer_local_flags, make_fixnum (0));
-  bset_zv_marker (&buffer_local_flags, make_fixnum (0));
-  bset_last_selected_window (&buffer_local_flags, make_fixnum (0));
-
-  idx = 1;
-  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, mode_line_format), idx); ++idx;
-  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, abbrev_mode), idx); ++idx;
-  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, overwrite_mode), idx); ++idx;
-  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, case_fold_search), idx); ++idx;
-  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, auto_fill_function), idx); ++idx;
-  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, selective_display), idx); ++idx;
-  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, selective_display_ellipses), idx); ++idx;
-  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, tab_width), idx); ++idx;
-  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, truncate_lines), idx); ++idx;
-  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, word_wrap), idx); ++idx;
-  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, ctl_arrow), idx); ++idx;
-  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, fill_column), idx); ++idx;
-  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, left_margin), idx); ++idx;
-  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, abbrev_table), idx); ++idx;
-  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, display_table), idx); ++idx;
-  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, syntax_table), idx); ++idx;
-  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, cache_long_scans), idx); ++idx;
-  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, category_table), idx); ++idx;
-  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, bidi_display_reordering), idx); ++idx;
-  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, bidi_paragraph_direction), idx); ++idx;
-  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, bidi_paragraph_separate_re), idx); ++idx;
-  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, bidi_paragraph_start_re), idx); ++idx;
-  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, buffer_file_coding_system), idx); ++idx;
-  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, left_margin_cols), idx); ++idx;
-  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, right_margin_cols), idx); ++idx;
-  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, left_fringe_width), idx); ++idx;
-  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, right_fringe_width), idx); ++idx;
-  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, fringes_outside_margins), idx); ++idx;
-  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, scroll_bar_width), idx); ++idx;
-  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, scroll_bar_height), idx); ++idx;
-  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, vertical_scroll_bar_type), idx); ++idx;
-  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, horizontal_scroll_bar_type), idx); ++idx;
-  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, indicate_empty_lines), idx); ++idx;
-  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, indicate_buffer_boundaries), idx); ++idx;
-  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, fringe_indicator_alist), idx); ++idx;
-  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, fringe_cursor_alist), idx); ++idx;
-  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, scroll_up_aggressively), idx); ++idx;
-  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, scroll_down_aggressively), idx); ++idx;
-  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, header_line_format), idx); ++idx;
-  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, tab_line_format), idx); ++idx;
-  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, cursor_type), idx); ++idx;
-  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, extra_line_spacing), idx); ++idx;
-  XSETFASTINT (BVAR_DIRECT (&buffer_local_flags, cursor_in_non_selected_windows), idx); ++idx;
-
-  /* buffer_local_flags contains no pointers, so it's safe to treat it
-     as a blob for pdumper.  */
-  PDUMPER_REMEMBER_SCALAR (buffer_local_flags);
-
-  /* Need more room? */
-  if (idx >= MAX_PER_BUFFER_VARS)
-    emacs_abort ();
-  last_per_buffer_idx = idx;
-  PDUMPER_REMEMBER_SCALAR (last_per_buffer_idx);
-
-  /* Make sure all markable slots in buffer_defaults
-     are initialized reasonably, so mark_buffer won't choke.  */
-  reset_buffer (&buffer_defaults);
-  eassert (NILP (BVAR_DIRECT (&buffer_defaults, name)));
-  eassert (NILP (BVAR_DIRECT (&buffer_local_symbols, name)));
-  reset_buffer (&buffer_local_symbols);
-  /* Prevent GC from getting confused.  */
-  buffer_defaults.text = &buffer_defaults.own_text;
-  buffer_local_symbols.text = &buffer_local_symbols.own_text;
-  /* No one will share the text with these buffers, but let's play it safe.  */
-  buffer_defaults.indirections = 0;
-  buffer_local_symbols.indirections = 0;
-  /* Likewise no one will display them.  */
-  buffer_defaults.window_count = 0;
-  buffer_local_symbols.window_count = 0;
-  set_buffer_intervals (&buffer_defaults, NULL);
-  set_buffer_intervals (&buffer_local_symbols, NULL);
-  /* This is not strictly necessary, but let's make them initialized.  */
-  bset_name (&buffer_defaults, build_pure_c_string (" *buffer-defaults*"));
-  bset_name (&buffer_local_symbols, build_pure_c_string (" *buffer-local-symbols*"));
   BUFFER_PVEC_INIT (&buffer_defaults);
   BUFFER_PVEC_INIT (&buffer_local_symbols);
 
   /* Set up the default values of various buffer slots.  */
   /* Must do these before making the first buffer! */
+  int offset;
+  FOR_EACH_PER_BUFFER_OBJECT_AT (offset)
+    {
+      /* These are initialized before us. */
+      if (!(offset == PER_BUFFER_VAR_OFFSET (syntax_table)
+            || offset == PER_BUFFER_VAR_OFFSET (category_table)))
+        set_per_buffer_default (offset, Qunbound);
+    }
+  set_per_buffer_default (PER_BUFFER_VAR_OFFSET (undo_list), Qunbound);
 
   /* real setup is done in bindings.el */
   bset_mode_line_format (&buffer_defaults, build_pure_c_string ("%-"));
@@ -5248,13 +5106,6 @@ init_buffer_once (void)
   bset_selective_display_ellipses (&buffer_defaults, Qt);
   bset_abbrev_table (&buffer_defaults, Qnil);
   bset_display_table (&buffer_defaults, Qnil);
-  bset_undo_list (&buffer_defaults, Qnil);
-  bset_mark_active (&buffer_defaults, Qnil);
-  bset_file_format (&buffer_defaults, Qnil);
-  bset_auto_save_file_format (&buffer_defaults, Qt);
-  set_buffer_overlays_before (&buffer_defaults, NULL);
-  set_buffer_overlays_after (&buffer_defaults, NULL);
-  buffer_defaults.overlay_center = BEG;
 
   XSETFASTINT (BVAR_DIRECT (&buffer_defaults, tab_width), 8);
   bset_truncate_lines (&buffer_defaults, Qnil);
@@ -5268,13 +5119,10 @@ init_buffer_once (void)
   bset_extra_line_spacing (&buffer_defaults, Qnil);
   bset_cursor_in_non_selected_windows (&buffer_defaults, Qt);
 
-  bset_enable_multibyte_characters (&buffer_defaults, Qt);
   bset_buffer_file_coding_system (&buffer_defaults, Qnil);
   XSETFASTINT (BVAR_DIRECT (&buffer_defaults, fill_column), 70);
   XSETFASTINT (BVAR_DIRECT (&buffer_defaults, left_margin), 0);
   bset_cache_long_scans (&buffer_defaults, Qt);
-  bset_file_truename (&buffer_defaults, Qnil);
-  XSETFASTINT (BVAR_DIRECT (&buffer_defaults, display_count), 0);
   XSETFASTINT (BVAR_DIRECT (&buffer_defaults, left_margin_cols), 0);
   XSETFASTINT (BVAR_DIRECT (&buffer_defaults, right_margin_cols), 0);
   bset_left_fringe_width (&buffer_defaults, Qnil);
@@ -5290,15 +5138,6 @@ init_buffer_once (void)
   bset_fringe_cursor_alist (&buffer_defaults, Qnil);
   bset_scroll_up_aggressively (&buffer_defaults, Qnil);
   bset_scroll_down_aggressively (&buffer_defaults, Qnil);
-  bset_display_time (&buffer_defaults, Qnil);
-
-  /* Assign the local-flags to the slots that have default values.
-     The local flag is a bit that is used in the buffer
-     to say that it has its own local value for the slot.
-     The local flag bits are in the local_var_flags slot of the buffer.  */
-
-  /* Nothing can work if this isn't true.  */
-  { verify (sizeof (EMACS_INT) == word_size); }
 
   Vbuffer_alist = Qnil;
   current_buffer = 0;
@@ -5316,6 +5155,26 @@ init_buffer_once (void)
   DEFSYM (Qkill_buffer_hook, "kill-buffer-hook");
   Fput (Qkill_buffer_hook, Qpermanent_local, Qt);
 
+  /* Sanity check that we didn't set the default for slots which
+     are permanent-buffer-locals. */
+  eassert (EQ (BVAR (&buffer_defaults, filename), Qunbound));
+  eassert (EQ (BVAR (&buffer_defaults, directory), Qunbound));
+  eassert (EQ (BVAR (&buffer_defaults, backed_up), Qunbound));
+  eassert (EQ (BVAR (&buffer_defaults, save_length), Qunbound));
+  eassert (EQ (BVAR (&buffer_defaults, auto_save_file_name), Qunbound));
+  eassert (EQ (BVAR (&buffer_defaults, read_only), Qunbound));
+  eassert (EQ (BVAR (&buffer_defaults, mode_name), Qunbound));
+  eassert (EQ (BVAR (&buffer_defaults, undo_list), Qunbound));
+  eassert (EQ (BVAR (&buffer_defaults, mark_active), Qunbound));
+  eassert (EQ (BVAR (&buffer_defaults, point_before_scroll), Qunbound));
+  eassert (EQ (BVAR (&buffer_defaults, file_truename), Qunbound));
+  eassert (EQ (BVAR (&buffer_defaults, invisibility_spec), Qunbound));
+  eassert (EQ (BVAR (&buffer_defaults, file_format), Qunbound));
+  eassert (EQ (BVAR (&buffer_defaults, auto_save_file_format), Qunbound));
+  eassert (EQ (BVAR (&buffer_defaults, display_count), Qunbound));
+  eassert (EQ (BVAR (&buffer_defaults, display_time), Qunbound));
+  eassert (EQ (BVAR (&buffer_defaults, enable_multibyte_characters), Qunbound));
+
   /* Super-magic invisible buffer.  */
   Vprin1_to_string_buffer = Fget_buffer_create (build_pure_c_string (" prin1"));
   Vbuffer_alist = Qnil;
@@ -5440,11 +5299,6 @@ defvar_per_buffer (struct Lisp_Buffer_Objfwd *bo_fwd, const char *namestring,
   sym->u.s.redirect = SYMBOL_FORWARDED;
   SET_SYMBOL_FWD (sym, bo_fwd);
   XSETSYMBOL (PER_BUFFER_SYMBOL (offset), sym);
-
-  if (PER_BUFFER_IDX (offset) == 0)
-    /* Did a DEFVAR_PER_BUFFER without initializing the corresponding
-       slot of buffer_local_flags.  */
-    emacs_abort ();
 }
 
 
diff --git a/src/buffer.h b/src/buffer.h
index ef44da0787..0186783e39 100644
--- a/src/buffer.h
+++ b/src/buffer.h
@@ -1112,26 +1112,11 @@ BUFFER_CHECK_INDIRECTION (struct buffer *b)
    that have special slots in each buffer.
    The default value occupies the same slot in this structure
    as an individual buffer's value occupies in that buffer.
-*/
+   Slots in this structure which are set to Qunbound are permanently
+   buffer-local. */
 
 extern struct buffer buffer_defaults;
 
-/* This structure marks which slots in a buffer have corresponding
-   default values in buffer_defaults.
-   Each such slot has a nonzero value in this structure.
-   The value has only one nonzero bit.
-
-   When a buffer has its own local value for a slot,
-   the entry for that slot (found in the same slot in this structure)
-   is turned on in the buffer's local_flags array.
-
-   If a slot in this structure is zero, then even though there may
-   be a Lisp-level local variable for the slot, it has no default value,
-   and the corresponding slot in buffer_defaults is not used.  */
-
-
-extern struct buffer buffer_local_flags;
-
 /* For each buffer slot, this points to the Lisp symbol name
    for that slot in the current buffer.  It is 0 for slots
    that don't have such names.  */
@@ -1404,43 +1389,6 @@ OVERLAY_POSITION (Lisp_Object p)
        offset <= PER_BUFFER_VAR_OFFSET (cursor_in_non_selected_windows); \
        offset += word_size)
 
-/* Return the index of buffer-local variable VAR.  Each per-buffer
-   variable has an index > 0 associated with it, except when it always
-   has buffer-local values, in which case the index is -1.  If this is
-   0, this is a bug and means that the slot of VAR in
-   buffer_local_flags wasn't initialized.  */
-
-#define PER_BUFFER_VAR_IDX(VAR) \
-    PER_BUFFER_IDX (PER_BUFFER_VAR_OFFSET (VAR))
-
-extern bool valid_per_buffer_idx (int);
-
-/* Return the index value of the per-buffer variable at offset OFFSET
-   in the buffer structure.
-
-   If the slot OFFSET has a corresponding default value in
-   buffer_defaults, the index value is positive and has only one
-   nonzero bit.  When a buffer has its own local value for a slot, the
-   bit for that slot (found in the same slot in this structure) is
-   turned on in the buffer's local_flags array.
-
-   If the index value is -1, even though there may be a
-   DEFVAR_PER_BUFFER for the slot, there is no default value for it;
-   and the corresponding slot in buffer_defaults is not used.
-
-   If the index value is -2, then there is no DEFVAR_PER_BUFFER for
-   the slot, but there is a default value which is copied into each
-   new buffer.
-
-   If a slot in this structure corresponding to a DEFVAR_PER_BUFFER is
-   zero, that is a bug.  */
-
-INLINE int
-PER_BUFFER_IDX (ptrdiff_t offset)
-{
-  return XFIXNUM (*(Lisp_Object *) (offset + (char *) &buffer_local_flags));
-}
-
 /* Functions to get and set default value of the per-buffer
    variable at offset OFFSET in the buffer structure.  */
 
@@ -1478,7 +1426,7 @@ set_per_buffer_value (struct buffer *b, int offset, Lisp_Object value)
 INLINE bool
 BUFFER_DEFAULT_VALUE_P (int offset)
 {
-  return PER_BUFFER_IDX (offset) > 0;
+  return !EQ (per_buffer_default (offset), Qunbound);
 }
 
 /* Value is true if the variable with offset OFFSET has a local value
diff --git a/src/data.c b/src/data.c
index 2e466ad207..3db235dd77 100644
--- a/src/data.c
+++ b/src/data.c
@@ -1600,7 +1600,6 @@ default_value (Lisp_Object symbol)
 	if (BUFFER_OBJFWDP (valcontents))
 	  {
 	    int offset = XBUFFER_OBJFWD (valcontents)->offset;
-	    eassert (PER_BUFFER_IDX (offset) != 0);
             return per_buffer_default (offset);
 	  }
 
-- 
2.28.0

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

[Kévin Le Gouguec]

Spencer Baugh <sbaugh@catern.com> writes:

> (My commit messages are still not in ChangeLog style - sorry - with 16
> patches in the series right now, and having not yet personally figured
> out a good magit-integrated way to generate ChangeLog-appropriate
> messages, I figured I'm better off adjusting the commit messages at
> the end of the review process, rather than continually throughout. If
> anyone has a recommendation for ChangeLog generation that is
> integrated with magit, that would be welcome - I haven't found
> anything yet...)

Hitting 'C' on a hunk in the magit-diff buffer inserts a ChangeLog
heading into the COMMIT_EDITMSG buffer.

If there's only one function in the hunk, you get the "full" heading:

* foo.c (bar):

Otherwise you just get "* foo.c:", and you must go from function to
function and hit 'C' for magit to insert further "(bar):" headings.

Hopefully someone somewhere is yelling at their screen "WHAT NO THAT'S
NOT HOW YOU DO IT; YOU JUST HIT <something> AND MAGIT FILLS IN THE
COMMIT_EDITMSG BUFFER WITH ALL THE HEADINGS FOR ALL FILES AT ONCE!!1".
Hopefully they will also followup to this message :)

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

[Eli Zaretskii]

> From: Spencer Baugh <sbaugh@catern.com>
> Date: Sat, 21 Nov 2020 21:34:29 -0500
> Cc: Spencer Baugh <sbaugh@catern.com>, Arnold Noronha <arnold@tdrhq.com>,
>  Stefan Monnier <monnier@iro.umontreal.ca>, Dmitry Gutov <dgutov@yandex.ru>
> 
> This patch series reworks access to Lisp_Object fields in struct
> buffer. The primary motivation is performance of DEFVAR_PER_BUFFER
> Lisp variables, but this also removes several now-unnecessary pieces
> of metadata and generally simplifies things.
> 
> Relative to v1, this patch series now performs a larger refactoring of
> struct buffer field access, which should provide for good performance
> while keeping the access method uniform.  I've also corrected style
> issues and bugs pointed out in review, and added some testing for
> buffer-local let variables.  (Thanks Stefan for the review, and the
> tests you added in the fix for bug#44733, both were quite helpful.)
> 
> It's possible to provide further performance improvements [...]

Thanks, but could you perhaps provide a more detailed overview of the
general idea of the changeset?  It is not easy to glean that from 16
patches, each one describing only the details of its narrow goal.
Please include in the more detailed overview indications what parts of
the series are really needed to speed up buffer-local variables and
which are cleanups in the areas of the changes not directly related to
speeding things up.

> I have an idea which would allow avoiding the mass change, if that's
> desired, which also has some additional benefits.  We could define
> getter functions (marked as inline) for each Lisp_Object field, and
> have BVAR call those field-specific getter functions by forming the
> name of the getter through token pasting.  The boilerplate of the
> getters would be kept under control by using an X macro to define both
> the fields in struct buffer and the getters. Something like:
> 
> #define FIELD_LIST \
>   C_FIELD(name) \
>   C_FIELD(filename) \
>   ...
>   LISP_FIELD(mode_line_format) \
>   ...
> 
> And then "instantiating" FIELD_LIST twice in two different contexts to
> define the Lisp_Object fields and the getters.

I don't think I have a clear idea of what will this mean in practice.
Can you show an example of its usage?

> Regardless of what approach we take, at least some level of
> benchmarking will be necessary.  Is there an established benchmark
> suite for these kinds of changes?

Is benchmark-run what you are after?  Or are you asking what use cases
of buffer-local bindings to use inside the benchmark?  If the latter,
then I guess some simple example with binding case-fold-search
followed by some regexp search would do, if you do that with various
numbers of live buffers in the session.

Thanks.

Re: [PATCH v2 13/16] Get rid of buffer_permanent_local_flags array

[Eli Zaretskii]

> From: Spencer Baugh <sbaugh@catern.com>
> Date: Sat, 21 Nov 2020 21:34:42 -0500
> Cc: Spencer Baugh <sbaugh@catern.com>, Arnold Noronha <arnold@tdrhq.com>,
>  Stefan Monnier <monnier@iro.umontreal.ca>, Dmitry Gutov <dgutov@yandex.ru>
> 
> This is only used in one place, for two variables; we can just special
> case those variables in the place it's used.

You assume that we will never want to use this kind of infrastructure
for any other variables?  The fact that we currently use this only for
2 variables doesn't mean it doesn't have a more general value, or that
we introduced it for those 2 variables alone.

Perhaps if you explained the general idea of the changes, I could have
answered these questions myself, but as things stand, I don't think I
understand how important this patch is for the general purpose of the
series -- just one example of where a more detailed overview could be
of value.

Thanks.

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

[Spencer Baugh]

Eli Zaretskii <eliz@gnu.org> writes:
> Thanks, but could you perhaps provide a more detailed overview of the
> general idea of the changeset?  It is not easy to glean that from 16
> patches, each one describing only the details of its narrow goal.
> Please include in the more detailed overview indications what parts of
> the series are really needed to speed up buffer-local variables and
> which are cleanups in the areas of the changes not directly related to
> speeding things up.

Patch 1 and 2 are tests; patch 15 is the speed-up change. Patches 3
through 14 all change the existing abstractions so that patch 15 can be
small, and should have no impact. Patch 16 is a followup further cleanup
enabled by 15. Only 15 should have any meaningful performance impact.

>> I have an idea which would allow avoiding the mass change, if that's
>> desired, which also has some additional benefits.  We could define
>> getter functions (marked as inline) for each Lisp_Object field, and
>> have BVAR call those field-specific getter functions by forming the
>> name of the getter through token pasting.  The boilerplate of the
>> getters would be kept under control by using an X macro to define both
>> the fields in struct buffer and the getters. Something like:
>> 
>> #define FIELD_LIST \
>>   C_FIELD(name) \
>>   C_FIELD(filename) \
>>   ...
>>   LISP_FIELD(mode_line_format) \
>>   ...
>> 
>> And then "instantiating" FIELD_LIST twice in two different contexts to
>> define the Lisp_Object fields and the getters.
>
> I don't think I have a clear idea of what will this mean in practice.
> Can you show an example of its usage?

There is a decent example on https://en.wikipedia.org/wiki/X_Macro

In our case, it would be something like
struct buffer {
#define C_FIELD(field) Lisp_Object field;
#define LISP_FIELD(field) Lisp_Object field;
       FIELD_LIST
#undef C_FIELD
#undef LISP_FIELD
}

#define C_FIELD(field) Lisp_Object bget_ ## (struct buffer *b) \
{ return b->field ## _; }
#define LISP_FIELD(field) Lisp_Object bget_ ## (struct buffer *b) \
{ return bvar_get(b, offsetof(struct buffer, field)); }
       FIELD_LIST
#undef C_FIELD
#undef LISP_FIELD

#define BVAR(b, field) bget_ ## field (b)

>> Regardless of what approach we take, at least some level of
>> benchmarking will be necessary.  Is there an established benchmark
>> suite for these kinds of changes?
>
> Is benchmark-run what you are after?  Or are you asking what use cases
> of buffer-local bindings to use inside the benchmark?  If the latter,
> then I guess some simple example with binding case-fold-search
> followed by some regexp search would do, if you do that with various
> numbers of live buffers in the session.

Ah, no, I mean benchmarking the rest of Emacs to see the impact.  I've
already benchmarked these changes on the case they're intended to fix.
Old results, but no change in this iteration:

(benchmark-run 500000 (let ((case-fold-search nil)) (string-match "" "")))
; unpatched emacs: (0.6872330339999999 0 0.0)
; patched emacs: (0.608068564 0 0.0)
(setq my-buffer-list nil)
(dotimes (idx 1000) (push (get-buffer-create (format "test-buffer:%s" idx)) my-buffer-list))

(benchmark-run 500000 (let ((case-fold-search nil)) (string-match "" "")))
; unpatched emacs: (18.591189848 0 0.0)
; patched emacs: (0.5885462539999999 0 0.0)
	

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, Richard Stallman <rms@gnu.org>
> Date: Sun, 22 Nov 2020 11:28:27 -0500
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> > Thanks, but could you perhaps provide a more detailed overview of the
> > general idea of the changeset?  It is not easy to glean that from 16
> > patches, each one describing only the details of its narrow goal.
> > Please include in the more detailed overview indications what parts of
> > the series are really needed to speed up buffer-local variables and
> > which are cleanups in the areas of the changes not directly related to
> > speeding things up.
> 
> Patch 1 and 2 are tests; patch 15 is the speed-up change. Patches 3
> through 14 all change the existing abstractions so that patch 15 can be
> small, and should have no impact. Patch 16 is a followup further cleanup
> enabled by 15. Only 15 should have any meaningful performance impact.

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.

TIA.

> In our case, it would be something like
> struct buffer {
> #define C_FIELD(field) Lisp_Object field;
> #define LISP_FIELD(field) Lisp_Object field;
>        FIELD_LIST
> #undef C_FIELD
> #undef LISP_FIELD
> }
> 
> #define C_FIELD(field) Lisp_Object bget_ ## (struct buffer *b) \
> { return b->field ## _; }
> #define LISP_FIELD(field) Lisp_Object bget_ ## (struct buffer *b) \
> { return bvar_get(b, offsetof(struct buffer, field)); }
>        FIELD_LIST
> #undef C_FIELD
> #undef LISP_FIELD
> 
> #define BVAR(b, field) bget_ ## field (b)

Thanks.  I think this looks like too complex and hard to read for the
benefit it brings.  What do others think about using this technique?

> Ah, no, I mean benchmarking the rest of Emacs to see the impact.

Perform some heavy text-process task many times and benchmark that, I
guess.  Rendering HTML by shr.el comes to mind.

> (benchmark-run 500000 (let ((case-fold-search nil)) (string-match "" "")))
> ; unpatched emacs: (0.6872330339999999 0 0.0)
> ; patched emacs: (0.608068564 0 0.0)
> (setq my-buffer-list nil)
> (dotimes (idx 1000) (push (get-buffer-create (format "test-buffer:%s" idx)) my-buffer-list))
> 
> (benchmark-run 500000 (let ((case-fold-search nil)) (string-match "" "")))
> ; unpatched emacs: (18.591189848 0 0.0)
> ; patched emacs: (0.5885462539999999 0 0.0)

I've seen those, but they are too synthetic to be interesting.  I
meant a real regexp search, in a large buffer.

Re: [PATCH v2 15/16] Remove local_flags array in struct buffer

[Stefan Monnier]

> Previously, we used the local_flags array in each struct buffer to
> know which field was buffer-local.  Now, we just set the field to
> Qunbound if it's not buffer-local, and fall back to the corresponding
> field in buffer_defaults.  So we can delete the local_flags array.

That's actually the approach I had in mind in my list of "things
I should do at some point" ;-)

I haven't had a chance to look at the actual patches yet, tho.


        Stefan

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

[Stefan Monnier]

> Ah, no, I mean benchmarking the rest of Emacs to see the impact.  I've
> already benchmarked these changes on the case they're intended to fix.

There's the `elisp-benchmark` suite in GNU ELPA, but that tests the
execution time to run ELisp code, which I can't imagine can be
significantly affected by your patch.  The other parts that could be
affected could be the time for redisplay or some other primitives that
repeatedly look at things like `BVAR (current_buffer, enable_multibyte_characters)`.

Tho, if you patch is careful not to do the "check for `Qunbound`" for
those vars whose buffer_local_flag said -1 (like
`enable_multibyte_characters`), then the risk is even lower.


        Stefan

Re: [PATCH v2 01/16] Add a test for let-binding unwinding

[Stefan Monnier]

> diff --git a/test/src/data-tests.el b/test/src/data-tests.el
> index 1312683c84..7e69b9c331 100644
> --- a/test/src/data-tests.el
> +++ b/test/src/data-tests.el
> @@ -364,6 +364,28 @@ comparing the subr with a much slower lisp implementation."
>            (should (equal (default-value var) (symbol-value var))))
>          (should (equal (default-value var) def))))))
>  
> +(ert-deftest data-tests--let-buffer-local-no-unwind-other-buffers ()
> +  "Test that a let-binding for a buffer-local unwinds only current-buffer"

Missing "." at the end.

> +  (let ((blvar (make-symbol "blvar")))
> +    (set-default blvar 0)
> +    (make-variable-buffer-local blvar)
> +    (dolist (var (list blvar 'left-margin))
> +      (let* ((def (default-value var))
> +             (newdef (+ def 1))
> +             (otherbuf (generate-new-buffer "otherbuf")))
> +        (with-temp-buffer
> +          (cl-progv (list var) (list newdef)
> +            (with-current-buffer otherbuf
> +              (set var 123)
> +              (should (local-variable-p var))
> +              (should (equal (symbol-value var) 123))
> +              (should (equal (default-value var) newdef))))
> +          (with-current-buffer otherbuf
> +            (should (local-variable-p var))
> +            (should (equal (symbol-value var) 123))
> +            (should (equal (default-value var) def)))
> +          )))))

LGTM


        Stefan

Re: [PATCH v2 02/16] Assert not local-variable-p after setq in let_default binding

[Stefan Monnier]

> diff --git a/test/src/data-tests.el b/test/src/data-tests.el
> index 7e69b9c331..7c403870f1 100644
> --- a/test/src/data-tests.el
> +++ b/test/src/data-tests.el
> @@ -358,6 +358,7 @@ comparing the subr with a much slower lisp implementation."
>              (should (equal (symbol-value var) 42))
>              (should (equal (default-value var) (symbol-value var)))
>              (set var 123)
> +            (should (not (local-variable-p var)))
>              (should (equal (symbol-value var) 123))
>              (should (equal (default-value var) (symbol-value var)))) ;bug#44733
>            (should (equal (symbol-value var) def))

LGTM,


        Stefan

Re: [PATCH v2 03/16] Stop checking the constant default for enable_multibyte_characters

[Stefan Monnier]

> The default is a constant "t", and can't be changed. So we don't need
> to check it.

Indeed.  It used to be changeable (running so-called "unibyte
sessions"), but that was removed several years ago.

This, like several other of the changes in this series, is not directly
related to the "speed up PER_BUFFER" change.  You might want to keep
them in a separate patch series to help the review.


        Stefan

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

[Spencer Baugh]

Eli Zaretskii <eliz@gnu.org> writes:
>> From: Spencer Baugh <sbaugh@catern.com>
>> Cc: emacs-devel@gnu.org, arnold@tdrhq.com, monnier@iro.umontreal.ca,
>>  dgutov@yandex.ru, Richard Stallman <rms@gnu.org>
>> Date: Sun, 22 Nov 2020 11:28:27 -0500
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> > Thanks, but could you perhaps provide a more detailed overview of the
>> > general idea of the changeset?  It is not easy to glean that from 16
>> > patches, each one describing only the details of its narrow goal.
>> > Please include in the more detailed overview indications what parts of
>> > the series are really needed to speed up buffer-local variables and
>> > which are cleanups in the areas of the changes not directly related to
>> > speeding things up.
>> 
>> Patch 1 and 2 are tests; patch 15 is the speed-up change. Patches 3
>> through 14 all change the existing abstractions so that patch 15 can be
>> small, and should have no impact. Patch 16 is a followup further cleanup
>> enabled by 15. Only 15 should have any meaningful performance impact.
>
> 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 ===

DEFVAR_PER_BUFFER is a C macro which sets up a correspondence between a
Lisp symbol and a field of type Lisp_Object in the C `struct buffer'
struct.  There are a finite number of such fields, and DEFVAR_PER_BUFFER
is called for a subset of them.

Each DEFVAR_PER_BUFFER variable appears to Lisp code as a buffer-local
variable, and should behave like pure Lisp buffer-local variables.  Each
DEFVAR_PER_BUFFER variable is either permanently buffer-local, or has a
default value which is used by buffers that don't currently have a
buffer-local binding.

If a buffer has a buffer-local binding for one of these variables, then
the per-buffer value is stored in the corresponding field in that
buffer's `struct buffer'.

Default values for these variables are stored in a global `struct
buffer' C variable, `buffer_defaults'.  This struct does not correspond
to a real buffer, and its non-DEFVAR_PER_BUFFER fields are unused.  When
a variable's default value is changed, the corresponding field is
changed in (at least) buffer_defaults.  When `default-value' is used to
read a DEFVAR_PER_BUFFER variable's default value, it's read from
buffer_defaults.

The underlying fields in `struct buffer' used with DEFVAR_PER_BUFFER are
also read and written directly from C, through the BVAR macro.  The BVAR
macro takes a pointer to a `struct buffer' and the name of a field in
`struct buffer', and evaluates to the value for that field.

The variables must behave the same both when used through the symbol in
Lisp, and used through BVAR in C.  For example, if BVAR reads a field
from a buffer that does not have a buffer-local binding for that field,
it should evaluate to the default value for that field.

=== Old implementation ===

In the old implementation, the BVAR macro is essentially a no-op.  It
turns into a simple field dereference, essentially:

  #define BVAR(buf, field) (buf)->field

We simply read the field directly out of the specific `struct buffer'
we're working with.  Neither BVAR nor surrounding C code checks whether
a buffer has a buffer-local binding for a variable before using this
field, and at no point does most code check what value is in
buffer_defaults.

This is fine for permanently buffer-local DEFVAR_PER_BUFFER variables,
which have no default value.

For variables which are not permanently buffer-local, though, this means
we need to ensure that the C Lisp_Object field always contains the
"correct" value for the variable, whether that's a currently
buffer-local value, or the global default value.

If there is a buffer-local binding, then the field contains the
per-buffer value, and all is well.

If there is no buffer-local binding, then we need to ensure that the
field contains the global default value.

To do this, whenever the global default value changes, we iterate over
all buffers, and if there's no buffer-local binding, set the field to
the new default value. This is O(n) in the number of buffers open in
Emacs - quite slow.

= Old implementation: local_flags and buffer_local_flags =

Also, we frequently need to know whether a variable has a buffer-local
binding.  We maintain this information with the `local_flags' field in
`struct buffer', which is a char array with an entry for each
DEFVAR_PER_BUFFER variable.

When we create or kill a buffer-local binding for a DEFVAR_PER_BUFFER
variable, we update local_flags.

To support local_flags, we need even further metadata;
buffer_local_flags is a global, initialized at startup and constant
after that, which maps the offsets of the DEFVAR_PER_BUFFER C fields to
indices in local_flags.  We perform a lookup in buffer_local_flags
whenever we need to change local_flags for a variable.

= Old implementation: BVAR as lvalue =

Also, the BVAR macro is, regrettably, used widely in C for fields which
are not DEFVAR_PER_BUFFER fields.

As a "benefit" of this implementation of BVAR, BVAR is an lvalue, so C
code can directly assign to it. For example:

  XSETINT (BVAR (b, save_length), -1);

For a DEFVAR_PER_BUFFER field, this code would be buggy, since it
wouldn't update local_flags, which would result in unexpected behavior
visible from Lisp.  There was already one bug of this sort in
let-binding from Lisp; fortunately, there don't appear to be any in C at
the moment.

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

= New implementation: No more local_flags and buffer_local_flags =

Both of these are now useless and can be deleted.

When we create a buffer-local binding for a DEFVAR_PER_BUFFER variable,
we simply set the field.  When we kill a buffer-local binding, we set
the field to Qunbound.  There's no additional metadata.

= New implementation: BVAR is not an lvalue =

BVAR is not an lvalue anymore.  We could preserve that property with
some hacks, but this is probably for the best.  Uses of BVAR to set
field values have been changed to use dedicated functions; there weren't
that many anyway.

BVAR is still used by C to read Lisp_Object fields which are not
DEFVAR_PER_BUFFER fields, as well as DEFVAR_PER_BUFFER fields
corresponding to permanent-buffer-local variables.  Such fields will
never be Qunbound, but they still go through the check.

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.

=== Individual commits ===

See the individual commit messages for more.

       Add a test for let-binding unwinding
       Assert not local-variable-p after setq in let_default binding

These are test changes; fairly self-explanatory.

       Stop checking the constant default for enable_multibyte_characters
       Take buffer field name in DEFVAR_PER_BUFFER
       Add BVAR_DEFAULT for access to buffer defaults
       Use bset_* functions instead of BVAR

These changes eliminate usage of BVAR which aren't supported by the new
implementation: either usage of BVAR as an lvalue, or usage of BVAR to
access buffer_defaults (or both at once).

       Take offset not idx in PER_BUFFER_VALUE_P
       Combine unnecessarily separate loops in buffer.c
       Add and use BUFFER_DEFAULT_VALUE_P
       Add and use KILL_PER_BUFFER_VALUE
       Assert that PER_BUFFER_IDX for Lisp variables is not 0
       Rearrange set_internal for buffer forwarded symbols

These change or add abstractions for accessing buffer Lisp_Object
fields, to remove dependencies on implementation details which will
change.

       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.

       Remove unnecessary Qunbound check

This removes an impossible-to-hit conditional that might otherwise be
confusing.

       Remove local_flags array in struct buffer
       Remove usage of buffer_local_flags

These are the actual remove-complexity, speed-things-up changes. See the
individual commit messages for more explanation of what happens in each.

Re: [PATCH v2 01/16] Add a test for let-binding unwinding

[Spencer Baugh]

Added that missing ".".

Perhaps this and the other test patch can be applied now, ahead of the
rest of the series, if they look good?

---
 test/src/data-tests.el | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/test/src/data-tests.el b/test/src/data-tests.el
index 1312683c84..7e69b9c331 100644
--- a/test/src/data-tests.el
+++ b/test/src/data-tests.el
@@ -364,6 +364,28 @@ comparing the subr with a much slower lisp implementation."
           (should (equal (default-value var) (symbol-value var))))
         (should (equal (default-value var) def))))))

+(ert-deftest data-tests--let-buffer-local-no-unwind-other-buffers ()
+  "Test that a let-binding for a buffer-local unwinds only current-buffer."
+  (let ((blvar (make-symbol "blvar")))
+    (set-default blvar 0)
+    (make-variable-buffer-local blvar)
+    (dolist (var (list blvar 'left-margin))
+      (let* ((def (default-value var))
+             (newdef (+ def 1))
+             (otherbuf (generate-new-buffer "otherbuf")))
+        (with-temp-buffer
+          (cl-progv (list var) (list newdef)
+            (with-current-buffer otherbuf
+              (set var 123)
+              (should (local-variable-p var))
+              (should (equal (symbol-value var) 123))
+              (should (equal (default-value var) newdef))))
+          (with-current-buffer otherbuf
+            (should (local-variable-p var))
+            (should (equal (symbol-value var) 123))
+            (should (equal (default-value var) def)))
+          )))))
+
 (ert-deftest binding-test-makunbound ()
   "Tests of makunbound, from the manual."
   (with-current-buffer binding-test-buffer-B
-- 
2.28.0

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.

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.

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

[Stefan Monnier]

> 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

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

[Andrea Corallo via Emacs development discussions.]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

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

Is not possible to provide hints to the branch predictor.  The
__builtin_expect is used only to hint GCC to reorder basic blocks so
that the most likely execution path is going to be sequential, this to
maximize i-cache efficiency.  I doubt a single __builtin_expect will
have a measurable impact here.

  Andrea

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: Mon, 30 Nov 2020 15:11:31 -0500
> 
> > 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.

In fact, the current code is slow only when the buffer that's current
at the moment of the let-binding doesn't have a buffer-local value for
the variable.  Because if the current buffer does have a local value,
the let-binding simply changes that single local value.

So in effect we make every access to built-in buffer-local variables
pay for the single use case when the let-binding is done in a buffer
that has no local value.  I'm not sure how to assess this kind of
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.

We should benchmark, but coming up with suitable use cases to make
these measurements is not easy.

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

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Eli Zaretskii <eliz@gnu.org>,  emacs-devel@gnu.org,  arnold@tdrhq.com,
>   dgutov@yandex.ru,  rms@gnu.org
> Date: Mon, 30 Nov 2020 17:10:20 -0500
> 
> > 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.

I think that could open a can of worms, whereby fun things can happen
if some Lisp removes the local bindings from one of these variables.

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

[Spencer Baugh]

Eli Zaretskii <eliz@gnu.org> writes:
>> 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: Mon, 30 Nov 2020 15:11:31 -0500
>> 
>> > 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.
>
> In fact, the current code is slow only when the buffer that's current
> at the moment of the let-binding doesn't have a buffer-local value for
> the variable.  Because if the current buffer does have a local value,
> the let-binding simply changes that single local value.

Just for the sake of completeness, note that set-default is also slow -
it's changing the default that's slow, which is done by both let-binding
without a buffer-local and set-default.

Lisp code is much less likely to call set-default in a tight loop, of
course.  Speculating wildly, maybe that's why this implementation was
chosen in the first place - the idea that it's fine for it to be slow to
change the default, since that should be a rare operation...

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

[Stefan Monnier]

> I think that could open a can of worms, whereby fun things can happen
> if some Lisp removes the local bindings from one of these variables.

I doubt this would be a can of worms, but yes this may introduce
regressions.  Based on my experiments, these should be very rare, so the
downside is pretty small and in the long run it's probably worthwhile.
This said, the upside is also pretty small, so you have to look fairly
far into the future before such a change becomes worthwhile.


        Stefan

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

[Stefan Monnier]

> So in effect we make every access to built-in buffer-local variables
> pay for the single use case when the let-binding is done in a buffer
> that has no local value.

Yup, increase the average time taken in order to avoid the
"pathological" worst case.

BTW, for the original problem (i.e. let-binding case-fold-search in
a buffer that doesn't have a buffer-local setting of that variable), we
could attack the problem in a different way:

1- change `case-fold-search` to a plain DEFVAR_LISP instead of a PER_BUFFER.
2- change its `let-binding` rule so it automatically becomes buffer-local
   within the `let` (i.e. the let-binding stops affecting other buffers).


        Stefan

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

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Spencer Baugh <sbaugh@catern.com>,  emacs-devel@gnu.org,
>   arnold@tdrhq.com,  dgutov@yandex.ru,  rms@gnu.org
> Date: Tue, 01 Dec 2020 11:01:12 -0500
> 
> BTW, for the original problem (i.e. let-binding case-fold-search in
> a buffer that doesn't have a buffer-local setting of that variable), we
> could attack the problem in a different way:
> 
> 1- change `case-fold-search` to a plain DEFVAR_LISP instead of a PER_BUFFER.
> 2- change its `let-binding` rule so it automatically becomes buffer-local
>    within the `let` (i.e. the let-binding stops affecting other buffers).

If there's a small number of such variables, maybe we should do the
same with all of them, and otherwise leave the current implementation
of buffer-local builtins as it is now?

Re: [PATCH v2 01/16] Add a test for let-binding unwinding

[Eli Zaretskii]

> From: Spencer Baugh <sbaugh@catern.com>
> Date: Sat, 21 Nov 2020 21:34:30 -0500
> Cc: Spencer Baugh <sbaugh@catern.com>, Arnold Noronha <arnold@tdrhq.com>,
>  Stefan Monnier <monnier@iro.umontreal.ca>, Dmitry Gutov <dgutov@yandex.ru>
> 
> Bindings in other buffers are not un-set when we unwind a let-binding
> which set the default value.  There doesn't seem to be an existing
> test which covers this, so here's one.

This is okay, but please accompany the change with a ChangeLog-style
commit log message (see CONTRIBUTE for details and the existing log
messages for examples).

Thanks.

Re: [PATCH v2 02/16] Assert not local-variable-p after setq in let_default binding

[Eli Zaretskii]

> From: Spencer Baugh <sbaugh@catern.com>
> Date: Sat, 21 Nov 2020 21:34:31 -0500
> Cc: Spencer Baugh <sbaugh@catern.com>, Arnold Noronha <arnold@tdrhq.com>,
>  Stefan Monnier <monnier@iro.umontreal.ca>, Dmitry Gutov <dgutov@yandex.ru>
> 
> Breaking this is a likely way to break this test, so this saves a bit
> of time in debugging.

This is OK, but here, too, please add the log message.

Re: [PATCH v2 03/16] Stop checking the constant default for enable_multibyte_characters

[Eli Zaretskii]

> From: Spencer Baugh <sbaugh@catern.com>
> Date: Sat, 21 Nov 2020 21:34:32 -0500
> Cc: Spencer Baugh <sbaugh@catern.com>, Arnold Noronha <arnold@tdrhq.com>,
>  Stefan Monnier <monnier@iro.umontreal.ca>, Dmitry Gutov <dgutov@yandex.ru>
> 
> The default is a constant "t", and can't be changed. So we don't need
> to check it.

This changeset is unrelated to the main issue.  It might be a
prerequisite for applying the main changeset, but it can also be
applied if the main changeset is not installed.  Right?  If so, I
don't object, and in general the parts of your patch series that can
stand on their own right should be so identified, so that we install
them separately.

> @@ -2510,8 +2509,6 @@ usage:  (make-pipe-process &rest ARGS)  */)
>        }
>      else if (!NILP (Vcoding_system_for_write))
>        val = Vcoding_system_for_write;
> -    else if (NILP (BVAR (current_buffer, enable_multibyte_characters)))
> -      val = Qnil;

Hmm... not sure I understand why you suggest to remove this.  It
doesn't refer to buffer_defaults.

Again, the log message is missing, but otherwise okay.

Thanks.

Re: [PATCH v2 04/16] Take buffer field name in DEFVAR_PER_BUFFER

[Eli Zaretskii]

> From: Spencer Baugh <sbaugh@catern.com>
> Date: Sat, 21 Nov 2020 21:34:33 -0500
> Cc: Spencer Baugh <sbaugh@catern.com>, Arnold Noronha <arnold@tdrhq.com>,
>  Stefan Monnier <monnier@iro.umontreal.ca>, Dmitry Gutov <dgutov@yandex.ru>
> 
> This removes some unnecessary usage of BVAR which would need to be
> changed later anyway.
> 
> Also, this makes the comment above the define of DEFVAR_PER_BUFFER
> correct - it already says that vname is the name of the buffer slot.

OK for this part (but log message is missing).

Re: [PATCH v2 05/16] Add BVAR_DEFAULT for access to buffer defaults

[Eli Zaretskii]

> From: Spencer Baugh <sbaugh@catern.com>
> Date: Sat, 21 Nov 2020 21:34:34 -0500
> Cc: Spencer Baugh <sbaugh@catern.com>, Arnold Noronha <arnold@tdrhq.com>,
>  Stefan Monnier <monnier@iro.umontreal.ca>, Dmitry Gutov <dgutov@yandex.ru>
> 
> This is more direct. This macro can stay as an lvalue even when we
> change BVAR to be a function call - directly changing and accessing
> buffer_defaults is always fine.

I think it will be confusing to have BVAR that cannot be an lvalue,
and BVAR_DEFAULT that can.  It will lead to mistakes and frustration.
If we want to be able to assign to Vstandard_category_table etc.,
let's invent some internal way of doing that, but without exposing a
macro whose name is similar to BVAR.

(Log message is missing.)

Thanks.

Re: [PATCH v2 06/16] Use bset_* functions instead of BVAR

[Eli Zaretskii]

> From: Spencer Baugh <sbaugh@catern.com>
> Date: Sat, 21 Nov 2020 21:34:35 -0500
> Cc: Spencer Baugh <sbaugh@catern.com>, Arnold Noronha <arnold@tdrhq.com>,
>  Stefan Monnier <monnier@iro.umontreal.ca>, Dmitry Gutov <dgutov@yandex.ru>
> 
> We move bset_save_length to buffer.h, and expand usage of the
> pre-existing bset_last_selected_window and
> bset_enable_multibyte_characters functions.
> 
> This removes some usage of BVAR as an lvalue, necessary for changing
> BVAR into a function call.

This is problematic.  Inline functions are only fast in optimized
builds, so this punishes people who run unoptimized debug builds,
which is true for many developers.  I'd prefer to have a macro for
that, okay?  And we don't actually need a separate macro for each
variable, right?

Thanks.

Re: [PATCH v2 07/16] Take offset not idx in PER_BUFFER_VALUE_P

[Eli Zaretskii]

> From: Spencer Baugh <sbaugh@catern.com>
> Date: Sat, 21 Nov 2020 21:34:36 -0500
> Cc: Spencer Baugh <sbaugh@catern.com>, Arnold Noronha <arnold@tdrhq.com>,
>  Stefan Monnier <monnier@iro.umontreal.ca>, Dmitry Gutov <dgutov@yandex.ru>
> 
> This also moves the common "idx == -1" check into PER_BUFFER_VALUE_P.
> If idx == -1, it's guaranteed that the corresponding buffer field has
> a per-buffer value.  We do this check everywhere we call
> PER_BUFFER_VALUE_P, so let's put it in the common code.

But the comment says:

   If a slot in this structure is -1, then even though there may
   be a DEFVAR_PER_BUFFER for the slot, there is no default value for it;
   and the corresponding slot in buffer_defaults is not used.

Doesn't this contradict your assumption, even though the existing uses
of the macro don't?

> This is motivated by later commits which will get rid of idx.

Why do something with idx if we will get rid of it?  I don't see any
purpose in such incremental changes: we will install the changes
regarding buffer-local values as a single changeset, so keeping the
code working after each separate patch in the series is not needed,
and just makes the changes unnecessarily larger.

IOW, let's consider the change which gets rid of idx without this
intermediate step.

> +/* Value is true if the variable with offset OFFSET has a local value
> +   in buffer B.  */
> +
> +INLINE bool
> +PER_BUFFER_VALUE_P (struct buffer *b, int offset)
> +{
> +  int idx = PER_BUFFER_IDX (offset);
> +  eassert (idx == -1 || valid_per_buffer_idx (idx));
> +  return idx == -1 || b->local_flags[idx];
> +}
> +
[...]
> @@ -1440,8 +1440,8 @@ set_internal (Lisp_Object symbol, Lisp_Object newval, Lisp_Object where,
>  	  {
>  	    int offset = XBUFFER_OBJFWD (innercontents)->offset;
>  	    int idx = PER_BUFFER_IDX (offset);
> -	    if (idx > 0 && bindflag == SET_INTERNAL_SET
> -	        && !PER_BUFFER_VALUE_P (buf, idx))
> +	    if (bindflag == SET_INTERNAL_SET
> +	        && !PER_BUFFER_VALUE_P (buf, offset))

The original code tests idx for a positive value, but your modified
PER_BUFFER_VALUE_P macro will only check that it's non-negative.
That's not the same thing.

Re: [PATCH v2 08/16] Combine unnecessarily separate loops in buffer.c

[Eli Zaretskii]

> From: Spencer Baugh <sbaugh@catern.com>
> Date: Sat, 21 Nov 2020 21:34:37 -0500
> Cc: Spencer Baugh <sbaugh@catern.com>, Arnold Noronha <arnold@tdrhq.com>,
>  Stefan Monnier <monnier@iro.umontreal.ca>, Dmitry Gutov <dgutov@yandex.ru>
> 
> These loops iterate over the same things with the same check.

This is again a cleanup not directly related to the main issue.  The
change is okay (modulo the log message), but let's not mix it with the
main changeset.

Re: [PATCH v2 09/16] Add and use BUFFER_DEFAULT_VALUE_P

[Eli Zaretskii]

> From: Spencer Baugh <sbaugh@catern.com>
> Date: Sat, 21 Nov 2020 21:34:38 -0500
> Cc: Spencer Baugh <sbaugh@catern.com>, Arnold Noronha <arnold@tdrhq.com>,
>  Stefan Monnier <monnier@iro.umontreal.ca>, Dmitry Gutov <dgutov@yandex.ru>
> 
> This makes the code more clear, IMO.  And this gets rid of some more
> usage of idx, which we'll remove later.

Once again, let's not make intermediate changes for something that
will be changed by later patches in the series.

I'm not sure which parts are about making the code more clear, but
let's not mix them with the main changes; instead, let's have a
separate series for cleaning up the existing code without significant
changes.

Thanks.

Re: [PATCH v2 10/16] Add and use KILL_PER_BUFFER_VALUE

[Eli Zaretskii]

> From: Spencer Baugh <sbaugh@catern.com>
> Date: Sat, 21 Nov 2020 21:34:39 -0500
> Cc: Spencer Baugh <sbaugh@catern.com>, Arnold Noronha <arnold@tdrhq.com>,
>  Stefan Monnier <monnier@iro.umontreal.ca>, Dmitry Gutov <dgutov@yandex.ru>
> 
> This improves clarity and removes usage of idx.

Same comment as earlier regarding changes related to idx.

As for clarity: you introduced an inline function, which will make the
code slower in unoptimized builds.  If you think the benefits from
bringing these two operations together will benefit readability, let's
make that a macro, not a function.

Thanks.

Re: [PATCH v2 11/16] Assert that PER_BUFFER_IDX for Lisp variables is not 0

[Eli Zaretskii]

> From: Spencer Baugh <sbaugh@catern.com>
> Date: Sat, 21 Nov 2020 21:34:40 -0500
> Cc: Spencer Baugh <sbaugh@catern.com>, Arnold Noronha <arnold@tdrhq.com>,
>  Stefan Monnier <monnier@iro.umontreal.ca>, Dmitry Gutov <dgutov@yandex.ru>
> 
> PER_BUFFER_IDX can't be 0 for Lisp variables - so this if-check was
> always pointless.

This is again a cleanup not directly related to the main issue.

>  	if (BUFFER_OBJFWDP (valcontents))
>  	  {
>  	    int offset = XBUFFER_OBJFWD (valcontents)->offset;
> -	    if (PER_BUFFER_IDX (offset) != 0)
> -	      return per_buffer_default (offset);
> +	    eassert (PER_BUFFER_IDX (offset) != 0);
> +            return per_buffer_default (offset);

Btw, it looks like your setting of indent-tabs-mode is different from
the default, because above you change the indentation of the 'return'
line to be all-spaces.  This is not our style, we use non-nil
indent-tabs-mode in C sources (but not in Lisp, see .dir-locals.el).

Re: [PATCH v2 12/16] Rearrange set_internal for buffer forwarded symbols

[Eli Zaretskii]

> From: Spencer Baugh <sbaugh@catern.com>
> Date: Sat, 21 Nov 2020 21:34:41 -0500
> Cc: Spencer Baugh <sbaugh@catern.com>, Arnold Noronha <arnold@tdrhq.com>,
>  Stefan Monnier <monnier@iro.umontreal.ca>, Dmitry Gutov <dgutov@yandex.ru>
> 
> Previously, we unnecessarily called store_symval_forwarding even after
> calling set_default_internal.  store_symval_forwarding would have no
> additional effect after a call to set_default_internal.

Hmm... I don't think I follow.  can you explain why
store_symval_forwarding would have no effect in that case?

> Now, we don't call store_symval_forwarding if we've called
> set_default_internal.  As a consequence, we can also move the call to
> SET_PER_BUFFER_VALUE_P into store_symval_forwarding, where it's closer
> to the per-buffer-value actually being set, which improves clarity.
> 
> We'll later move the effect of SET_PER_BUFFER_VALUE_P into
> set_per_buffer_value, so they need to be together.

Once again, if this is some preparation for a next patch in the
series, let's make all those changes in one go, skipping any
intermediate steps; they are just a distraction.

Given these comments, I'm not sure I understand whether this patch is
part of the main change or some cleanup (or a mix of them).  Please
clarify (and separate the cleanup if needed).

Re: [PATCH v2 13/16] Get rid of buffer_permanent_local_flags array

[Eli Zaretskii]

> From: Spencer Baugh <sbaugh@catern.com>
> Date: Sat, 21 Nov 2020 21:34:42 -0500
> Cc: Spencer Baugh <sbaugh@catern.com>, Arnold Noronha <arnold@tdrhq.com>,
>  Stefan Monnier <monnier@iro.umontreal.ca>, Dmitry Gutov <dgutov@yandex.ru>
> 
> This is only used in one place, for two variables; we can just special
> case those variables in the place it's used.

Is this necessary for the speedup changes?  It sounds like this is not
directly related, and if so, I'd like to leave this issue alone for
now.  One serious change at a time.

>    /* For each slot that has a default value, copy that into the slot.  */
>    FOR_EACH_PER_BUFFER_OBJECT_AT (offset)
>      {
> -      int idx = PER_BUFFER_IDX (offset);
>        if ((BUFFER_DEFAULT_VALUE_P (offset)
> -	   && (permanent_too
> -	       || buffer_permanent_local_flags[idx] == 0)))
> +           && (permanent_too
> +               /* Special case these two for backwards-compat; they're
> +                  flagged as permanent-locals in bindings.el, even
> +                  though they do have default values. */
> +               || (offset != PER_BUFFER_VAR_OFFSET (truncate_lines)
> +                   && offset !=
> +                   PER_BUFFER_VAR_OFFSET (buffer_file_coding_system)))))
>          KILL_PER_BUFFER_VALUE (b, offset);

In any case, I don't think we want to special-case some buffer-local
variables in this way.  It is a maintenance burden, if nothing else.

Thanks.

Re: [PATCH v2 14/16] Remove unnecessary Qunbound check

[Eli Zaretskii]

> From: Spencer Baugh <sbaugh@catern.com>
> Date: Sat, 21 Nov 2020 21:34:43 -0500
> Cc: Spencer Baugh <sbaugh@catern.com>, Arnold Noronha <arnold@tdrhq.com>,
>  Stefan Monnier <monnier@iro.umontreal.ca>, Dmitry Gutov <dgutov@yandex.ru>
> 
> This can never be the case.

It can't? why?

In any case, this also seems an unrelated cleanup, isn't it?

Re: [PATCH v2 15/16] Remove local_flags array in struct buffer

[Eli Zaretskii]

> From: Spencer Baugh <sbaugh@catern.com>
> Date: Sat, 21 Nov 2020 21:34:44 -0500
> Cc: Spencer Baugh <sbaugh@catern.com>, Arnold Noronha <arnold@tdrhq.com>,
>  Stefan Monnier <monnier@iro.umontreal.ca>, Dmitry Gutov <dgutov@yandex.ru>
> 
> +#define BVAR_DIRECT(buf, field) ((buf)->field ## _)

I think I'd prefer to call this BVAL instead.  Or maybe leave this
BVAR and rename the getter to something else.

> @@ -963,8 +958,6 @@ reset_buffer (register struct buffer *b)
>    bset_display_count (b, make_fixnum (0));
>    bset_display_time (b, Qnil);
>    bset_enable_multibyte_characters (b, Qt);
> -  bset_cursor_type (b, BVAR (&buffer_defaults, cursor_type));
> -  bset_extra_line_spacing (b, BVAR (&buffer_defaults, extra_line_spacing));

Why are we removing these two?

> @@ -1238,7 +1231,7 @@ buffer_local_value (Lisp_Object variable, Lisp_Object buffer)
>        {
>  	lispfwd fwd = SYMBOL_FWD (sym);
>  	if (BUFFER_OBJFWDP (fwd))
> -	  result = per_buffer_value (buf, XBUFFER_OBJFWD (fwd)->offset);
> +          result = bvar_get (buf, XBUFFER_OBJFWD (fwd)->offset);

Indentation whitespace changes again.

> +INLINE Lisp_Object
> +bvar_get (struct buffer *b, ptrdiff_t offset)
> +{
> +  Lisp_Object val = per_buffer_value (b, offset);
> +  return EQ (val, Qunbound)
> +    ? per_buffer_default (offset)
> +    : val;
>  }

Ouch! an inline function that calls another inline function, instead
of a simple pointer dereference.  Now I'm _really_ worried about the
effect on performance.

Can we make this faster, especially in unoptimized builds?

Re: [PATCH v2 16/16] Remove usage of buffer_local_flags

[Eli Zaretskii]

> From: Spencer Baugh <sbaugh@catern.com>
> Date: Sat, 21 Nov 2020 21:34:45 -0500
> Cc: Spencer Baugh <sbaugh@catern.com>, Arnold Noronha <arnold@tdrhq.com>,
>  Stefan Monnier <monnier@iro.umontreal.ca>, Dmitry Gutov <dgutov@yandex.ru>
> 
> Previously, we used the buffer_local_flags structure to track which
> fields in buffer_defaults contain usable defaults, as well as the
> index into local_flags to use for each struct buffer field.
> 
> After the immediate previous commit, we don't need the index into
> local_flags anymore.  So all we need is to know whether the fields in
> buffer_defaults contain usable defaults.  We can do that by just
> setting unusable and unused buffer_defaults fields to Qunbound. Then
> we can delete buffer_local_flags.
> 
> In these changes, we delete buffer_local_flags and ensure that
> buffer_defaults is initialized appropriately for this new usage.

Looks like some of the previous patches made changes in parts that are
being deleted here.  Let's skip such unnecessary intermediate steps,
okay?

> +   Slots in this structure which are set to Qunbound are permanently
> +   buffer-local. */

I thought the value of _any_ buffer-local variable starts Qunbound,
no?  Why only permanently buffer-local ones?

> +  int offset;
> +  FOR_EACH_PER_BUFFER_OBJECT_AT (offset)
> +    {
> +      /* These are initialized before us. */
> +      if (!(offset == PER_BUFFER_VAR_OFFSET (syntax_table)
> +            || offset == PER_BUFFER_VAR_OFFSET (category_table)))
> +        set_per_buffer_default (offset, Qunbound);
> +    }
> +  set_per_buffer_default (PER_BUFFER_VAR_OFFSET (undo_list), Qunbound);

This again special-cases 2 variables, something I'd like to avoid.

> -  bset_undo_list (&buffer_defaults, Qnil);
> -  bset_mark_active (&buffer_defaults, Qnil);
> -  bset_file_format (&buffer_defaults, Qnil);
> -  bset_auto_save_file_format (&buffer_defaults, Qt);
> -  set_buffer_overlays_before (&buffer_defaults, NULL);
> -  set_buffer_overlays_after (&buffer_defaults, NULL);
> -  buffer_defaults.overlay_center = BEG;

Why are these being removed?

> -  bset_enable_multibyte_characters (&buffer_defaults, Qt);
>    bset_buffer_file_coding_system (&buffer_defaults, Qnil);
>    XSETFASTINT (BVAR_DIRECT (&buffer_defaults, fill_column), 70);
>    XSETFASTINT (BVAR_DIRECT (&buffer_defaults, left_margin), 0);
>    bset_cache_long_scans (&buffer_defaults, Qt);
> -  bset_file_truename (&buffer_defaults, Qnil);

And these?

> -  bset_display_time (&buffer_defaults, Qnil);

And this one?

> @@ -1478,7 +1426,7 @@ set_per_buffer_value (struct buffer *b, int offset, Lisp_Object value)
>  INLINE bool
>  BUFFER_DEFAULT_VALUE_P (int offset)
>  {
> -  return PER_BUFFER_IDX (offset) > 0;
> +  return !EQ (per_buffer_default (offset), Qunbound);

Another function call where previously we just called a simple macro.

Thanks.

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

[Eli Zaretskii]

> Date: Mon, 30 Nov 2020 20:32:47 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: arnold@tdrhq.com, dgutov@yandex.ru, monnier@iro.umontreal.ca, rms@gnu.org,
>  emacs-devel@gnu.org
> 
> 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

Now done.

To summarize:

  . I had some minor comments about the code and some questions
  . Can we please make the modified code faster by using macros
    instead of inline functions?
  . Please separate the series into a single patch that is needed to
    speed up let-binding of variables (skipping any intermediate
    steps), and several other patches for cleanups and simplifications
    not directly related to the speedup.
  . Please add log messages for each patch.

Last, but not least, I'd be happy if we could find a way of
benchmarking the change, to be sure we are not paying too heavy a
price for this.

Thanks!