From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Kelly Dean Newsgroups: gmane.emacs.devel Subject: Re: [PATCH] (Updated) Run hook when variable is set Date: Fri, 06 Feb 2015 05:34:41 +0000 Message-ID: References: NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Trace: ger.gmane.org 1423200984 9021 80.91.229.3 (6 Feb 2015 05:36:24 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Fri, 6 Feb 2015 05:36:24 +0000 (UTC) Cc: emacs-devel@gnu.org To: Stefan Monnier Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Fri Feb 06 06:36:24 2015 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by plane.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1YJbaX-0007bx-AR for ged-emacs-devel@m.gmane.org; Fri, 06 Feb 2015 06:36:21 +0100 Original-Received: from localhost ([::1]:46765 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YJbaV-0008L6-2D for ged-emacs-devel@m.gmane.org; Fri, 06 Feb 2015 00:36:19 -0500 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:43305) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YJbaG-0008L0-59 for emacs-devel@gnu.org; Fri, 06 Feb 2015 00:36:05 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YJbaC-0002Yi-Qw for emacs-devel@gnu.org; Fri, 06 Feb 2015 00:36:04 -0500 Original-Received: from relay3-d.mail.gandi.net ([2001:4b98:c:538::195]:51272) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YJbaC-0002YY-HM for emacs-devel@gnu.org; Fri, 06 Feb 2015 00:36:00 -0500 Original-Received: from mfilter22-d.gandi.net (mfilter22-d.gandi.net [217.70.178.150]) by relay3-d.mail.gandi.net (Postfix) with ESMTP id 6E96DA80B4; Fri, 6 Feb 2015 06:35:59 +0100 (CET) X-Virus-Scanned: Debian amavisd-new at mfilter22-d.gandi.net Original-Received: from relay3-d.mail.gandi.net ([217.70.183.195]) by mfilter22-d.gandi.net (mfilter22-d.gandi.net [10.0.15.180]) (amavisd-new, port 10024) with ESMTP id wGhzxGei8+Cr; Fri, 6 Feb 2015 06:35:57 +0100 (CET) X-Originating-IP: 66.220.3.179 Original-Received: from localhost (gm179.geneticmail.com [66.220.3.179]) (Authenticated sender: kelly@prtime.org) by relay3-d.mail.gandi.net (Postfix) with ESMTPSA id 104D3A80AB; Fri, 6 Feb 2015 06:35:54 +0100 (CET) In-Reply-To: X-detected-operating-system: by eggs.gnu.org: Error: Malformed IPv6 address (bad octet value). X-Received-From: 2001:4b98:c:538::195 X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Original-Sender: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Xref: news.gmane.org gmane.emacs.devel:182494 Archived-At: 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 functio= n (or set_internal_1 in my patch) would require duplicating the entire fu= nction, 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 sec= ond version if =C2=ABconstant=C2=BB indicates =C2=ABhooked=C2=BB. The sec= ond version would be the same as now (_with_ my patch), except with the =C2= =ABconstant=C2=BB check omitted and the three calls to try_run_varhook re= placed by run_varhook. > Currently `constant' occupies two bits but is used as > a boolean value. The definition of Lisp_Symbol says about =C2=ABconstant=C2=BB, =E2=80=9FI= f the value is 3, then the var can be changed, but only by `defconst'.=E2= =80=9D I assumed that comment was accurate. But I grepped for =E2=8C=9C->constant=E2=8C=9D and =E2=8C=9CSYMBOL_CONSTA= NT_P=E2=8C=9D just now, and it looks like the comment is wrong. I guess i= t 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 =E2=8C=9Cvarhook=E2=8C=9D symbol is the hook (an ordinary hook) that'= s run when hooked symbols are set. The =C2=ABhook=C2=BB function sets the= =C2=ABhooked=C2=BB bit, =C2=ABunhook=C2=BB clears that bit, and =C2=ABho= okedp=C2=BB 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 se= t it (using the usual setq, etc), the hook should be run _instead_ of act= ually 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 th= e 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-h= ook 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 =C2= =ABaround=C2=BB 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 p= ause on a variable, manually set some value (different from what the orig= inal program set) as an experiment while debugging, then continue? You ca= n already do that (it doesn't matter that varhook lets the original progr= am 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 infi= nite 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 bot= h to be consistent with the surrounding code because I thought I added bo= th.) I missed it when reviewing the patch because, as is well-known, I ca= n'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 =C2=ABhooked=C2=BB bit into the =C2= =ABconstant=C2=BB field could avoid this extra conditional branch. >> if (sym->redirect =3D3D=3D3D SYMBOL_PLAINVAL) >> { >> - SET_SYMBOL_VAL (sym, specpdl_old_value (specpdl_ptr)); >> + Lisp_Object oldval =3D3D 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 =C2=ABconstant=C2=BB= field, so even if =C2=ABhooked=C2=BB were put into =C2=ABconstant=C2=BB,= it would save nothing here; it would just result in adding a check of =C2= =ABconstant=C2=BB rather than adding a check of the separate =C2=ABhooked= =C2=BB bit. But remember that try_run_varhook is inlined, and the target of the =C2=AB= sym=C2=BB pointer is already in L1 cache at this point, so if the =C2=ABh= ooked=C2=BB bit isn't set, all that's executed here is a conditional bran= ch on that bit. Well and I guess an =C2=ABand=C2=BB mask to extract it fr= om the bit field (no bit shift needed, since it's just used for compariso= n to zero). There's no function call or main memory access. I'm not very familiar with CPU branch prediction, but IIUC, it will resul= t in almost all executions of try_run_varhook (in specbind, unbind_to, se= t_internal_1, and set_default_internal) being as fast as a =C2=ABjump=C2=BB= instruction--one CPU cycle, or maybe less on superscalar CPUs (not very = familiar with this either). So I think even if =C2=ABhooked=C2=BB is left as a separate bit, it will = cost practically nothing extra compared to making it be part of =C2=ABcon= stant=C2=BB (and the latter would increase complexity and require code du= plication). Is there someone on the mailing list who understands branch p= rediction 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 nece= ssary information to replicate what run_varhook currently does. But I don= 't understand what the advantage would be, compared to how run_varhook cu= rrently works. > BTW in order to keep the performance cost of this patch to a minimum, w= e > may end up reducing its precision. I think even the current implementation might have close to zero performa= nce 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 =E2=8C=9Csymbol-watch-set=E2=8C=9D, =E2= =8C=9Csymbol-watch-unset=E2=8C=9D and =E2=8C=9Csymbol-watch-p=E2=8C=9D wo= uld be more accurate, because those functions set/check the =C2=ABhooked=C2= =BB bit not just for a particular variable, but for all non-lexical varia= bles of the specified symbol (i.e. all occurrences of the symbol in all n= on-lexical run-time environments: global, buffer-local, and dynamic). Usi= ng =E2=8C=9Cvariable=E2=8C=9D in those functions' names would imply that = individual variables of a symbol can be hooked/unhooked, when in fact var= hook doesn't support that. It only supports all-or-none. But =E2=8C=9Cvariable-watch-function=E2=8C=9D is not misleading, since it= doesn't suggest that it applies to just one particular variable. It appl= ies 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 sy= mbol, is itself called a =E2=80=9Fsymbol=E2=80=9D. This is probably why p= eople say symbols don't occur in multiple environments.