[PATCH] (Updated) Run hook when variable is set

[Kelly Dean]

Stefan Monnier wrote:
> No, the idea was rather to do:
>
> (defun set-internal-1 (args)
>   (block nil ; Because Elisp isn't CL
>     (if (constant-or-hooked-p)
>         (if (constant-p)
>             (if (forbidden-p)
>                 (error "setting constant")
>               (return))
>           (funcall symbol-watch-function ..args..))
>       (do-some-stuff)
>       (and-many-more-lines-of-stuff)
>       (set-some-variable))))

Then the many more lines of stuff have to be copied into symbol-watch-function. 
Those lines can't just be skipped; they're necessary for correctly setting 
localized and forwarded symbols. By default, hooking a symbol must not change 
the behavior of Emacs, except for slowing it down. A hook handler can 
optionally change the behavior, but just hooking the symbol must cause no 
change if the handler doesn't.

I decided to solve the problem by factoring out the many more lines of stuff to 
a separate function, so they don't have to be duplicated. This introduces an 
extra function call for them, but that's ok, since setting localized and 
forwarded symbols isn't the hot path (and they're already slow anyway). This 
change has no effect on the hot path.

> I think watchpoints usually have the functionality of catching the
> modifications, being able to see the "before-change value" and being
> able to replace the assignment with something else.  So "watch" makes
> a lot of sense to me.

Um, ok. I already improved the names, but I can change them again if necessary.


Updated patches attached. The first applies to 24.4. The second applies to 
trunk (using «patch», not «git apply»).


Feature improvements:

It reports the environment as «dyn-local» rather than «invalid» for set-default 
within a dynamic let-binding, since you said that's a valid thing to do.

It passes not only the new value being set, but also the current value of the 
variable.

The function names are more informative.

The API now uses function advice instead of a standard hook. Whenever a hooked 
variable is set, symbol-setter-function is called. Since it's the setter 
function, it's natural that overriding it would override the setting that 
occurs, so this is how I implemented the API. As a result, it's not necessary 
to explicitly set the variable in your handler, which means it's not necessary 
to temporarily unhook it either; all you have to do is return the value you 
want to set it to. See the docstring for symbol-setter-function for details.


Performance improvements:

I put «hooked» into «constant», and removed my branches from the hot paths of 
specbind and set_internal.

I did the minor optimizations of set_internal I mentioned previously, plus the 
relevant one in specbind and in find_symbol_value. Also in a few others that 
aren't on hot paths, but they were trivial so I did them anyway.

I inlined grow_specpdl, which speeds up specbind, which more than compensates 
for the conditional branch that varhook requires in unbind_to. I also inlined 
the first part (the hot path) of set_internal (i.e. of set_internal_1 in my 
patch). Both of those are fairly small and not used in very many places. The 
cost is an increase in the size of the Emacs executable by about 20kB (out of 
18.9MB on my system, so that's about 0.1%).

The result of these changes is a measurable performance improvement compared to 
unmodified Emacs for the common cases, i.e. for setting and dynamically binding 
plainval (non-buffer-local, non-forwarded) symbols. So now, my patch actually 
_does_ make Emacs faster. ;-) Here are the benchmarks:

(require 'cl)

(defun test ()
  (setq x 0)
  (benchmark-run-compiled 100000 (incf x)))

(defun test1 ()
  (setq x 0)
  (benchmark-run-compiled
      (do () ((> x 100000)) (incf x))))

(defun test2 ()
  (benchmark-run-compiled 1000
    (letrec
        ((foo
          (lambda (x)
            (let ((x (1+ x)))
              (if (> x 100)
                  x
                (1- (funcall foo x)))))))
      (funcall foo 0))))

Execution time results to 4 significant digits:

test
           minimum  average  maximum
Emacs 24.4 0.006253 0.007259 0.009721
with patch 0.001816 0.002148 0.002425
Average improvement: 70%

test1
           minimum  average  maximum
Emacs 24.4 0.01074  0.01075  0.01078
with patch 0.009081 0.009301 0.009671
Average improvement: 13%

test2
           minimum  average  maximum
Emacs 24.4 0.04401  0.04409  0.04417
with patch 0.04196  0.04202  0.04207
Average improvement: 4.7%

For all three tests, the slowest run with the patch was faster than the fastest 
run without the patch.

varhook-advice-24.4.patch.gz
Description: Binary data

varhook-advice.patch.gz
Description: Binary data