[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 09/10] Access buffer_defaults in BVAR if there's no local bin
From: |
Stefan Monnier |
Subject: |
Re: [PATCH 09/10] Access buffer_defaults in BVAR if there's no local binding |
Date: |
Thu, 19 Nov 2020 13:08:16 -0500 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/28.0.50 (gnu/linux) |
> If the buffer_local_flags Lisp_Object is 0, though, it means this
> field in buffer_local_flags wasn't initialized,
Actually, no it means it was explicitly set (otherwise it would be Qnil).
0 means that this slot isn't exposed as a lisp variable (and those
fields don't have a "default value").
> which means this field is always buffer-local (and therefore doesn't
> use buffer_defaults).
That's right.
> @@ -825,9 +825,31 @@ set_per_buffer_value (struct buffer *b, int offset,
> Lisp_Object value)
> *(Lisp_Object *)(offset + (char *) b) = value;
> }
>
> +INLINE bool
> +SHOULD_USE_BUFFER_DEFAULTS(struct buffer *b, ptrdiff_t offset)
^^
SPC
Remember to use a space before opening parens (there are other
occurrences in your patches).
> +{
> + Lisp_Object obj = *((Lisp_Object *) (offset + (char *)
> &buffer_local_flags));
I think you want to use `PER_BUFFER_IDX` here.
> + if (obj.i == 0)
> + return false;
Please don't use `obj.i`, which relies on the internal representation of
`Lisp_Object`.
Also, this test is not needed: a zero value here (i.e. obj == Qnil)
would be a bug.
We already test this elsewhere, but even if we don't test it here,
`XFIXNUM` could signal an error if `obj` is not a fixnum, so this test
shouldn't be needed even for debugging purposes.
> + int idx = XFIXNUM(obj);
> + if (idx < 1)
> + return false;
> + return b->local_flags[idx] == 0;
I'd write it as
return idx > 0 && !PER_BUFFER_VALUE_P (b, idx);
Largely because I dislike writing the constant `true` or `false`,
especially within `if` branches.
> +INLINE Lisp_Object
> +bvar_get(struct buffer *b, ptrdiff_t offset)
> +{
> + if (SHOULD_USE_BUFFER_DEFAULTS(b, offset)) {
> + return per_buffer_value(&buffer_defaults, offset);
> + } else {
> + return per_buffer_value(b, offset);
> + }
Remember also that our coding style puts braces on their own lines like:
if (SHOULD_USE_BUFFER_DEFAULTS (b, offset))
{
return per_buffer_value (&buffer_defaults, offset);
}
else
{
return per_buffer_value (b, offset);
}
But here you can just drop the braces (and add the spaces and use
`per_buffer_default`):
if (SHOULD_USE_BUFFER_DEFAULTS (b, offset))
return per_buffer_default (offset);
else
return per_buffer_value (b, offset);
aka
return SHOULD_USE_BUFFER_DEFAULTS (b, offset))
? per_buffer_value (&buffer_defaults, offset)
: per_buffer_value (b, offset);
> -#define BVAR(buf, field) ((void)0, (buf)->field ## _)
> +#define BVAR(buf, field) bvar_get(buf, PER_BUFFER_VAR_OFFSET (field))
This will unnecessarily slow down access to those fields which have
an `idx < 1` since I can't imagine the compiler will be able to figure
out that `SHOULD_USE_BUFFER_DEFAULTS` will always return false.
So maybe those fields should be accessed via BVAR_DIRECT (which could
include an `eassert` to make sure that `SHOULD_USE_BUFFER_DEFAULTS`
indeed is false).
Your patch series will also presumably slow down BVAR access to the
other fields since they need to test `SHOULD_USE_BUFFER_DEFAULTS` now,
but I think this is the price to pay. It should be negligible for
accesses to those vars from Elisp code, but of the 600 or so uses of
BVAR in the current code, I wonder if some of them could show
a measurable speed penalty.
Stefan
- Re: ido-switch-buffer is slow with many buffers; others are fast, (continued)
- Re: ido-switch-buffer is slow with many buffers; others are fast, Spencer Baugh, 2020/11/15
- Re: ido-switch-buffer is slow with many buffers; others are fast, Arnold Noronha, 2020/11/15
- [PATCH 02/10] Add bset_save_length and use it, Spencer Baugh, 2020/11/19
- [PATCH 04/10] Use bset_enable_multibyte_characters everywhere, Spencer Baugh, 2020/11/19
- [PATCH 00/10] Speeding up DEFVAR_PER_BUFFER (Was: ido-switch-buffer is slow), Spencer Baugh, 2020/11/19
- Re: [PATCH 00/10] Speeding up DEFVAR_PER_BUFFER, Stefan Monnier, 2020/11/19
- [PATCH 03/10] Use bset_last_selected_window everywhere, Spencer Baugh, 2020/11/19
- [PATCH 01/10] Take buffer field name in DEFVAR_PER_BUFFER, Spencer Baugh, 2020/11/19
- [PATCH 06/10] Disallow using BVAR as an lvalue, Spencer Baugh, 2020/11/19
- [PATCH 09/10] Access buffer_defaults in BVAR if there's no local binding, Spencer Baugh, 2020/11/19
- Re: [PATCH 09/10] Access buffer_defaults in BVAR if there's no local binding,
Stefan Monnier <=
- Re: [PATCH 09/10] Access buffer_defaults in BVAR if there's no local binding, Eli Zaretskii, 2020/11/19
- [PATCH 05/10] Add BVAR_DEFAULT for access to buffer defaults, Spencer Baugh, 2020/11/19
- [PATCH 08/10] Make cache_long_scans buffer-local when setting it, Spencer Baugh, 2020/11/19
- [PATCH 07/10] Reorder buffer.h for upcoming rework of BVAR, Spencer Baugh, 2020/11/19
- [PATCH 10/10] Don't iterate over all buffers in set_default_internal, Spencer Baugh, 2020/11/19
- Re: [PATCH 10/10] Don't iterate over all buffers in set_default_internal, Stefan Monnier, 2020/11/19