* [PATCH v2 01/16] Add a test for let-binding unwinding
2020-11-22 2:34 ` [PATCH v2 00/16] " Spencer Baugh
@ 2020-11-22 2:34 ` Spencer Baugh
2020-11-25 20:53 ` Stefan Monnier
2020-12-01 16:44 ` Eli Zaretskii
2020-11-22 2:34 ` [PATCH v2 02/16] Assert not local-variable-p after setq in let_default binding Spencer Baugh
` (16 subsequent siblings)
17 siblings, 2 replies; 80+ messages in thread
From: Spencer Baugh @ 2020-11-22 2:34 UTC (permalink / raw)
To: emacs-devel; +Cc: Spencer Baugh, Arnold Noronha, Stefan Monnier, Dmitry Gutov
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
^ permalink raw reply related [flat|nested] 80+ messages in thread* Re: [PATCH v2 01/16] Add a test for let-binding unwinding
2020-11-22 2:34 ` [PATCH v2 01/16] Add a test for let-binding unwinding Spencer Baugh
@ 2020-11-25 20:53 ` Stefan Monnier
2020-11-30 17:31 ` Spencer Baugh
2020-12-01 16:44 ` Eli Zaretskii
1 sibling, 1 reply; 80+ messages in thread
From: Stefan Monnier @ 2020-11-25 20:53 UTC (permalink / raw)
To: Spencer Baugh; +Cc: Arnold Noronha, Dmitry Gutov, emacs-devel
> 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
^ permalink raw reply [flat|nested] 80+ messages in thread* Re: [PATCH v2 01/16] Add a test for let-binding unwinding
2020-11-25 20:53 ` Stefan Monnier
@ 2020-11-30 17:31 ` Spencer Baugh
0 siblings, 0 replies; 80+ messages in thread
From: Spencer Baugh @ 2020-11-30 17:31 UTC (permalink / raw)
To: Stefan Monnier; +Cc: Arnold Noronha, Dmitry Gutov, emacs-devel
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
^ permalink raw reply related [flat|nested] 80+ messages in thread
* Re: [PATCH v2 01/16] Add a test for let-binding unwinding
2020-11-22 2:34 ` [PATCH v2 01/16] Add a test for let-binding unwinding Spencer Baugh
2020-11-25 20:53 ` Stefan Monnier
@ 2020-12-01 16:44 ` Eli Zaretskii
1 sibling, 0 replies; 80+ messages in thread
From: Eli Zaretskii @ 2020-12-01 16:44 UTC (permalink / raw)
To: Spencer Baugh; +Cc: sbaugh, arnold, dgutov, monnier, emacs-devel
> 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.
^ permalink raw reply [flat|nested] 80+ messages in thread
* [PATCH v2 02/16] Assert not local-variable-p after setq in let_default binding
2020-11-22 2:34 ` [PATCH v2 00/16] " Spencer Baugh
2020-11-22 2:34 ` [PATCH v2 01/16] Add a test for let-binding unwinding Spencer Baugh
@ 2020-11-22 2:34 ` Spencer Baugh
2020-11-25 20:54 ` Stefan Monnier
2020-12-01 16:45 ` Eli Zaretskii
2020-11-22 2:34 ` [PATCH v2 03/16] Stop checking the constant default for enable_multibyte_characters Spencer Baugh
` (15 subsequent siblings)
17 siblings, 2 replies; 80+ messages in thread
From: Spencer Baugh @ 2020-11-22 2:34 UTC (permalink / raw)
To: emacs-devel; +Cc: Spencer Baugh, Arnold Noronha, Stefan Monnier, Dmitry Gutov
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
^ permalink raw reply related [flat|nested] 80+ messages in thread* Re: [PATCH v2 02/16] Assert not local-variable-p after setq in let_default binding
2020-11-22 2:34 ` [PATCH v2 02/16] Assert not local-variable-p after setq in let_default binding Spencer Baugh
@ 2020-11-25 20:54 ` Stefan Monnier
2020-12-01 16:45 ` Eli Zaretskii
1 sibling, 0 replies; 80+ messages in thread
From: Stefan Monnier @ 2020-11-25 20:54 UTC (permalink / raw)
To: Spencer Baugh; +Cc: Arnold Noronha, Dmitry Gutov, emacs-devel
> 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
^ permalink raw reply [flat|nested] 80+ messages in thread* Re: [PATCH v2 02/16] Assert not local-variable-p after setq in let_default binding
2020-11-22 2:34 ` [PATCH v2 02/16] Assert not local-variable-p after setq in let_default binding Spencer Baugh
2020-11-25 20:54 ` Stefan Monnier
@ 2020-12-01 16:45 ` Eli Zaretskii
1 sibling, 0 replies; 80+ messages in thread
From: Eli Zaretskii @ 2020-12-01 16:45 UTC (permalink / raw)
To: Spencer Baugh; +Cc: sbaugh, arnold, dgutov, monnier, emacs-devel
> 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.
^ permalink raw reply [flat|nested] 80+ messages in thread
* [PATCH v2 03/16] Stop checking the constant default for enable_multibyte_characters
2020-11-22 2:34 ` [PATCH v2 00/16] " Spencer Baugh
2020-11-22 2:34 ` [PATCH v2 01/16] Add a test for let-binding unwinding Spencer Baugh
2020-11-22 2:34 ` [PATCH v2 02/16] Assert not local-variable-p after setq in let_default binding Spencer Baugh
@ 2020-11-22 2:34 ` Spencer Baugh
2020-11-25 20:57 ` Stefan Monnier
2020-12-01 16:52 ` Eli Zaretskii
2020-11-22 2:34 ` [PATCH v2 04/16] Take buffer field name in DEFVAR_PER_BUFFER Spencer Baugh
` (14 subsequent siblings)
17 siblings, 2 replies; 80+ messages in thread
From: Spencer Baugh @ 2020-11-22 2:34 UTC (permalink / raw)
To: emacs-devel; +Cc: Spencer Baugh, Arnold Noronha, Stefan Monnier, Dmitry Gutov
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
^ permalink raw reply related [flat|nested] 80+ messages in thread* Re: [PATCH v2 03/16] Stop checking the constant default for enable_multibyte_characters
2020-11-22 2:34 ` [PATCH v2 03/16] Stop checking the constant default for enable_multibyte_characters Spencer Baugh
@ 2020-11-25 20:57 ` Stefan Monnier
2020-12-01 16:52 ` Eli Zaretskii
1 sibling, 0 replies; 80+ messages in thread
From: Stefan Monnier @ 2020-11-25 20:57 UTC (permalink / raw)
To: Spencer Baugh; +Cc: Arnold Noronha, Dmitry Gutov, emacs-devel
> 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
^ permalink raw reply [flat|nested] 80+ messages in thread* Re: [PATCH v2 03/16] Stop checking the constant default for enable_multibyte_characters
2020-11-22 2:34 ` [PATCH v2 03/16] Stop checking the constant default for enable_multibyte_characters Spencer Baugh
2020-11-25 20:57 ` Stefan Monnier
@ 2020-12-01 16:52 ` Eli Zaretskii
1 sibling, 0 replies; 80+ messages in thread
From: Eli Zaretskii @ 2020-12-01 16:52 UTC (permalink / raw)
To: Spencer Baugh; +Cc: sbaugh, arnold, dgutov, monnier, emacs-devel
> 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.
^ permalink raw reply [flat|nested] 80+ messages in thread
* [PATCH v2 04/16] Take buffer field name in DEFVAR_PER_BUFFER
2020-11-22 2:34 ` [PATCH v2 00/16] " Spencer Baugh
` (2 preceding siblings ...)
2020-11-22 2:34 ` [PATCH v2 03/16] Stop checking the constant default for enable_multibyte_characters Spencer Baugh
@ 2020-11-22 2:34 ` Spencer Baugh
2020-12-01 16:56 ` Eli Zaretskii
2020-11-22 2:34 ` [PATCH v2 05/16] Add BVAR_DEFAULT for access to buffer defaults Spencer Baugh
` (13 subsequent siblings)
17 siblings, 1 reply; 80+ messages in thread
From: Spencer Baugh @ 2020-11-22 2:34 UTC (permalink / raw)
To: emacs-devel; +Cc: Spencer Baugh, Arnold Noronha, Stefan Monnier, Dmitry Gutov
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
^ permalink raw reply related [flat|nested] 80+ messages in thread* Re: [PATCH v2 04/16] Take buffer field name in DEFVAR_PER_BUFFER
2020-11-22 2:34 ` [PATCH v2 04/16] Take buffer field name in DEFVAR_PER_BUFFER Spencer Baugh
@ 2020-12-01 16:56 ` Eli Zaretskii
0 siblings, 0 replies; 80+ messages in thread
From: Eli Zaretskii @ 2020-12-01 16:56 UTC (permalink / raw)
To: Spencer Baugh; +Cc: sbaugh, arnold, dgutov, monnier, emacs-devel
> 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).
^ permalink raw reply [flat|nested] 80+ messages in thread
* [PATCH v2 05/16] Add BVAR_DEFAULT for access to buffer defaults
2020-11-22 2:34 ` [PATCH v2 00/16] " Spencer Baugh
` (3 preceding siblings ...)
2020-11-22 2:34 ` [PATCH v2 04/16] Take buffer field name in DEFVAR_PER_BUFFER Spencer Baugh
@ 2020-11-22 2:34 ` Spencer Baugh
2020-12-01 17:00 ` Eli Zaretskii
2020-11-22 2:34 ` [PATCH v2 06/16] Use bset_* functions instead of BVAR Spencer Baugh
` (12 subsequent siblings)
17 siblings, 1 reply; 80+ messages in thread
From: Spencer Baugh @ 2020-11-22 2:34 UTC (permalink / raw)
To: emacs-devel; +Cc: Spencer Baugh, Arnold Noronha, Stefan Monnier, Dmitry Gutov
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
^ permalink raw reply related [flat|nested] 80+ messages in thread* Re: [PATCH v2 05/16] Add BVAR_DEFAULT for access to buffer defaults
2020-11-22 2:34 ` [PATCH v2 05/16] Add BVAR_DEFAULT for access to buffer defaults Spencer Baugh
@ 2020-12-01 17:00 ` Eli Zaretskii
0 siblings, 0 replies; 80+ messages in thread
From: Eli Zaretskii @ 2020-12-01 17:00 UTC (permalink / raw)
To: Spencer Baugh; +Cc: sbaugh, arnold, dgutov, monnier, emacs-devel
> 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.
^ permalink raw reply [flat|nested] 80+ messages in thread
* [PATCH v2 06/16] Use bset_* functions instead of BVAR
2020-11-22 2:34 ` [PATCH v2 00/16] " Spencer Baugh
` (4 preceding siblings ...)
2020-11-22 2:34 ` [PATCH v2 05/16] Add BVAR_DEFAULT for access to buffer defaults Spencer Baugh
@ 2020-11-22 2:34 ` Spencer Baugh
2020-12-01 17:12 ` Eli Zaretskii
2020-11-22 2:34 ` [PATCH v2 07/16] Take offset not idx in PER_BUFFER_VALUE_P Spencer Baugh
` (11 subsequent siblings)
17 siblings, 1 reply; 80+ messages in thread
From: Spencer Baugh @ 2020-11-22 2:34 UTC (permalink / raw)
To: emacs-devel; +Cc: Spencer Baugh, Arnold Noronha, Stefan Monnier, Dmitry Gutov
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
^ permalink raw reply related [flat|nested] 80+ messages in thread* Re: [PATCH v2 06/16] Use bset_* functions instead of BVAR
2020-11-22 2:34 ` [PATCH v2 06/16] Use bset_* functions instead of BVAR Spencer Baugh
@ 2020-12-01 17:12 ` Eli Zaretskii
0 siblings, 0 replies; 80+ messages in thread
From: Eli Zaretskii @ 2020-12-01 17:12 UTC (permalink / raw)
To: Spencer Baugh; +Cc: sbaugh, arnold, dgutov, monnier, emacs-devel
> 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.
^ permalink raw reply [flat|nested] 80+ messages in thread
* [PATCH v2 07/16] Take offset not idx in PER_BUFFER_VALUE_P
2020-11-22 2:34 ` [PATCH v2 00/16] " Spencer Baugh
` (5 preceding siblings ...)
2020-11-22 2:34 ` [PATCH v2 06/16] Use bset_* functions instead of BVAR Spencer Baugh
@ 2020-11-22 2:34 ` Spencer Baugh
2020-12-01 17:20 ` Eli Zaretskii
2020-11-22 2:34 ` [PATCH v2 08/16] Combine unnecessarily separate loops in buffer.c Spencer Baugh
` (10 subsequent siblings)
17 siblings, 1 reply; 80+ messages in thread
From: Spencer Baugh @ 2020-11-22 2:34 UTC (permalink / raw)
To: emacs-devel; +Cc: Spencer Baugh, Arnold Noronha, Stefan Monnier, Dmitry Gutov
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
^ permalink raw reply related [flat|nested] 80+ messages in thread* Re: [PATCH v2 07/16] Take offset not idx in PER_BUFFER_VALUE_P
2020-11-22 2:34 ` [PATCH v2 07/16] Take offset not idx in PER_BUFFER_VALUE_P Spencer Baugh
@ 2020-12-01 17:20 ` Eli Zaretskii
0 siblings, 0 replies; 80+ messages in thread
From: Eli Zaretskii @ 2020-12-01 17:20 UTC (permalink / raw)
To: Spencer Baugh; +Cc: sbaugh, arnold, dgutov, monnier, emacs-devel
> 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.
^ permalink raw reply [flat|nested] 80+ messages in thread
* [PATCH v2 08/16] Combine unnecessarily separate loops in buffer.c
2020-11-22 2:34 ` [PATCH v2 00/16] " Spencer Baugh
` (6 preceding siblings ...)
2020-11-22 2:34 ` [PATCH v2 07/16] Take offset not idx in PER_BUFFER_VALUE_P Spencer Baugh
@ 2020-11-22 2:34 ` Spencer Baugh
2020-12-01 17:22 ` Eli Zaretskii
2020-11-22 2:34 ` [PATCH v2 09/16] Add and use BUFFER_DEFAULT_VALUE_P Spencer Baugh
` (9 subsequent siblings)
17 siblings, 1 reply; 80+ messages in thread
From: Spencer Baugh @ 2020-11-22 2:34 UTC (permalink / raw)
To: emacs-devel; +Cc: Spencer Baugh, Arnold Noronha, Stefan Monnier, Dmitry Gutov
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
^ permalink raw reply related [flat|nested] 80+ messages in thread* Re: [PATCH v2 08/16] Combine unnecessarily separate loops in buffer.c
2020-11-22 2:34 ` [PATCH v2 08/16] Combine unnecessarily separate loops in buffer.c Spencer Baugh
@ 2020-12-01 17:22 ` Eli Zaretskii
0 siblings, 0 replies; 80+ messages in thread
From: Eli Zaretskii @ 2020-12-01 17:22 UTC (permalink / raw)
To: Spencer Baugh; +Cc: sbaugh, arnold, dgutov, monnier, emacs-devel
> 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.
^ permalink raw reply [flat|nested] 80+ messages in thread
* [PATCH v2 09/16] Add and use BUFFER_DEFAULT_VALUE_P
2020-11-22 2:34 ` [PATCH v2 00/16] " Spencer Baugh
` (7 preceding siblings ...)
2020-11-22 2:34 ` [PATCH v2 08/16] Combine unnecessarily separate loops in buffer.c Spencer Baugh
@ 2020-11-22 2:34 ` Spencer Baugh
2020-12-01 17:24 ` Eli Zaretskii
2020-11-22 2:34 ` [PATCH v2 10/16] Add and use KILL_PER_BUFFER_VALUE Spencer Baugh
` (8 subsequent siblings)
17 siblings, 1 reply; 80+ messages in thread
From: Spencer Baugh @ 2020-11-22 2:34 UTC (permalink / raw)
To: emacs-devel; +Cc: Spencer Baugh, Arnold Noronha, Stefan Monnier, Dmitry Gutov
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
^ permalink raw reply related [flat|nested] 80+ messages in thread* Re: [PATCH v2 09/16] Add and use BUFFER_DEFAULT_VALUE_P
2020-11-22 2:34 ` [PATCH v2 09/16] Add and use BUFFER_DEFAULT_VALUE_P Spencer Baugh
@ 2020-12-01 17:24 ` Eli Zaretskii
0 siblings, 0 replies; 80+ messages in thread
From: Eli Zaretskii @ 2020-12-01 17:24 UTC (permalink / raw)
To: Spencer Baugh; +Cc: sbaugh, arnold, dgutov, monnier, emacs-devel
> 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.
^ permalink raw reply [flat|nested] 80+ messages in thread
* [PATCH v2 10/16] Add and use KILL_PER_BUFFER_VALUE
2020-11-22 2:34 ` [PATCH v2 00/16] " Spencer Baugh
` (8 preceding siblings ...)
2020-11-22 2:34 ` [PATCH v2 09/16] Add and use BUFFER_DEFAULT_VALUE_P Spencer Baugh
@ 2020-11-22 2:34 ` Spencer Baugh
2020-12-01 17:26 ` Eli Zaretskii
2020-11-22 2:34 ` [PATCH v2 11/16] Assert that PER_BUFFER_IDX for Lisp variables is not 0 Spencer Baugh
` (7 subsequent siblings)
17 siblings, 1 reply; 80+ messages in thread
From: Spencer Baugh @ 2020-11-22 2:34 UTC (permalink / raw)
To: emacs-devel; +Cc: Spencer Baugh, Arnold Noronha, Stefan Monnier, Dmitry Gutov
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
^ permalink raw reply related [flat|nested] 80+ messages in thread* Re: [PATCH v2 10/16] Add and use KILL_PER_BUFFER_VALUE
2020-11-22 2:34 ` [PATCH v2 10/16] Add and use KILL_PER_BUFFER_VALUE Spencer Baugh
@ 2020-12-01 17:26 ` Eli Zaretskii
0 siblings, 0 replies; 80+ messages in thread
From: Eli Zaretskii @ 2020-12-01 17:26 UTC (permalink / raw)
To: Spencer Baugh; +Cc: sbaugh, arnold, dgutov, monnier, emacs-devel
> 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.
^ permalink raw reply [flat|nested] 80+ messages in thread
* [PATCH v2 11/16] Assert that PER_BUFFER_IDX for Lisp variables is not 0
2020-11-22 2:34 ` [PATCH v2 00/16] " Spencer Baugh
` (9 preceding siblings ...)
2020-11-22 2:34 ` [PATCH v2 10/16] Add and use KILL_PER_BUFFER_VALUE Spencer Baugh
@ 2020-11-22 2:34 ` Spencer Baugh
2020-12-01 17:32 ` Eli Zaretskii
2020-11-22 2:34 ` [PATCH v2 12/16] Rearrange set_internal for buffer forwarded symbols Spencer Baugh
` (6 subsequent siblings)
17 siblings, 1 reply; 80+ messages in thread
From: Spencer Baugh @ 2020-11-22 2:34 UTC (permalink / raw)
To: emacs-devel; +Cc: Spencer Baugh, Arnold Noronha, Stefan Monnier, Dmitry Gutov
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
^ permalink raw reply related [flat|nested] 80+ messages in thread* Re: [PATCH v2 11/16] Assert that PER_BUFFER_IDX for Lisp variables is not 0
2020-11-22 2:34 ` [PATCH v2 11/16] Assert that PER_BUFFER_IDX for Lisp variables is not 0 Spencer Baugh
@ 2020-12-01 17:32 ` Eli Zaretskii
0 siblings, 0 replies; 80+ messages in thread
From: Eli Zaretskii @ 2020-12-01 17:32 UTC (permalink / raw)
To: Spencer Baugh; +Cc: sbaugh, arnold, dgutov, monnier, emacs-devel
> 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).
^ permalink raw reply [flat|nested] 80+ messages in thread
* [PATCH v2 12/16] Rearrange set_internal for buffer forwarded symbols
2020-11-22 2:34 ` [PATCH v2 00/16] " Spencer Baugh
` (10 preceding siblings ...)
2020-11-22 2:34 ` [PATCH v2 11/16] Assert that PER_BUFFER_IDX for Lisp variables is not 0 Spencer Baugh
@ 2020-11-22 2:34 ` Spencer Baugh
2020-12-01 17:35 ` Eli Zaretskii
2020-11-22 2:34 ` [PATCH v2 13/16] Get rid of buffer_permanent_local_flags array Spencer Baugh
` (5 subsequent siblings)
17 siblings, 1 reply; 80+ messages in thread
From: Spencer Baugh @ 2020-11-22 2:34 UTC (permalink / raw)
To: emacs-devel; +Cc: Spencer Baugh, Arnold Noronha, Stefan Monnier, Dmitry Gutov
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
^ permalink raw reply related [flat|nested] 80+ messages in thread* Re: [PATCH v2 12/16] Rearrange set_internal for buffer forwarded symbols
2020-11-22 2:34 ` [PATCH v2 12/16] Rearrange set_internal for buffer forwarded symbols Spencer Baugh
@ 2020-12-01 17:35 ` Eli Zaretskii
0 siblings, 0 replies; 80+ messages in thread
From: Eli Zaretskii @ 2020-12-01 17:35 UTC (permalink / raw)
To: Spencer Baugh; +Cc: sbaugh, arnold, dgutov, monnier, emacs-devel
> 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).
^ permalink raw reply [flat|nested] 80+ messages in thread
* [PATCH v2 13/16] Get rid of buffer_permanent_local_flags array
2020-11-22 2:34 ` [PATCH v2 00/16] " Spencer Baugh
` (11 preceding siblings ...)
2020-11-22 2:34 ` [PATCH v2 12/16] Rearrange set_internal for buffer forwarded symbols Spencer Baugh
@ 2020-11-22 2:34 ` Spencer Baugh
2020-11-22 16:16 ` Eli Zaretskii
2020-12-01 17:39 ` Eli Zaretskii
2020-11-22 2:34 ` [PATCH v2 14/16] Remove unnecessary Qunbound check Spencer Baugh
` (4 subsequent siblings)
17 siblings, 2 replies; 80+ messages in thread
From: Spencer Baugh @ 2020-11-22 2:34 UTC (permalink / raw)
To: emacs-devel; +Cc: Spencer Baugh, Arnold Noronha, Stefan Monnier, Dmitry Gutov
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
^ permalink raw reply related [flat|nested] 80+ messages in thread* Re: [PATCH v2 13/16] Get rid of buffer_permanent_local_flags array
2020-11-22 2:34 ` [PATCH v2 13/16] Get rid of buffer_permanent_local_flags array Spencer Baugh
@ 2020-11-22 16:16 ` Eli Zaretskii
2020-12-01 17:39 ` Eli Zaretskii
1 sibling, 0 replies; 80+ messages in thread
From: Eli Zaretskii @ 2020-11-22 16:16 UTC (permalink / raw)
To: Spencer Baugh
Cc: arnold, Richard Stallman, sbaugh, emacs-devel, monnier, dgutov
> 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.
^ permalink raw reply [flat|nested] 80+ messages in thread
* Re: [PATCH v2 13/16] Get rid of buffer_permanent_local_flags array
2020-11-22 2:34 ` [PATCH v2 13/16] Get rid of buffer_permanent_local_flags array Spencer Baugh
2020-11-22 16:16 ` Eli Zaretskii
@ 2020-12-01 17:39 ` Eli Zaretskii
1 sibling, 0 replies; 80+ messages in thread
From: Eli Zaretskii @ 2020-12-01 17:39 UTC (permalink / raw)
To: Spencer Baugh; +Cc: sbaugh, arnold, dgutov, monnier, emacs-devel
> 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.
^ permalink raw reply [flat|nested] 80+ messages in thread
* [PATCH v2 14/16] Remove unnecessary Qunbound check
2020-11-22 2:34 ` [PATCH v2 00/16] " Spencer Baugh
` (12 preceding siblings ...)
2020-11-22 2:34 ` [PATCH v2 13/16] Get rid of buffer_permanent_local_flags array Spencer Baugh
@ 2020-11-22 2:34 ` Spencer Baugh
2020-12-01 17:40 ` Eli Zaretskii
2020-11-22 2:34 ` [PATCH v2 15/16] Remove local_flags array in struct buffer Spencer Baugh
` (3 subsequent siblings)
17 siblings, 1 reply; 80+ messages in thread
From: Spencer Baugh @ 2020-11-22 2:34 UTC (permalink / raw)
To: emacs-devel; +Cc: Spencer Baugh, Arnold Noronha, Stefan Monnier, Dmitry Gutov
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
^ permalink raw reply related [flat|nested] 80+ messages in thread* Re: [PATCH v2 14/16] Remove unnecessary Qunbound check
2020-11-22 2:34 ` [PATCH v2 14/16] Remove unnecessary Qunbound check Spencer Baugh
@ 2020-12-01 17:40 ` Eli Zaretskii
0 siblings, 0 replies; 80+ messages in thread
From: Eli Zaretskii @ 2020-12-01 17:40 UTC (permalink / raw)
To: Spencer Baugh; +Cc: sbaugh, arnold, dgutov, monnier, emacs-devel
> 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?
^ permalink raw reply [flat|nested] 80+ messages in thread
* [PATCH v2 15/16] Remove local_flags array in struct buffer
2020-11-22 2:34 ` [PATCH v2 00/16] " Spencer Baugh
` (13 preceding siblings ...)
2020-11-22 2:34 ` [PATCH v2 14/16] Remove unnecessary Qunbound check Spencer Baugh
@ 2020-11-22 2:34 ` Spencer Baugh
2020-11-22 20:04 ` Stefan Monnier
2020-12-01 17:57 ` Eli Zaretskii
2020-11-22 2:34 ` [PATCH v2 16/16] Remove usage of buffer_local_flags Spencer Baugh
` (2 subsequent siblings)
17 siblings, 2 replies; 80+ messages in thread
From: Spencer Baugh @ 2020-11-22 2:34 UTC (permalink / raw)
To: emacs-devel; +Cc: Spencer Baugh, Arnold Noronha, Stefan Monnier, Dmitry Gutov
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
^ permalink raw reply related [flat|nested] 80+ messages in thread* Re: [PATCH v2 15/16] Remove local_flags array in struct buffer
2020-11-22 2:34 ` [PATCH v2 15/16] Remove local_flags array in struct buffer Spencer Baugh
@ 2020-11-22 20:04 ` Stefan Monnier
2020-12-01 17:57 ` Eli Zaretskii
1 sibling, 0 replies; 80+ messages in thread
From: Stefan Monnier @ 2020-11-22 20:04 UTC (permalink / raw)
To: Spencer Baugh; +Cc: Arnold Noronha, Dmitry Gutov, emacs-devel
> 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
^ permalink raw reply [flat|nested] 80+ messages in thread* Re: [PATCH v2 15/16] Remove local_flags array in struct buffer
2020-11-22 2:34 ` [PATCH v2 15/16] Remove local_flags array in struct buffer Spencer Baugh
2020-11-22 20:04 ` Stefan Monnier
@ 2020-12-01 17:57 ` Eli Zaretskii
1 sibling, 0 replies; 80+ messages in thread
From: Eli Zaretskii @ 2020-12-01 17:57 UTC (permalink / raw)
To: Spencer Baugh; +Cc: sbaugh, arnold, dgutov, monnier, emacs-devel
> 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?
^ permalink raw reply [flat|nested] 80+ messages in thread
* [PATCH v2 16/16] Remove usage of buffer_local_flags
2020-11-22 2:34 ` [PATCH v2 00/16] " Spencer Baugh
` (14 preceding siblings ...)
2020-11-22 2:34 ` [PATCH v2 15/16] Remove local_flags array in struct buffer Spencer Baugh
@ 2020-11-22 2:34 ` Spencer Baugh
2020-12-01 18:05 ` Eli Zaretskii
2020-11-22 11:19 ` [PATCH v2 00/16] Speeding up DEFVAR_PER_BUFFER Kévin Le Gouguec
2020-11-22 16:12 ` Eli Zaretskii
17 siblings, 1 reply; 80+ messages in thread
From: Spencer Baugh @ 2020-11-22 2:34 UTC (permalink / raw)
To: emacs-devel; +Cc: Spencer Baugh, Arnold Noronha, Stefan Monnier, Dmitry Gutov
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
^ permalink raw reply related [flat|nested] 80+ messages in thread* Re: [PATCH v2 16/16] Remove usage of buffer_local_flags
2020-11-22 2:34 ` [PATCH v2 16/16] Remove usage of buffer_local_flags Spencer Baugh
@ 2020-12-01 18:05 ` Eli Zaretskii
0 siblings, 0 replies; 80+ messages in thread
From: Eli Zaretskii @ 2020-12-01 18:05 UTC (permalink / raw)
To: Spencer Baugh; +Cc: sbaugh, arnold, dgutov, monnier, emacs-devel
> 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.
^ permalink raw reply [flat|nested] 80+ messages in thread
* Re: [PATCH v2 00/16] Speeding up DEFVAR_PER_BUFFER
2020-11-22 2:34 ` [PATCH v2 00/16] " Spencer Baugh
` (15 preceding siblings ...)
2020-11-22 2:34 ` [PATCH v2 16/16] Remove usage of buffer_local_flags Spencer Baugh
@ 2020-11-22 11:19 ` Kévin Le Gouguec
2020-11-22 16:12 ` Eli Zaretskii
17 siblings, 0 replies; 80+ messages in thread
From: Kévin Le Gouguec @ 2020-11-22 11:19 UTC (permalink / raw)
To: Spencer Baugh; +Cc: Arnold Noronha, Dmitry Gutov, Stefan Monnier, emacs-devel
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 :)
^ permalink raw reply [flat|nested] 80+ messages in thread* Re: [PATCH v2 00/16] Speeding up DEFVAR_PER_BUFFER
2020-11-22 2:34 ` [PATCH v2 00/16] " Spencer Baugh
` (16 preceding siblings ...)
2020-11-22 11:19 ` [PATCH v2 00/16] Speeding up DEFVAR_PER_BUFFER Kévin Le Gouguec
@ 2020-11-22 16:12 ` Eli Zaretskii
2020-11-22 16:28 ` Spencer Baugh
17 siblings, 1 reply; 80+ messages in thread
From: Eli Zaretskii @ 2020-11-22 16:12 UTC (permalink / raw)
To: Spencer Baugh
Cc: arnold, Richard Stallman, sbaugh, emacs-devel, monnier, dgutov
> 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.
^ permalink raw reply [flat|nested] 80+ messages in thread* Re: [PATCH v2 00/16] Speeding up DEFVAR_PER_BUFFER
2020-11-22 16:12 ` Eli Zaretskii
@ 2020-11-22 16:28 ` Spencer Baugh
2020-11-22 17:13 ` Eli Zaretskii
2020-11-22 20:11 ` Stefan Monnier
0 siblings, 2 replies; 80+ messages in thread
From: Spencer Baugh @ 2020-11-22 16:28 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: arnold, dgutov, monnier, Richard Stallman, emacs-devel
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)
^ permalink raw reply [flat|nested] 80+ messages in thread* Re: [PATCH v2 00/16] Speeding up DEFVAR_PER_BUFFER
2020-11-22 16:28 ` Spencer Baugh
@ 2020-11-22 17:13 ` Eli Zaretskii
2020-11-29 17:41 ` Spencer Baugh
2020-11-22 20:11 ` Stefan Monnier
1 sibling, 1 reply; 80+ messages in thread
From: Eli Zaretskii @ 2020-11-22 17:13 UTC (permalink / raw)
To: Spencer Baugh; +Cc: arnold, dgutov, monnier, rms, emacs-devel
> 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.
^ permalink raw reply [flat|nested] 80+ messages in thread* Re: [PATCH v2 00/16] Speeding up DEFVAR_PER_BUFFER
2020-11-22 17:13 ` Eli Zaretskii
@ 2020-11-29 17:41 ` Spencer Baugh
2020-11-30 18:32 ` Eli Zaretskii
0 siblings, 1 reply; 80+ messages in thread
From: Spencer Baugh @ 2020-11-29 17:41 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: arnold, dgutov, monnier, rms, emacs-devel
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.
^ permalink raw reply [flat|nested] 80+ messages in thread* Re: [PATCH v2 00/16] Speeding up DEFVAR_PER_BUFFER
2020-11-29 17:41 ` Spencer Baugh
@ 2020-11-30 18:32 ` Eli Zaretskii
2020-11-30 20:11 ` Spencer Baugh
2020-12-01 18:10 ` Eli Zaretskii
0 siblings, 2 replies; 80+ messages in thread
From: Eli Zaretskii @ 2020-11-30 18:32 UTC (permalink / raw)
To: Spencer Baugh; +Cc: arnold, dgutov, monnier, rms, emacs-devel
> 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.
^ permalink raw reply [flat|nested] 80+ messages in thread
* Re: [PATCH v2 00/16] Speeding up DEFVAR_PER_BUFFER
2020-11-30 18:32 ` Eli Zaretskii
@ 2020-11-30 20:11 ` Spencer Baugh
2020-11-30 22:10 ` Stefan Monnier
2020-12-01 15:10 ` Eli Zaretskii
2020-12-01 18:10 ` Eli Zaretskii
1 sibling, 2 replies; 80+ messages in thread
From: Spencer Baugh @ 2020-11-30 20:11 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: arnold, dgutov, monnier, rms, emacs-devel
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.
^ permalink raw reply [flat|nested] 80+ messages in thread
* Re: [PATCH v2 00/16] Speeding up DEFVAR_PER_BUFFER
2020-11-30 20:11 ` Spencer Baugh
@ 2020-11-30 22:10 ` Stefan Monnier
2020-11-30 22:26 ` Andrea Corallo via Emacs development discussions.
2020-12-01 15:15 ` Eli Zaretskii
2020-12-01 15:10 ` Eli Zaretskii
1 sibling, 2 replies; 80+ messages in thread
From: Stefan Monnier @ 2020-11-30 22:10 UTC (permalink / raw)
To: Spencer Baugh; +Cc: Eli Zaretskii, dgutov, arnold, rms, emacs-devel
> 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
^ permalink raw reply [flat|nested] 80+ messages in thread* Re: [PATCH v2 00/16] Speeding up DEFVAR_PER_BUFFER
2020-11-30 22:10 ` Stefan Monnier
@ 2020-11-30 22:26 ` Andrea Corallo via Emacs development discussions.
2020-12-01 15:15 ` Eli Zaretskii
1 sibling, 0 replies; 80+ messages in thread
From: Andrea Corallo via Emacs development discussions. @ 2020-11-30 22:26 UTC (permalink / raw)
To: Stefan Monnier
Cc: Spencer Baugh, Eli Zaretskii, dgutov, arnold, rms, emacs-devel
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
^ permalink raw reply [flat|nested] 80+ messages in thread
* Re: [PATCH v2 00/16] Speeding up DEFVAR_PER_BUFFER
2020-11-30 22:10 ` Stefan Monnier
2020-11-30 22:26 ` Andrea Corallo via Emacs development discussions.
@ 2020-12-01 15:15 ` Eli Zaretskii
2020-12-01 15:56 ` Stefan Monnier
1 sibling, 1 reply; 80+ messages in thread
From: Eli Zaretskii @ 2020-12-01 15:15 UTC (permalink / raw)
To: Stefan Monnier; +Cc: sbaugh, arnold, dgutov, rms, emacs-devel
> 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.
^ permalink raw reply [flat|nested] 80+ messages in thread
* Re: [PATCH v2 00/16] Speeding up DEFVAR_PER_BUFFER
2020-12-01 15:15 ` Eli Zaretskii
@ 2020-12-01 15:56 ` Stefan Monnier
0 siblings, 0 replies; 80+ messages in thread
From: Stefan Monnier @ 2020-12-01 15:56 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: sbaugh, arnold, dgutov, rms, emacs-devel
> 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
^ permalink raw reply [flat|nested] 80+ messages in thread
* Re: [PATCH v2 00/16] Speeding up DEFVAR_PER_BUFFER
2020-11-30 20:11 ` Spencer Baugh
2020-11-30 22:10 ` Stefan Monnier
@ 2020-12-01 15:10 ` Eli Zaretskii
2020-12-01 15:20 ` Spencer Baugh
2020-12-01 16:01 ` Stefan Monnier
1 sibling, 2 replies; 80+ messages in thread
From: Eli Zaretskii @ 2020-12-01 15:10 UTC (permalink / raw)
To: Spencer Baugh; +Cc: arnold, dgutov, monnier, rms, emacs-devel
> 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.
^ permalink raw reply [flat|nested] 80+ messages in thread
* Re: [PATCH v2 00/16] Speeding up DEFVAR_PER_BUFFER
2020-12-01 15:10 ` Eli Zaretskii
@ 2020-12-01 15:20 ` Spencer Baugh
2020-12-01 16:01 ` Stefan Monnier
1 sibling, 0 replies; 80+ messages in thread
From: Spencer Baugh @ 2020-12-01 15:20 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: arnold, dgutov, monnier, rms, emacs-devel
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...
^ permalink raw reply [flat|nested] 80+ messages in thread
* Re: [PATCH v2 00/16] Speeding up DEFVAR_PER_BUFFER
2020-12-01 15:10 ` Eli Zaretskii
2020-12-01 15:20 ` Spencer Baugh
@ 2020-12-01 16:01 ` Stefan Monnier
2020-12-01 16:16 ` Eli Zaretskii
1 sibling, 1 reply; 80+ messages in thread
From: Stefan Monnier @ 2020-12-01 16:01 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: Spencer Baugh, arnold, dgutov, rms, emacs-devel
> 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
^ permalink raw reply [flat|nested] 80+ messages in thread* Re: [PATCH v2 00/16] Speeding up DEFVAR_PER_BUFFER
2020-12-01 16:01 ` Stefan Monnier
@ 2020-12-01 16:16 ` Eli Zaretskii
0 siblings, 0 replies; 80+ messages in thread
From: Eli Zaretskii @ 2020-12-01 16:16 UTC (permalink / raw)
To: Stefan Monnier; +Cc: sbaugh, arnold, dgutov, rms, emacs-devel
> 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?
^ permalink raw reply [flat|nested] 80+ messages in thread
* Re: [PATCH v2 00/16] Speeding up DEFVAR_PER_BUFFER
2020-11-30 18:32 ` Eli Zaretskii
2020-11-30 20:11 ` Spencer Baugh
@ 2020-12-01 18:10 ` Eli Zaretskii
1 sibling, 0 replies; 80+ messages in thread
From: Eli Zaretskii @ 2020-12-01 18:10 UTC (permalink / raw)
To: sbaugh; +Cc: arnold, emacs-devel, monnier, rms, dgutov
> 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!
^ permalink raw reply [flat|nested] 80+ messages in thread
* Re: [PATCH v2 00/16] Speeding up DEFVAR_PER_BUFFER
2020-11-22 16:28 ` Spencer Baugh
2020-11-22 17:13 ` Eli Zaretskii
@ 2020-11-22 20:11 ` Stefan Monnier
1 sibling, 0 replies; 80+ messages in thread
From: Stefan Monnier @ 2020-11-22 20:11 UTC (permalink / raw)
To: Spencer Baugh
Cc: Eli Zaretskii, dgutov, arnold, Richard Stallman, emacs-devel
> 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
^ permalink raw reply [flat|nested] 80+ messages in thread