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

[Kelly Dean]

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.