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

[Kelly Dean <kelly@prtime.org>]

Stefan Monnier wrote:
>> But avoiding the conditional branch that way for the set_internal function
>> (or set_internal_1 in my patch) would require duplicating the entire
>> function, in both the source code and the compiled code.
>
> I don't understand why.

set_internal_1 (which is just set_internal with an additional argument; the new name is just to avoid having to change other code) has try_run_varhook in three places: one for each of the cases of
⌜switch (sym->redirect)⌝ except SYMBOL_VARALIAS. The first three things that set_internal_1 does are:
set «voide»
check «symbol»
check the «constant» bit, using ⌜if (SYMBOL_CONSTANT_P (symbol))⌝

If «hooked» is put into «constant», and checked only within that «if» block at the beginning of set_internal_1, then the entire remainder of the function must be duplicated, with one version to handle the case that «hooked» is false, and one version to handle the case that «hooked» is true.

For example, if you have:
(defun set-internal-1 (args)
  (block nil ; Because Elisp isn't CL
    (if (constant-p) ; This is the «if» near the start of set_internal_1
	(if (forbidden-p)
	    (error "setting constant")
	  (return)))
    (do-some-stuff)
    (and-many-more-lines-of-stuff)
    (set-some-variable)
    (if hooked (run-varhook)))) ; This is what try_run_varhook does

Your goal is to avoid the «if» at the end. In order to do that, you'd need two functions:
(defun set-internal-1 (args)
  (block nil
    (if (constant-p)
	(if hooked ; Mutually exclusive with actual constant
	    (progn
	      (set-internal-1-and-run-varhook args)
	      (return))
	  (if (forbidden-p)
	      (error "setting constant")
	    (return))))
    (do-some-stuff)
    (and-many-more-lines-of-stuff)
    (set-some-variable)))

(defun set-internal-1-and-run-varhook (args)
  (do-some-stuff)
  (and-many-more-lines-of-stuff)
  (set-some-variable)
  (run-varhook))

You can't factor the common parts out to a separate function, because function call overhead would be even greater than the conditional-branch overhead of try_run_varhook, which would defeat the purpose of putting «hooked» into «constant». I guess you could factor the common parts out to an _inline_ function, assuming the compiler would follow your advice to inline such a big function, but it would still be duplicated in the compiled code.

If the performance of Emacs would really be measurably affected by that «if» (but as I've shown, it isn't, even in the worst case), I see three other ways to optimize set_internal that together would be more effective than eliminating the «if»:
0. Untag «symbol» once, rather than twice (currently it's done in SYMBOL_CONSTANT_P and again to set «sym»).
1. Delay the setting of «voide», since it's not needed in the case of SYMBOL_PLAINVAL.
2. Swap the order of the SYMBOL_PLAINVAL and SYMBOL_VARALIAS cases of the «switch» statement, which would avoid an unnecessary conditional branch before the SYMBOL_PLAINVAL case (which is the common case). IIUC, putting SYMBOL_VARALIAS before SYMBOL_PLAINVAL (which is how the code is now) is just as expensive as my branch that you're objecting to.

But if you still want me to eliminate my branch, I'll do it.

> This way the hook can see the value before the assignment
> (very useful for watchpoint-style debugging) and can decide to block the
> assignment altogether (e.g. let's say you want to prevent assignment of
> values of the wrong type to a particular variable; or you want to
> implement the "defconst makes the variable read-only", ...).

I see. Yes, that would make varhook more useful. I'll implement it.

> The real question is: why do it otherwise?
>
> I happen to have an answer for that question as well: because, if
> the assignment calls the hook, how does the hook perform the assignment?
> (I.e. we need some way to perform the assignment without triggering the
> hook)

That's easy even in the current version of varhook: in your handler function, just unhook the symbol before performing the assignment, then re-hook it before the handler exits.

But I'll change it so even that isn't necessary.

> Ideally this is done by having two layers: a top-layer that triggers the
> hook(s) and a bottom layer that performs a "raw" access.  That's how
> I did it for defalias-vs-fset, but for setq we don't have 2 layers yet,
> so either we need to add a new layer (and since we want the hook to
> trigger on plain old `setq' it means the new layer has to be the "raw"
> assignment).

I think I have an ideal solution in mind. But I'll try to implement it tomorrow before posting a description, so I can deny I ever had this idea if it turns out to be stupid.

>> Ok.  However (just a nitpick), using ⌜symbol-watch-set⌝, ⌜symbol-watch-unset⌝
>> and ⌜symbol-watch-p⌝ would be more accurate,
>
> Fine by me.

I'd like to bikeshed the function names a bit more. Handler functions for varhook are already capable of not only reading variables, but also writing them. And now, writing (or blocking of writing) is actually going to be one of the intended use cases. That means ‟watch” is a misleading way of describing it; watching (which implies only reading) is only one of the things that a handler might do.

So now, I recommend ⌜symbol-hook-set⌝, ⌜symbol-hook-unset⌝, and ⌜symbol-hooked-p⌝. (Actually, I prefer ⌜symbol-hook⌝ and ⌜symbol-unhook⌝, but if these are too short then the others are ok.)

>> But ⌜variable-watch-function⌝ is not misleading, since it doesn't suggest
>> that it applies to just one particular variable.  It applies to all
>> hooked variables.
>
> But please use `symbol-watch-function' for consistency.

And now, for consistency, it would be ⌜symbol-hook-function⌝, but with the planned interface change to use add-function instead of add-hook, it would be more descriptive to call it ⌜symbol-mutate-function⌝ (from the sense of ‟mutable variable”, since it's run when a hooked variable is mutated). This name also has the advantage of eliciting visions of your program creating monsters.

BTW, why is the order of the first two arguments to advice-add reversed from add-function? And the words in their names too.