From: Kelly Dean <kelly@prtime.org>
To: Stefan Monnier <monnier@IRO.UMontreal.CA>
Cc: emacs-devel@gnu.org
Subject: Re: [PATCH] (Updated) Run hook when variable is set
Date: Sat, 07 Feb 2015 12:27:06 +0000 [thread overview]
Message-ID: <WmN9nhPIb2lg2d4VeXtEYOxMYvsjN3Xk9HkeAAqK1tJ@local> (raw)
In-Reply-To: <jwvoap7169o.fsf-monnier+emacs@gnu.org>
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.
next prev parent reply other threads:[~2015-02-07 12:27 UTC|newest]
Thread overview: 110+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-04-20 2:54 Proposal to change cursor appearance to indicate region activation Kelly Dean
2013-04-20 7:23 ` Drew Adams
2015-01-22 5:38 ` [PATCH] " Kelly Dean
2015-01-22 14:25 ` Stefan Monnier
2015-01-23 3:08 ` [PATCH] " Kelly Dean
2015-01-23 4:55 ` Stefan Monnier
2015-01-23 11:07 ` Kelly Dean
2015-01-23 17:49 ` Drew Adams
2015-01-24 3:06 ` Kelly Dean
2015-01-24 4:52 ` Stefan Monnier
2015-01-24 9:22 ` Kelly Dean
2015-01-25 14:29 ` Stefan Monnier
2015-01-28 9:15 ` [PATCH] Run hook when variable is set Kelly Dean
2015-01-28 9:23 ` [PATCH] Proposal to change cursor appearance to indicate region activation Kelly Dean
2015-01-28 11:24 ` David Kastrup
2015-01-28 12:13 ` David Kastrup
2015-01-29 10:46 ` Kelly Dean
2015-01-29 11:16 ` David Kastrup
2015-01-30 7:20 ` Kelly Dean
2015-01-30 9:19 ` David Kastrup
2015-01-30 10:05 ` Kelly Dean
2015-01-30 10:12 ` David Kastrup
2015-01-30 9:43 ` Kelly Dean
2015-01-28 19:25 ` [PATCH] Run hook when variable is set Stefan Monnier
2015-01-29 8:20 ` Kelly Dean
2015-01-29 8:28 ` Lars Ingebrigtsen
2015-01-29 14:58 ` Stefan Monnier
2015-01-30 7:34 ` Kelly Dean
2015-01-30 15:55 ` Stefan Monnier
2015-01-31 9:18 ` Kelly Dean
2015-01-31 20:48 ` Stefan Monnier
2015-02-02 5:40 ` Kelly Dean
2015-02-02 15:57 ` Stefan Monnier
2015-02-03 19:56 ` Kelly Dean
2015-02-03 22:49 ` Stefan Monnier
2015-02-05 3:10 ` [PATCH] (Updated) " Kelly Dean
2015-02-05 13:57 ` Stefan Monnier
2015-02-06 5:34 ` Kelly Dean
2015-02-06 14:42 ` Stefan Monnier
2015-02-07 12:27 ` Kelly Dean [this message]
2015-02-07 15:09 ` Stefan Monnier
2015-02-09 3:24 ` Kelly Dean
2015-02-12 19:58 ` Stefan Monnier
2015-02-13 23:08 ` Kelly Dean
2015-02-14 0:55 ` Stefan Monnier
2015-02-14 22:19 ` Kelly Dean
2015-02-15 20:25 ` Stefan Monnier
2015-02-17 2:22 ` Kelly Dean
2015-02-17 23:07 ` Richard Stallman
2015-02-18 3:19 ` The purpose of makunbound (Was: Run hook when variable is set) Kelly Dean
2015-02-18 5:48 ` The purpose of makunbound Stefan Monnier
2015-02-18 8:51 ` Kelly Dean
2015-02-18 14:34 ` Stefan Monnier
2015-02-18 18:53 ` Kelly Dean
2015-02-18 22:42 ` Stefan Monnier
2015-02-19 10:36 ` Kelly Dean
2015-02-22 0:18 ` Kelly Dean
2015-02-19 10:45 ` Kelly Dean
2015-02-19 13:33 ` Stefan Monnier
2015-02-19 23:51 ` Kelly Dean
2015-02-20 1:59 ` Stefan Monnier
2015-02-20 9:35 ` Kelly Dean
2015-02-20 16:55 ` Stefan Monnier
2015-02-20 2:58 ` Stephen J. Turnbull
2015-02-20 0:56 ` Richard Stallman
2015-02-20 9:02 ` Kelly Dean
2015-02-20 15:41 ` Richard Stallman
2015-02-21 5:45 ` Stephen J. Turnbull
2015-02-22 0:32 ` Kelly Dean
2015-02-22 8:45 ` Andreas Schwab
2015-02-18 5:15 ` [PATCH] (Updated) Run hook when variable is set Kelly Dean
2015-02-18 22:37 ` Stefan Monnier
2015-02-18 22:37 ` Stefan Monnier
2015-02-19 10:35 ` Kelly Dean
2015-02-19 13:30 ` Stefan Monnier
2015-02-20 6:48 ` Kelly Dean
2015-02-20 19:29 ` Stefan Monnier
2015-02-21 14:18 ` Kelly Dean
2015-02-21 20:51 ` Stefan Monnier
2015-02-22 0:32 ` Kelly Dean
2015-02-22 10:40 ` Stephen J. Turnbull
2015-02-22 21:35 ` Stefan Monnier
2015-02-23 3:09 ` Kelly Dean
2015-02-23 4:19 ` Stefan Monnier
2015-02-20 20:27 ` Proposal for debugging/testing option Kelly Dean
2015-02-24 16:28 ` Stefan Monnier
2015-02-14 20:37 ` [PATCH] (Updated) Run hook when variable is set Johan Bockgård
2015-02-15 19:36 ` Stefan Monnier
2015-02-15 19:53 ` Patches: inline vs. attachment, compressed vs. uncompressed. [was: Run hook when variable is set] Alan Mackenzie
2015-02-06 9:55 ` [PATCH] (Updated) Run hook when variable is set Kelly Dean
2015-01-30 23:29 ` [PATCH] " Richard Stallman
2015-01-31 9:23 ` Kelly Dean
2015-01-31 23:16 ` Richard Stallman
2015-02-02 5:41 ` Kelly Dean
2015-02-01 2:04 ` Alexis
2015-02-01 4:05 ` Stefan Monnier
2015-02-01 8:58 ` David Kastrup
2015-01-29 16:06 ` Eli Zaretskii
2015-01-30 7:14 ` Kelly Dean
2015-01-30 9:08 ` Eli Zaretskii
2015-01-23 20:34 ` [PATCH] Proposal to change cursor appearance to indicate region activation Stefan Monnier
2015-01-24 0:25 ` Kelly Dean
2015-01-23 10:01 ` Tassilo Horn
2015-01-23 17:49 ` Drew Adams
2015-01-23 10:06 ` Eli Zaretskii
2015-01-23 11:40 ` Kelly Dean
2015-01-23 11:56 ` Eli Zaretskii
2015-01-22 5:41 ` Kelly Dean
2013-11-23 13:34 ` Stefan Monnier
2013-11-23 20:25 ` Drew Adams
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=WmN9nhPIb2lg2d4VeXtEYOxMYvsjN3Xk9HkeAAqK1tJ@local \
--to=kelly@prtime.org \
--cc=emacs-devel@gnu.org \
--cc=monnier@IRO.UMontreal.CA \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this external index
https://git.savannah.gnu.org/cgit/emacs.git
https://git.savannah.gnu.org/cgit/emacs/org-mode.git
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.