From: Andrea Corallo <acorallo@gnu.org>
To: Jens Schmidt <jschmidt4gnu@vodafonemail.de>
Cc: 67005@debbugs.gnu.org, stefan monnier <monnier@iro.umontreal.ca>
Subject: bug#67005: 30.0.50; improve nadivce/comp/trampoline handling
Date: Mon, 13 Nov 2023 04:54:23 -0500 [thread overview]
Message-ID: <yp1msvi6ks0.fsf@fencepost.gnu.org> (raw)
In-Reply-To: <874jhv9921.fsf@sappc2.fritz.box> (Jens Schmidt's message of "Wed, 08 Nov 2023 23:25:42 +0100")
Jens Schmidt <jschmidt4gnu@vodafonemail.de> writes:
> X-Debbugs-Cc: Stefan Monnier <monnier@iro.umontreal.ca>, Andrea Corallo <acorallo@gnu.org>
> Severity: minor
>
>
> While digging around in nadvice.el, mainly still because of
> bug#66032, I came across the special-cased handling of
> `rename-buffer' and `macroexpand' in `advice--add-function'.
>>From there I came to `native-comp-never-optimize-functions' in
> comp.el. So I started fiddeling away and came to the conclusion
> that these are probably no longer needed, and then I found more
> issues ... please check whether my reasoning below is correct.
>
> The attached patch is probably a nice TL;DR.
>
>
> Some notes:
>
> - Nothing described below is really a bug (except probably item
> 4b), but fixing these issues should improve long-term
> maintainability, I think.
>
> - All patches have been built on (somewhat randomly chosen)
> commit b819b8d6e90337b4cb36b35c2c6d0112c90a8e24, just to have a
> reference that builds and tests fine on my system.
>
> - When I write "bootstrap & check: OK" below I mean the obvious: I did a
> "make bootstrap && make check" and that came through without errors.
> This is for sure no proof of correctness, but hopefully at least an
> indication that I'm not completely wrong.
>
> - All diffs in 1, ..., 6 are to be successively cumulated, while the
> attached patch is again to be applied directly on commit
> b819b8d6e90337b4cb36b35c2c6d0112c90a8e24.
>
> - When whatever changes have been decided on, I will be glad to
> write unit tests for whatever makes sense to test.
>
> - It's a lengthy essay, sorry. I hope it's worth reading, though.
>
>
> 1. Since Stefan has removed the advices on uniquify functions in
> 1a724cc2d2e7, that special handling of `rename-buffer' in
> `advice--add-function' and `native-comp-never-optimize-functions'
> seems to be obsolete. Away with it:
>
> ------------------------- snip(1) -------------------------
> diff --git a/lisp/emacs-lisp/comp.el b/lisp/emacs-lisp/comp.el
> index 7fd9543d2ba..d94c19c20fa 100644
> --- a/lisp/emacs-lisp/comp.el
> +++ b/lisp/emacs-lisp/comp.el
> @@ -119,10 +119,9 @@ native-comp-bootstrap-deny-list
> :version "28.1")
>
> (defcustom native-comp-never-optimize-functions
> - '(;; The following two are mandatory for Emacs to be working
> - ;; correctly (see comment in `advice--add-function'). DO NOT
> - ;; REMOVE.
> - macroexpand rename-buffer)
> + '(;; The following is mandatory for Emacs to be working correctly
> + ;; (see comment in `advice--add-function'). DO NOT REMOVE.
> + macroexpand)
> "Primitive functions to exclude from trampoline optimization.
>
> Primitive functions included in this list will not be called
> diff --git a/lisp/emacs-lisp/nadvice.el b/lisp/emacs-lisp/nadvice.el
> index ce5467f3c5c..946ca43f1cf 100644
> --- a/lisp/emacs-lisp/nadvice.el
> +++ b/lisp/emacs-lisp/nadvice.el
> @@ -397,14 +397,12 @@ advice--add-function
> (subr-primitive-p (gv-deref ref)))
> (let ((subr-name (intern (subr-name (gv-deref ref)))))
> ;; Requiring the native compiler to advice `macroexpand' cause a
> - ;; circular dependency in eager macro expansion. uniquify is
> - ;; advising `rename-buffer' while being loaded in loadup.el.
> - ;; This would require the whole native compiler machinery but we
> - ;; don't want to include it in the dump. Because these two
> - ;; functions are already handled in
> - ;; `native-comp-never-optimize-functions' we hack the problem
> - ;; this way for now :/
> - (unless (memq subr-name '(macroexpand rename-buffer))
> + ;; circular dependency in eager macro expansion. This would
> + ;; require the whole native compiler machinery but we don't want
> + ;; to include it in the dump. Because these two functions are
> + ;; already handled in `native-comp-never-optimize-functions' we
> + ;; hack the problem this way for now :/
> + (unless (memq subr-name '(macroexpand))
> ;; Must require explicitly as during bootstrap we have no
> ;; autoloads.
> (require 'comp)
> ------------------------- snip(1) -------------------------
>
> => bootstrap & check: OK
+1
>
> 2. The comment in `advice--add-function' says that `macroexpand' causes
> a circular dependency, and there *are* still advices done on
> `macroexpand' (at least in `cl-symbol-macrolet'). But probably these
> are not executed during the bootstrap? Let's try.
>
> ------------------------- snip(2) -------------------------
> diff --git a/lisp/emacs-lisp/comp.el b/lisp/emacs-lisp/comp.el
> index d94c19c20fa..5bbbabe548f 100644
> --- a/lisp/emacs-lisp/comp.el
> +++ b/lisp/emacs-lisp/comp.el
> @@ -119,9 +119,7 @@ native-comp-bootstrap-deny-list
> :version "28.1")
>
> (defcustom native-comp-never-optimize-functions
> - '(;; The following is mandatory for Emacs to be working correctly
> - ;; (see comment in `advice--add-function'). DO NOT REMOVE.
> - macroexpand)
> + '()
> "Primitive functions to exclude from trampoline optimization.
>
> Primitive functions included in this list will not be called
+1
> diff --git a/lisp/emacs-lisp/nadvice.el b/lisp/emacs-lisp/nadvice.el
> index 946ca43f1cf..e1945cc2b1b 100644
> --- a/lisp/emacs-lisp/nadvice.el
> +++ b/lisp/emacs-lisp/nadvice.el
> @@ -396,17 +396,7 @@ advice--add-function
> (when (and (featurep 'native-compile)
> (subr-primitive-p (gv-deref ref)))
> (let ((subr-name (intern (subr-name (gv-deref ref)))))
> - ;; Requiring the native compiler to advice `macroexpand' cause a
> - ;; circular dependency in eager macro expansion. This would
> - ;; require the whole native compiler machinery but we don't want
> - ;; to include it in the dump. Because these two functions are
> - ;; already handled in `native-comp-never-optimize-functions' we
> - ;; hack the problem this way for now :/
> - (unless (memq subr-name '(macroexpand))
> - ;; Must require explicitly as during bootstrap we have no
> - ;; autoloads.
> - (require 'comp)
> - (comp-subr-trampoline-install subr-name))))
> + (comp-subr-trampoline-install subr-name)))
> (let* ((name (cdr (assq 'name props)))
> (a (advice--member-p (or name function) (if name t) (gv-deref ref))))
> (when a
> ------------------------- snip(2) -------------------------
>
> => bootstrap & check: OK
+1
> 3. But then on the other hand, function Ffset also calls
> `comp-subr-trampoline-install', and if we have condition
>
> `(subr-primitive-p (gv-deref ref))'
>
> fulfilled in `advice--add-function', then the following `(setf
> (gv-deref ref) ...)' should ultimately call `fset'. So probably we
> can completely remove that `comp-subr-trampoline-install' call from
> `advice--add-function', like this:
>
> ------------------------- snip(3) -------------------------
> diff --git a/lisp/emacs-lisp/nadvice.el b/lisp/emacs-lisp/nadvice.el
> index e1945cc2b1b..e7027bb36a7 100644
> --- a/lisp/emacs-lisp/nadvice.el
> +++ b/lisp/emacs-lisp/nadvice.el
> @@ -393,10 +393,6 @@ add-function
>
> ;;;###autoload
> (defun advice--add-function (how ref function props)
> - (when (and (featurep 'native-compile)
> - (subr-primitive-p (gv-deref ref)))
> - (let ((subr-name (intern (subr-name (gv-deref ref)))))
> - (comp-subr-trampoline-install subr-name)))
> (let* ((name (cdr (assq 'name props)))
> (a (advice--member-p (or name function) (if name t) (gv-deref ref))))
> (when a
> ------------------------- snip(3) -------------------------
>
> => bootstrap & check: OK
Unfortanatelly I can't remember why we have this specific handling, I
guess there was a reason but anyway... If there is still a good reason
we'll discover it.
> 4. At this stage, what happens if we reactivate (a different) advice
> on `rename-buffer' in uniquify.el?
>
> ------------------------- snip(4) -------------------------
> diff --git a/lisp/uniquify.el b/lisp/uniquify.el
> index 7119ae7eac3..24d6ccaefad 100644
> --- a/lisp/uniquify.el
> +++ b/lisp/uniquify.el
> @@ -489,6 +489,14 @@ uniquify-kill-buffer-function
> ;; rename-buffer and create-file-buffer. (Setting find-file-hook isn't
> ;; sufficient.)
>
> +(defconst uniquify--rename-buffer-history '())
> +
> +(advice-add 'rename-buffer :around #'uniquify--the-real-rename-buffer-advice)
> +(defun uniquify--the-real-rename-buffer-advice (origfun newname &optional unique)
> + (setq uniquify--rename-buffer-history
> + (cons newname uniquify--rename-buffer-history))
> + (funcall origfun newname unique))
> +
> ;; (advice-add 'rename-buffer :around #'uniquify--rename-buffer-advice)
> (defun uniquify--rename-buffer-advice (newname &optional unique)
> ;; BEWARE: This is called directly from `buffer.c'!
> ------------------------- snip(4) -------------------------
>
> => bootstrap & check: OK
>
> Plus the advice gets properly called, even from natively compiled
> code! Huh?
>
> I think this works without problems since there is already a second
> line of defense as follows:
>
> a) loadup.el sets `native-comp-enable-subr-trampolines' to t only
> after all files have been loaded, so Ffset, which specifically
> tests for `native-comp-enable-subr-trampolines', never will call
> `comp-subr-trampoline-install' during bootstrap.
Ok
> b) As soon as `rename-buffer' got advised (which *is* soon in the
> bootstrap), the test on `subrp' in function
> `comp-call-optim-form-call' evals to nil, since the `subrp' only
> "sees" the surrounding advice, and not the inner primitive. Which
> means that call optimizations won't take place for that.
>
>
> 5. And actually I think that b) above probably is a bug: It means that
> for advised subrs, the optimzation to indirect calls in function
> `comp-call-optim-form-call' never takes place.
>
> So I tried to recognize subrs in function `comp-call-optim-form-call'
> even when they are advised:
>
> ------------------------- snip(5) -------------------------
> diff --git a/lisp/emacs-lisp/comp.el b/lisp/emacs-lisp/comp.el
> index 5bbbabe548f..e60e5a0fa61 100644
> --- a/lisp/emacs-lisp/comp.el
> +++ b/lisp/emacs-lisp/comp.el
> @@ -3409,7 +3409,7 @@ comp-call-optim-form-call
> (gethash callee (comp-ctxt-byte-func-to-func-h comp-ctxt)))
> (not (memq callee native-comp-never-optimize-functions)))
> (let* ((f (if (symbolp callee)
> - (symbol-function callee)
> + (advice--cd*r (symbol-function callee))
Maybe we should make advice--cd*r public if we use it here?
> (cl-assert (byte-code-function-p callee))
> callee))
> (subrp (subrp f))
> @@ -3419,9 +3419,7 @@ comp-call-optim-form-call
> ;; Trampoline removal.
> (let* ((callee (intern (subr-name f))) ; Fix aliased names.
> (maxarg (cdr (subr-arity f)))
> - (call-type (if (if subrp
> - (not (numberp maxarg))
> - (comp-nargs-p comp-func-callee))
> + (call-type (if (not (numberp maxarg))
> 'callref
> 'call))
> (args (if (eq call-type 'callref)
> ------------------------- snip(5) -------------------------
>
> (The second diff in that function is a minor optimization - subrp
> should be always t in that branch of the `cond'.)
>
> => bootstrap & check: OK
>
> But: Natively compiled code now does NOT execute the advice on
> `rename-buffer' added in step 4, since its call gets optimized but no
> trampoline has been created for it.
It's good we optimize the call but we can't apply this patch if the this
issue is not solved.
> 6. So there is the question whether we should actively disallow advices
> during bootstrap, now that we are free of them. Like this:
Others can have different opinions but if it's not a big limitation for
the future it sounds like a good idea to me.
Thanks
Andrea
prev parent reply other threads:[~2023-11-13 9:54 UTC|newest]
Thread overview: 50+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-11-08 22:25 bug#67005: 30.0.50; improve nadivce/comp/trampoline handling Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-11-08 22:50 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-11-12 19:50 ` Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-11-14 8:02 ` Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-11-14 8:31 ` Andrea Corallo
2023-11-14 20:23 ` Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-11-15 0:06 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-11-15 21:47 ` Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-11-20 23:04 ` Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-11-21 3:35 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-11-21 11:07 ` Andrea Corallo
2023-11-21 22:10 ` Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-11-21 22:18 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-11-22 15:40 ` Andrea Corallo
2023-11-22 16:07 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-11-22 16:31 ` Eli Zaretskii
2023-11-23 13:02 ` Andrea Corallo
2023-11-23 16:27 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-11-23 20:26 ` Andrea Corallo
2023-11-22 22:01 ` Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-11-23 6:19 ` Eli Zaretskii
2023-11-23 15:01 ` Andrea Corallo
2023-11-23 21:13 ` Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-11-23 15:00 ` Andrea Corallo
2023-11-23 16:24 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-11-23 16:38 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-11-23 20:21 ` Andrea Corallo
2023-11-23 21:36 ` Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-11-23 23:48 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-11-25 12:41 ` Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-11-25 14:39 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-11-25 18:35 ` Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-12-04 20:08 ` Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-12-04 20:47 ` Andrea Corallo
2023-12-04 23:57 ` Andy Moreton
2023-12-05 17:09 ` Andrea Corallo
2023-12-05 21:32 ` Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-12-06 0:13 ` Andy Moreton
2023-12-18 21:27 ` Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-11-23 21:18 ` Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-11-22 14:18 ` Eli Zaretskii
2023-11-16 8:46 ` Andrea Corallo
2023-11-17 15:58 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-11-17 20:37 ` Andrea Corallo
2023-11-17 20:44 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-11-20 8:58 ` Andrea Corallo
2023-11-20 13:13 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-11-18 7:05 ` Eli Zaretskii
2023-11-20 9:31 ` Andrea Corallo
2023-11-13 9:54 ` Andrea Corallo [this message]
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=yp1msvi6ks0.fsf@fencepost.gnu.org \
--to=acorallo@gnu.org \
--cc=67005@debbugs.gnu.org \
--cc=jschmidt4gnu@vodafonemail.de \
--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.