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

[Kelly Dean]

Stefan Monnier wrote:
> I thought it was clear: so that the new feature does not cost anything
> as long as you don't use it.  I.e. that extends existing tests of
> "constant is 0" to mean both "not constant" and "not hooked either".

Oh, I see. Yes, that would avoid the extra conditional branch in specbind (but 
not in unbind_to).

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. The first version would be the same 
as now (without my patch), except it would call the second version if 
«constant» indicates «hooked». The second version would be the same as now 
(_with_ my patch), except with the «constant» check omitted and the three calls 
to try_run_varhook replaced by run_varhook.

> Currently `constant' occupies two bits but is used as
> a boolean value.

The definition of Lisp_Symbol says about «constant», ‟If the value is 3, then 
the var can be changed, but only by `defconst'.” I assumed that comment was 
accurate.

But I grepped for ⌜->constant⌝ and ⌜SYMBOL_CONSTANT_P⌝ just now, and it looks 
like the comment is wrong. I guess it is/was a planned feature.

> So there's a bit available right there.  And if not,
> you can add a bit to that field.

Ok.

> I don't understand your example: what's `varhook'?  What's `hook'?
> What's `unhook'?

The ⌜varhook⌝ symbol is the hook (an ordinary hook) that's run when hooked 
symbols are set. The «hook» function sets the «hooked» bit, «unhook» clears 
that bit, and «hookedp» returns it. All documented in the docstrings in my 
patch, but I guess my example usage should mention it at the beginning.

> The hook should be called *to perform the assignment*, rather than
> calling it after performing the assignment ourselves.

I'm confused. You mean if a symbol is hooked, then when you attempt to set it 
(using the usual setq, etc), the hook should be run _instead_ of actually 
setting the symbol?

That would give the hook the option to perform the requested assignment, or 
instead perform a different assignment, or none at all. I don't see the reason 
for doing this.

Or do you mean the higher-level feature you talked about a few days ago, where 
hooking has no effect on setq, etc, and you have to use setq-with-hook instead 
of setq if you want the hook to run? That wouldn't be useful for debugging and 
profiling, which are the current goals.

> And it should not use run_hook* but funcall (i.e. the hook should be
> manipulated in Elisp via `add-function' rather than via `add-hook').

That would make sense if the hook replaces the actual assignment, like «around» 
advice can replace a function. But I don't see why to do this.

I'm sure I must have misunderstood what you meant.

Or did you mean just to enable writing a handler function that lets you pause 
on a variable, manually set some value (different from what the original 
program set) as an experiment while debugging, then continue? You can already 
do that (it doesn't matter that varhook lets the original program set the 
value, because the handler runs afterward, so you can override what the program 
set, before resuming the program); just make sure your handler temporarily 
unhooks the variable before setting it, to avoid infinite recursion.

>> -extern Lisp_Object run_hook_with_args (ptrdiff_t nargs, Lisp_Object *args,
>> -                                   Lisp_Object (*funcall)
>> -                                   (ptrdiff_t nargs, Lisp_Object *args));
>> +extern Lisp_Object run_hook_with_args (ptrdiff_t, Lisp_Object *,
>> +                                   Lisp_Object (*) (ptrdiff_t, Lisp_Object 
>> *));
>
> What for?

A code cleanup that I included in this patch by accident. (I forgot that I only 
added the next function signature, not this one, then I edited both to be 
consistent with the surrounding code because I thought I added both.) I missed 
it when reviewing the patch because, as is well-known, I can't read. ;-)

I'll remove it.

>>       if (!sym->constant)
>> -    SET_SYMBOL_VAL (sym, value);
>> +    {
>> +      SET_SYMBOL_VAL (sym, value);
>> +      try_run_varhook (sym, false, Dyn_Bind, value);
>> +    }
>
> You just slowed down all the code even if the feature is not used.
> I think we can do better.

This is in specbind. Yes, putting the «hooked» bit into the «constant» field 
could avoid this extra conditional branch.

>>          if (sym->redirect =3D=3D SYMBOL_PLAINVAL)
>>            {
>> -            SET_SYMBOL_VAL (sym, specpdl_old_value (specpdl_ptr));
>> +            Lisp_Object oldval =3D specpdl_old_value (specpdl_ptr);
>> +            SET_SYMBOL_VAL (sym, oldval);
>> +            try_run_varhook (sym, false, Dyn_Unbind, oldval);
>
> Same here.

This is in unbind_to, which doesn't already check the «constant» field, so even 
if «hooked» were put into «constant», it would save nothing here; it would just 
result in adding a check of «constant» rather than adding a check of the 
separate «hooked» bit.

But remember that try_run_varhook is inlined, and the target of the «sym» 
pointer is already in L1 cache at this point, so if the «hooked» bit isn't set, 
all that's executed here is a conditional branch on that bit. Well and I guess 
an «and» mask to extract it from the bit field (no bit shift needed, since it's 
just used for comparison to zero). There's no function call or main memory 
access.

I'm not very familiar with CPU branch prediction, but IIUC, it will result in 
almost all executions of try_run_varhook (in specbind, unbind_to, 
set_internal_1, and set_default_internal) being as fast as a «jump» 
instruction--one CPU cycle, or maybe less on superscalar CPUs (not very 
familiar with this either).

So I think even if «hooked» is left as a separate bit, it will cost practically 
nothing extra compared to making it be part of «constant» (and the latter would 
increase complexity and require code duplication). Is there someone on the 
mailing list who understands branch prediction well and can comment?

> I think I'd rather turn `dir' into a Lisp_Object (a symbol) and send
> this directly to the hook.  And then, if/when needed we can try and
> make available to Elisp the functionality of things like
> let_shadows_*_binding_p.

Ok. I'd also have to pass buf_local, so that the hook would have the necessary 
information to replicate what run_varhook currently does. But I don't 
understand what the advantage would be, compared to how run_varhook currently 
works.

> BTW in order to keep the performance cost of this patch to a minimum, we
> may end up reducing its precision.

I think even the current implementation might have close to zero performance 
cost. But I haven't tested to confirm this.

>> +  DEFSYM (Qvarhook, "varhook");
>
> Ah, here it is.  Please use a name more along the lines of Elisp style.
> E.g. `variable-watch-function'.
>
>> +  defsubr (&Shook);
>> +  defsubr (&Sunhook);
>> +  defsubr (&Shookedp);
>
> Use less generic names.  E.g. `variable-watch-set',
> `variable-watch-unset', and `variable-watch-p'.

Ok. However (just a nitpick), using ⌜symbol-watch-set⌝, ⌜symbol-watch-unset⌝ 
and ⌜symbol-watch-p⌝ would be more accurate, because those functions set/check 
the «hooked» bit not just for a particular variable, but for all non-lexical 
variables of the specified symbol (i.e. all occurrences of the symbol in all 
non-lexical run-time environments: global, buffer-local, and dynamic). Using 
⌜variable⌝ in those functions' names would imply that individual variables of a 
symbol can be hooked/unhooked, when in fact varhook doesn't support that. It 
only supports all-or-none.

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.

It's unfortunate that in Emacs, an instance of the Lisp_Symbol structure, which 
records a representation of (and information associated with) a symbol, is 
itself called a ‟symbol”. This is probably why people say symbols don't occur 
in multiple environments.