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

[Kelly Dean <kelly@prtime.org>]

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.