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: Sat, 07 Feb 2015 12:27:06 +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 1423312145 7277 80.91.229.3 (7 Feb 2015 12:29:05 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Sat, 7 Feb 2015 12:29:05 +0000 (UTC) Cc: emacs-devel@gnu.org To: Stefan Monnier Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Sat Feb 07 13:29:01 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 1YK4VQ-0007JY-Ra for ged-emacs-devel@m.gmane.org; Sat, 07 Feb 2015 13:29:01 +0100 Original-Received: from localhost ([::1]:52605 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YK4VQ-0004fg-8u for ged-emacs-devel@m.gmane.org; Sat, 07 Feb 2015 07:29:00 -0500 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:57507) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YK4V0-0004VG-F2 for emacs-devel@gnu.org; Sat, 07 Feb 2015 07:28:35 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YK4Ux-0005a7-3N for emacs-devel@gnu.org; Sat, 07 Feb 2015 07:28:34 -0500 Original-Received: from relay3-d.mail.gandi.net ([2001:4b98:c:538::195]:42296) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YK4Uw-0005Yi-QS for emacs-devel@gnu.org; Sat, 07 Feb 2015 07:28:31 -0500 Original-Received: from mfilter1-d.gandi.net (mfilter1-d.gandi.net [217.70.178.130]) by relay3-d.mail.gandi.net (Postfix) with ESMTP id 712E8A80B8; Sat, 7 Feb 2015 13:28:26 +0100 (CET) X-Virus-Scanned: Debian amavisd-new at mfilter1-d.gandi.net Original-Received: from relay3-d.mail.gandi.net ([217.70.183.195]) by mfilter1-d.gandi.net (mfilter1-d.gandi.net [10.0.15.180]) (amavisd-new, port 10024) with ESMTP id o7SiE4KXuujz; Sat, 7 Feb 2015 13:28:24 +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 9474CA80AD; Sat, 7 Feb 2015 13:28:22 +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:182597 Archived-At: Stefan Monnier wrote: >> But avoiding the conditional branch that way for the set_internal func= tion >> (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; t= he new name is just to avoid having to change other code) has try_run_var= hook in three places: one for each of the cases of =E2=8C=9Cswitch (sym->redirect)=E2=8C=9D except SYMBOL_VARALIAS. The firs= t three things that set_internal_1 does are: set =C2=ABvoide=C2=BB check =C2=ABsymbol=C2=BB check the =C2=ABconstant=C2=BB bit, using =E2=8C=9Cif (SYMBOL_CONSTANT_P = (symbol))=E2=8C=9D If =C2=ABhooked=C2=BB is put into =C2=ABconstant=C2=BB, and checked only = within that =C2=ABif=C2=BB block at the beginning of set_internal_1, then= the entire remainder of the function must be duplicated, with one versio= n to handle the case that =C2=ABhooked=C2=BB is false, and one version to= handle the case that =C2=ABhooked=C2=BB 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 =C2=ABif=C2=BB near the start of set_i= nternal_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 =C2=ABif=C2=BB 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 fun= ction call overhead would be even greater than the conditional-branch ove= rhead of try_run_varhook, which would defeat the purpose of putting =C2=AB= hooked=C2=BB into =C2=ABconstant=C2=BB. I guess you could factor the comm= on parts out to an _inline_ function, assuming the compiler would follow = your advice to inline such a big function, but it would still be duplicat= ed in the compiled code. If the performance of Emacs would really be measurably affected by that =C2= =ABif=C2=BB (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 eff= ective than eliminating the =C2=ABif=C2=BB: 0. Untag =C2=ABsymbol=C2=BB once, rather than twice (currently it's done = in SYMBOL_CONSTANT_P and again to set =C2=ABsym=C2=BB). 1. Delay the setting of =C2=ABvoide=C2=BB, since it's not needed in the c= ase of SYMBOL_PLAINVAL. 2. Swap the order of the SYMBOL_PLAINVAL and SYMBOL_VARALIAS cases of the= =C2=ABswitch=C2=BB statement, which would avoid an unnecessary condition= al branch before the SYMBOL_PLAINVAL case (which is the common case). IIU= C, 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 th= e > 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 funct= ion, just unhook the symbol before performing the assignment, then re-hoo= k 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 th= e > 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 to= morrow before posting a description, so I can deny I ever had this idea i= f it turns out to be stupid. >> 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 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 writ= ing them. And now, writing (or blocking of writing) is actually going to = be one of the intended use cases. That means =E2=80=9Fwatch=E2=80=9D is a= misleading way of describing it; watching (which implies only reading) i= s only one of the things that a handler might do. So now, I recommend =E2=8C=9Csymbol-hook-set=E2=8C=9D, =E2=8C=9Csymbol-ho= ok-unset=E2=8C=9D, and =E2=8C=9Csymbol-hooked-p=E2=8C=9D. (Actually, I pr= efer =E2=8C=9Csymbol-hook=E2=8C=9D and =E2=8C=9Csymbol-unhook=E2=8C=9D, b= ut if these are too short then the others are ok.) >> 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 applies to all >> hooked variables. > > But please use `symbol-watch-function' for consistency. And now, for consistency, it would be =E2=8C=9Csymbol-hook-function=E2=8C= =9D, but with the planned interface change to use add-function instead of= add-hook, it would be more descriptive to call it =E2=8C=9Csymbol-mutate= -function=E2=8C=9D (from the sense of =E2=80=9Fmutable variable=E2=80=9D,= since it's run when a hooked variable is mutated). This name also has th= e advantage of eliciting visions of your program creating monsters. BTW, why is the order of the first two arguments to advice-add reversed f= rom add-function? And the words in their names too.