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

[Spencer Baugh]

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

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

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

out of shr-tests.el.

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

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

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

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

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

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

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

What do people prefer?

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

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

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