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

[Kelly Dean <kelly@prtime.org>]

[-- Attachment #1: Type: text/plain, Size: 4621 bytes --]

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.



[-- Attachment #2: varhook-advice-24.4.patch.gz --]
[-- Type: application/octet-stream, Size: 8998 bytes --]

[-- Attachment #3: varhook-advice.patch.gz --]
[-- Type: application/octet-stream, Size: 8632 bytes --]