[PATCH 0/7] Cleanups and tests for DEFVAR_PER_BUFFER variables

[PATCH 0/7] Cleanups and tests for DEFVAR_PER_BUFFER variables

[Spencer Baugh]

Hi emacs-devel,

These are some cleanups and tests for DEFVAR_PER_BUFFER variables.
I think they are all uncontroversial.

These are extracted from the patch series I sent some months ago
(Subject: Speeding up DEFVAR_PER_BUFFER).  Specifically, these are the
patches that were approved as fine independent of the rest of the
series.  And they have changelog entries in the commit messages now :)

These are pre-requisites for my DEFVAR_PER_BUFFER improvements (which
I'll be resending soon), so it would be nice to apply them ahead of
that.

Spencer Baugh (7):
  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
  Combine unnecessarily separate loops in buffer.c
  Assert that PER_BUFFER_IDX for Lisp variables is not 0
  Remove unnecessary Qunbound check

 src/buffer.c           | 147 ++++++++++++++++++++---------------------
 src/data.c             |   4 +-
 src/print.c            |   6 +-
 src/process.c          |  15 ++---
 test/src/data-tests.el |  23 +++++++
 5 files changed, 101 insertions(+), 94 deletions(-)

-- 
2.28.0

[PATCH 1/7] 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 (data-tests--let-buffer-local-no-unwind-other-buffers):
  Add test for let-binding unwinding behavior
---
 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 03d867f18a..d0cb87293f 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 2/7] 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 (data-tests--let-buffer-local):
Add assertion to test.
---
 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 d0cb87293f..b1e5fa0767 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 3/7] 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 (reset_buffer, init_buffer):
* src/print.c (print_string, temp_output_buffer_setup):
* src/process.c (Fmake_pipe_process, Fmake_serial_process)
(set_network_socket_coding_system):
Don't check buffer_defaults for enable_multibyte_characters
---
 src/buffer.c  |  7 ++-----
 src/print.c   |  6 ++----
 src/process.c | 15 ++++-----------
 3 files changed, 8 insertions(+), 20 deletions(-)

diff --git a/src/buffer.c b/src/buffer.c
index 8e33162989..9d87803cc6 100644
--- a/src/buffer.c
+++ b/src/buffer.c
@@ -988,8 +988,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));
 
@@ -5405,9 +5404,7 @@ init_buffer (void)
 #endif /* USE_MMAP_FOR_BUFFERS */
 
   AUTO_STRING (scratch, "*scratch*");
-  Fset_buffer (Fget_buffer_create (scratch, Qnil));
-  if (NILP (BVAR (&buffer_defaults, enable_multibyte_characters)))
-    Fset_buffer_multibyte (Qnil);
+  Fset_buffer (Fget_buffer_create (scratch));
 
   char const *pwd = emacs_wd;
 
diff --git a/src/print.c b/src/print.c
index 14af919547..0f1987c405 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 b98bc297a3..c084dca4d0 100644
--- a/src/process.c
+++ b/src/process.c
@@ -2399,8 +2399,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
@@ -2425,8 +2424,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))
@@ -3124,8 +3121,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);
 
@@ -3138,8 +3134,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);
 
@@ -3181,9 +3176,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 4/7] 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 (DEFVAR_PER_BUFFER): Take name of field as argument
(syms_of_buffer): Pass name of field as argument
---
 src/buffer.c | 126 +++++++++++++++++++++++++--------------------------
 1 file changed, 62 insertions(+), 64 deletions(-)

diff --git a/src/buffer.c b/src/buffer.c
index 9d87803cc6..de3b202ebc 100644
--- a/src/buffer.c
+++ b/src/buffer.c
@@ -5461,18 +5461,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;
@@ -5537,20 +5535,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.
 
@@ -5616,7 +5614,7 @@ 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.
@@ -5624,30 +5622,30 @@ A value of nil means to use the current buffer's major mode, provided
 it is not marked as "special".  */);
 
   DEFVAR_PER_BUFFER ("local-minor-modes",
-		     &BVAR (current_buffer, local_minor_modes),
+		     local_minor_modes,
 		     Qnil,
 		     doc: /* Minor modes currently active in the current buffer.
 This is a list of symbols, or nil if there are no minor modes active.  */);
 
-  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',
@@ -5655,26 +5653,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,
@@ -5688,7 +5686,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'.
@@ -5706,7 +5704,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.
@@ -5714,7 +5712,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
@@ -5736,7 +5734,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.
@@ -5757,7 +5755,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
@@ -5768,7 +5766,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.
 
@@ -5778,7 +5776,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.
@@ -5796,14 +5794,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
@@ -5811,31 +5809,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.
@@ -5845,7 +5843,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.
 
@@ -5858,11 +5856,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',
@@ -5872,7 +5870,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.
 
@@ -5909,7 +5907,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.
@@ -5917,7 +5915,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.
@@ -5925,7 +5923,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.
@@ -5934,7 +5932,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.
@@ -5943,7 +5941,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.
@@ -5951,17 +5949,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;
@@ -5971,7 +5969,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;
@@ -5983,13 +5981,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
@@ -6014,7 +6012,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
@@ -6033,7 +6031,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
@@ -6048,7 +6046,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,
@@ -6061,7 +6059,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,
@@ -6112,7 +6110,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.
 
@@ -6158,10 +6156,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.
@@ -6197,23 +6195,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.
@@ -6227,12 +6225,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'.
@@ -6268,7 +6266,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:
 
@@ -6292,7 +6290,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'.
@@ -6300,7 +6298,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 5/7] Combine unnecessarily separate loops in buffer.c

[Spencer Baugh]

These loops iterate over the same things with the same check.

* src/buffer.c (reset_buffer_local_variables): Combine loops
---
 src/buffer.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/src/buffer.c b/src/buffer.c
index de3b202ebc..563e5b7180 100644
--- a/src/buffer.c
+++ b/src/buffer.c
@@ -1006,7 +1006,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.
@@ -1100,10 +1100,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)
     {
@@ -1111,7 +1107,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 6/7] 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 (default_value): Change if to eassert
---
 src/data.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/data.c b/src/data.c
index 0fa491b17a..2de76099ca 100644
--- a/src/data.c
+++ b/src/data.c
@@ -1709,8 +1709,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 7/7] Remove unnecessary Qunbound check

[Spencer Baugh]

DEFVAR_PER_BUFFER variables (which this function deals with) cannot be
Qunbound, at least right now; they're either set to a value, or
PER_BUFFER_VALUE_P is false.

* src/buffer.c (buffer_local_variables_1): Remove Qunbound check
---
 src/buffer.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/src/buffer.c b/src/buffer.c
index 563e5b7180..a1555c729c 100644
--- a/src/buffer.c
+++ b/src/buffer.c
@@ -1323,8 +1323,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

Re: [PATCH 0/7] Cleanups and tests for DEFVAR_PER_BUFFER variables

[Eli Zaretskii]

> From: Spencer Baugh <sbaugh@catern.com>
> Date: Tue, 23 Mar 2021 23:11:50 -0400
> Cc: Spencer Baugh <sbaugh@catern.com>, Eli Zaretskii <eliz@gnu.org>,
>  Stefan Monnier <monnier@iro.umontreal.ca>
> 
> Hi emacs-devel,

Actually, it is best to send this (and patches in general) to
bug-gnu-emacs@gnu.org, so that the whole discussion is recorded by our
issue tracker.

> These are some cleanups and tests for DEFVAR_PER_BUFFER variables.
> I think they are all uncontroversial.
> 
> These are extracted from the patch series I sent some months ago
> (Subject: Speeding up DEFVAR_PER_BUFFER).  Specifically, these are the
> patches that were approved as fine independent of the rest of the
> series.  And they have changelog entries in the commit messages now :)
> 
> These are pre-requisites for my DEFVAR_PER_BUFFER improvements (which
> I'll be resending soon), so it would be nice to apply them ahead of
> that.

I'd prefer to pick up where we left off back then.  The main issue
left unresolved in the past discussion was the potential slowdown of
simple accesses to buffer-local vars due to your proposal.  Can we
please have benchmarks for that, so we could decide whether the
tradeoff is worth it?  It's quite possible that the performance
aspects could affect the code changes, so even uncontroversial
cleanups should perhaps wait until we have figured out the more
important aspects of these changesets.

Thanks.

Re: [PATCH 7/7] Remove unnecessary Qunbound check

[Stefan Monnier]

> DEFVAR_PER_BUFFER variables (which this function deals with) cannot be
> Qunbound, at least right now; they're either set to a value, or
> PER_BUFFER_VALUE_P is false.

Are you sure?

What if you do something like `(makunbound 'buffer-file-coding-system)`?

The other 6 patches look good to me.


        Stefan

Re: [PATCH 0/7] Cleanups and tests for DEFVAR_PER_BUFFER variables

[Stefan Monnier]

> I'd prefer to pick up where we left off back then.  The main issue
> left unresolved in the past discussion was the potential slowdown of
> simple accesses to buffer-local vars due to your proposal.  Can we
> please have benchmarks for that, so we could decide whether the
> tradeoff is worth it?  It's quite possible that the performance
> aspects could affect the code changes, so even uncontroversial
> cleanups should perhaps wait until we have figured out the more
> important aspects of these changesets.

I think those changes are good to go and fundamentally unrelated to
the discussion (except to the extent that they were found while
investigating that part of the code).

Of course, they may not be the final word, but I'd rather install those
now so they don't get lost if the rest of the discussion ends up not
going anywhere and also so the rest of the discussion doesn't need to be
using patches relative to code that's not in `master`.


        Stefan

Re: [PATCH 0/7] Cleanups and tests for DEFVAR_PER_BUFFER variables

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Spencer Baugh <sbaugh@catern.com>,  emacs-devel@gnu.org
> Date: Thu, 25 Mar 2021 12:48:53 -0400
> 
> > I'd prefer to pick up where we left off back then.  The main issue
> > left unresolved in the past discussion was the potential slowdown of
> > simple accesses to buffer-local vars due to your proposal.  Can we
> > please have benchmarks for that, so we could decide whether the
> > tradeoff is worth it?  It's quite possible that the performance
> > aspects could affect the code changes, so even uncontroversial
> > cleanups should perhaps wait until we have figured out the more
> > important aspects of these changesets.
> 
> I think those changes are good to go and fundamentally unrelated to
> the discussion (except to the extent that they were found while
> investigating that part of the code).
> 
> Of course, they may not be the final word, but I'd rather install those
> now so they don't get lost if the rest of the discussion ends up not
> going anywhere and also so the rest of the discussion doesn't need to be
> using patches relative to code that's not in `master`.

Sorry, no.  I don't want to make minor cleanups unless they are
related to a useful development.  Those cleanups are more trouble than
they are worth (excluding the additions to the test suite, of course).
Nothing of value will be lost if they are lost.

Re: [PATCH 0/7] Cleanups and tests for DEFVAR_PER_BUFFER variables

[Spencer Baugh]

Eli Zaretskii <eliz@gnu.org> writes:
> I'd prefer to pick up where we left off back then.  The main issue
> left unresolved in the past discussion was the potential slowdown of
> simple accesses to buffer-local vars due to your proposal.  Can we
> please have benchmarks for that, so we could decide whether the
> tradeoff is worth it?  It's quite possible that the performance
> aspects could affect the code changes, so even uncontroversial
> cleanups should perhaps wait until we have figured out the more
> important aspects of these changesets.

OK, I ran some basic benchmarking. Specifically, I ran

(defun shr-benchmark ()
  (let ((gc-cons-threshold 800000000))
    (message "shr-benchmark result: %s"
             (benchmark-run 100
               (dolist (file (directory-files (ert-resource-directory) nil "\\.html\\'"))
                 (shr-test (replace-regexp-in-string "\\.html\\'" "" file)))))))

out of shr-tests.el.

My results (each from running emacs -f shr-benchmark 10 times and
discarding the first 2 results):

| Unmodified Emacs                  | 2.265s |
| With original changes             | 2.295s |
| Avoid unnecessary Qunbound checks | 2.266s |

My original changes imposed about a 1-2% slowdown on an unmodified
Emacs.  That's unacceptable, so I made a further change to only check
Qunbound for vars where it was actually necessary.  That makes the
performance comparable to an unmodified Emacs, so I think in terms of
performance that should put this in a good state.

To statically avoid checking Qunbound for vars where it's not necessary,
we need to use separate syntax for accessing BVARs that have defaults
and BVARs that don't have defaults. Only BVARs that have defaults need a
Qunbound check.  (We could use the same syntax for both, but it would
require X macros which Eli disliked earlier)

I see three general options for the syntax for accessing each kind of
var:

|            | Var with default             | Var without default      |
|------------+------------------------------+--------------------------|
| Status quo | BVAR (buf, field)            | BVAR (buf, field)        |
| Option 1   | BVAR (buf, field)            | buf->field               |
| Option 2   | BVAR (buf, field)            | BVAR_DIRECT (buf, field) |
| Option 3   | BVAR_OR_DEFAULT (buf, field) | BVAR (buf, field)        |

(The specific names for BVAR_DIRECT or BVAR_OR_DEFAULT don't matter,
we can make them shorter if we want).

What do people prefer?

I went with option 3 for the large refactoring, but if it's OK in
terms of coding style I think option 1 is nicer.

In any case we should ensure that we can't accidentally, for example,
use BVAR_DIRECT on a field that actually has a default.  This should
be easy enough by just naming the fields slightly differently.

I'll post my updated patches after I know which syntax people would
prefer. (The change compared to the earlier patches is pretty small,
besides adjusting all the BVAR usages to use the new syntax)

Re: [PATCH 0/7] Cleanups and tests for DEFVAR_PER_BUFFER variables

[Eli Zaretskii]

> From: Spencer Baugh <sbaugh@catern.com>
> Cc: emacs-devel@gnu.org, monnier@iro.umontreal.ca
> Date: Thu, 01 Apr 2021 14:13:50 -0400
> 
> OK, I ran some basic benchmarking. Specifically, I ran
> 
> (defun shr-benchmark ()
>   (let ((gc-cons-threshold 800000000))
>     (message "shr-benchmark result: %s"
>              (benchmark-run 100
>                (dolist (file (directory-files (ert-resource-directory) nil "\\.html\\'"))
>                  (shr-test (replace-regexp-in-string "\\.html\\'" "" file)))))))
> 
> out of shr-tests.el.
> 
> My results (each from running emacs -f shr-benchmark 10 times and
> discarding the first 2 results):
> 
> | Unmodified Emacs                  | 2.265s |
> | With original changes             | 2.295s |
> | Avoid unnecessary Qunbound checks | 2.266s |

Thanks, that's encouraging.  But we need a few more benchmarks, I
think.  One of them should be for redisplay, as it's a
performance-critical part of Emacs, and it accesses buffer-local
variables quite a lot.  So something like a benchmark of scrolling
through xdisp.c one line at a time in an interactive session would
probably give us an idea.

Another benchmark we frequently use is to remove all *.elc files and
then time "make" which byte-compiles them all.

> |            | Var with default             | Var without default      |
> |------------+------------------------------+--------------------------|
> | Status quo | BVAR (buf, field)            | BVAR (buf, field)        |
> | Option 1   | BVAR (buf, field)            | buf->field               |
> | Option 2   | BVAR (buf, field)            | BVAR_DIRECT (buf, field) |
> | Option 3   | BVAR_OR_DEFAULT (buf, field) | BVAR (buf, field)        |
> 
> (The specific names for BVAR_DIRECT or BVAR_OR_DEFAULT don't matter,
> we can make them shorter if we want).
> 
> What do people prefer?

Either 2 or 3 (perhaps with some slightly different names: maybe none
of the new macros should inherit the old name?).

> In any case we should ensure that we can't accidentally, for example,
> use BVAR_DIRECT on a field that actually has a default.  This should
> be easy enough by just naming the fields slightly differently.

Not sure what you mean by naming differently, but yes, we need to
detect incorrect usage at compile time, at least in the build with
"--enable-checking".

Thanks.

Re: [PATCH 0/7] Cleanups and tests for DEFVAR_PER_BUFFER variables

[Stefan Monnier]

> To statically avoid checking Qunbound for vars where it's not necessary,
> we need to use separate syntax for accessing BVARs that have defaults
> and BVARs that don't have defaults. Only BVARs that have defaults need a
> Qunbound check.  (We could use the same syntax for both, but it would
> require X macros which Eli disliked earlier)

BTW, we may also want to try and increase the proportion of those vars
which don't have a global default (i.e. look at which variables fall
into this camp and that can be changed without too much trouble).

Regarding benchmarking, you may also want to try `elisp-benchmarks` in
GNU ELPA (I suspect those benchmarks will not be very much influenced
by your patch, tho, so please try the "scroll xdisp.c" first).


        Stefan

Re: [PATCH 0/7] Cleanups and tests for DEFVAR_PER_BUFFER variables

[Spencer Baugh]

Stefan Monnier <monnier@iro.umontreal.ca> writes:
>> To statically avoid checking Qunbound for vars where it's not necessary,
>> we need to use separate syntax for accessing BVARs that have defaults
>> and BVARs that don't have defaults. Only BVARs that have defaults need a
>> Qunbound check.  (We could use the same syntax for both, but it would
>> require X macros which Eli disliked earlier)
>
> BTW, we may also want to try and increase the proportion of those vars
> which don't have a global default (i.e. look at which variables fall
> into this camp and that can be changed without too much trouble).

For sure, that has many benefits.

I would have investigated this, but I don't know to what extent we try
to provide backwards-compatibility for this kind of thing.  If it
doesn't break any of the tests, is it probably fine?  Or if we're
concerned about breaking third-party packages, how can one judge the
risk of that?

If we could get rid of defaults from most of the variables, we could
just convert the rest into regular buffer-local variables and then
remove support for defaults in DEFVAR_PER_BUFFER entirely, which would
be a big simplification (and performance improvement).  But of course
this depends on how many variables can have their defaults removed -
since converting a variable into a regular buffer-local variable will
make its performance worse for sure.

Re: [PATCH 0/7] Cleanups and tests for DEFVAR_PER_BUFFER variables

[Stefan Monnier]

>> BTW, we may also want to try and increase the proportion of those vars
>> which don't have a global default (i.e. look at which variables fall
>> into this camp and that can be changed without too much trouble).
> I would have investigated this,

I think we'd want to do things one step at a time, tho, so better
concentrate on what you have now.

> but I don't know to what extent we try to provide
> backwards-compatibility for this kind of thing.

We very much want to preserve backwards-compatibility.

> If it doesn't break any of the tests, is it probably fine?

No, that's definitely not sufficient.

> Or if we're concerned about breaking third-party packages, how can one
> judge the risk of that?

Searching through uses of the variable in the packages we can find (GNU
ELPA and MELPA are good starting points).  It's hard work and often
results in concluding that it's too risky.  Tho often those problems can
be seen much earlier, luckily.

Better is to find some way to make the change gradually or to first
install changes that detect when the code exploits the "to be old"
behavior and emit messages warning about such a use.

It's can be a long process.

> If we could get rid of defaults from most of the variables, we could
> just convert the rest into regular buffer-local variables and then
> remove support for defaults in DEFVAR_PER_BUFFER entirely, which would
> be a big simplification (and performance improvement).  But of course
> this depends on how many variables can have their defaults removed -
> since converting a variable into a regular buffer-local variable will
> make its performance worse for sure.

Indeed, that's also my hope.  I don't know how realistic it is, OTOH.


        Stefan

Re: [PATCH 0/7] Cleanups and tests for DEFVAR_PER_BUFFER variables

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Eli Zaretskii <eliz@gnu.org>,  emacs-devel@gnu.org
> Date: Thu, 01 Apr 2021 17:42:38 -0400
> 
> > To statically avoid checking Qunbound for vars where it's not necessary,
> > we need to use separate syntax for accessing BVARs that have defaults
> > and BVARs that don't have defaults. Only BVARs that have defaults need a
> > Qunbound check.  (We could use the same syntax for both, but it would
> > require X macros which Eli disliked earlier)
> 
> BTW, we may also want to try and increase the proportion of those vars
> which don't have a global default (i.e. look at which variables fall
> into this camp and that can be changed without too much trouble).

Let's make this a separate issue and separate patch series, please.

Re: [PATCH 0/7] Cleanups and tests for DEFVAR_PER_BUFFER variables

[Spencer Baugh]

Eli Zaretskii <eliz@gnu.org> writes:
> Thanks, that's encouraging.  But we need a few more benchmarks, I
> think.  One of them should be for redisplay, as it's a
> performance-critical part of Emacs, and it accesses buffer-local
> variables quite a lot.  So something like a benchmark of scrolling
> through xdisp.c one line at a time in an interactive session would
> probably give us an idea.

OK, I did that.  Specifically I ran:

(defun xdisp-bench ()
  (let ((gc-cons-threshold 8000000000))
    (print (number-to-string (car
      (benchmark-run 3 (with-temp-buffer 
        (switch-to-buffer (current-buffer))
        (let* ((height (window-total-height))
               (lines (+ 10 height)))
          (dotimes (i (* 2 height)) (insert "\nhello" (number-to-string i)))
          (dotimes (_ 5)
            (dotimes (_ lines) (scroll-down 1) (redisplay))
            (dotimes (_ lines) (scroll-up 1) (redisplay))))))
    )) #'external-debugging-output)
    (kill-emacs)))

My results (from running emacs -f xdisp-bench 10 times and discarding
the first 2 results, on X11 with GTK):

| Unmodified Emacs                                    | 6.529 |
| My changes                                          | 6.610 |
| My changes + no default for bidi_display_reordering | 6.582 |

About a 1-2% slowdown from my changes.

I tried removing the default for bidi_display_reordering; customizing
bidi-display-reording is explicitly unsupported anyway, so I think
changing its behavior is reasonable.

That improved performance but unfortunately not enough to fully match an
unmodified Emacs.  Not sure what else to do.

I could remove the defaults for more variables that are heavily used
in xdisp.c, but I guess that will be fairly difficult for most of
them.

Thoughts?

> Another benchmark we frequently use is to remove all *.elc files and
> then time "make" which byte-compiles them all.

I did this, seems fine:

| Unmodified Emacs | 38m2.042s  |
| With my changes  | 37m48.127s |

Re: [PATCH 0/7] Cleanups and tests for DEFVAR_PER_BUFFER variables

[Eli Zaretskii]

> From: Spencer Baugh <sbaugh@catern.com>
> Cc: emacs-devel@gnu.org, monnier@iro.umontreal.ca
> Date: Sat, 03 Apr 2021 16:53:42 -0400
> 
> | Unmodified Emacs                                    | 6.529 |
> | My changes                                          | 6.610 |
> | My changes + no default for bidi_display_reordering | 6.582 |
> 
> About a 1-2% slowdown from my changes.
> 
> I tried removing the default for bidi_display_reordering; customizing
> bidi-display-reording is explicitly unsupported anyway, so I think
> changing its behavior is reasonable.
> 
> That improved performance but unfortunately not enough to fully match an
> unmodified Emacs.  Not sure what else to do.

Thanks.

I'm okay with 1-2% slowdown in redisplay, and I don't want to remove
the default value from bidi-display-reordering.  Let's hear if someone
else feels that such slowdown in redisplay is a bother; if not, it
sounds like a good tradeoff.

If someone has other good benchmarks before we make the decision,
please speak up.