[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
bug#67005: 30.0.50; improve nadivce/comp/trampoline handling
From: |
Jens Schmidt |
Subject: |
bug#67005: 30.0.50; improve nadivce/comp/trampoline handling |
Date: |
Sat, 25 Nov 2023 19:35:25 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) |
Stefan Monnier <monnier@iro.umontreal.ca> writes:
>> Plus another idea: Instead of placing the test into `advice-add', how
>> about checking functions for advices in loadup.el? We could place that
>> check just before site-load.el gets loaded, to avoid trouble for Emacs
>> modders. I tried the following, and it seemed to do the trick:
>>
>> +(mapatoms
>> + (lambda (f)
>> + (and (advice--p (symbol-function f))
>> + (error "Pre-dump advice on %s" f))))
>
> Even better!
Patch reworked accordingly, also to include the additional commit
message Andreas has proposed. Please review.
Would that merit a NEWS entry?
Thanks.
>From fc68d3e9084ede1fb8a109084398aac9e16f2e80 Mon Sep 17 00:00:00 2001
From: Jens Schmidt <jschmidt4gnu@vodafonemail.de>
Date: Mon, 20 Nov 2023 23:42:01 +0100
Subject: [PATCH] Update handling of advices during preload
* lisp/emacs-lisp/comp-common.el
(native-comp-never-optimize-functions): Remove macroexpand and
rename-buffer from default value.
* lisp/emacs-lisp/comp.el (comp-call-optim-form-call): Document call
optimization for advised primitives.
* lisp/emacs-lisp/nadvice.el (advice-add): Remove references to TODOs
that were completed already earlier.
* lisp/loadup.el: Disallow advices during preload. (Bug#67005)
---
lisp/emacs-lisp/comp-common.el | 9 ++++-----
lisp/emacs-lisp/comp.el | 8 ++++++++
lisp/emacs-lisp/nadvice.el | 2 --
lisp/loadup.el | 9 +++++++++
4 files changed, 21 insertions(+), 7 deletions(-)
diff --git a/lisp/emacs-lisp/comp-common.el b/lisp/emacs-lisp/comp-common.el
index 6d94d1bd82e..b7a685223ed 100644
--- a/lisp/emacs-lisp/comp-common.el
+++ b/lisp/emacs-lisp/comp-common.el
@@ -49,11 +49,10 @@ native-comp-verbose
:version "28.1")
(defcustom native-comp-never-optimize-functions
- '(eval
- ;; The following two are mandatory for Emacs to be working
- ;; correctly (see comment in `advice--add-function'). DO NOT
- ;; REMOVE.
- macroexpand rename-buffer)
+ ;; We used to list those functions here that were advised during
+ ;; preload, but we now prefer to disallow preload advices in
+ ;; loadup.el (bug#67005).
+ '(eval)
"Primitive functions to exclude from trampoline optimization.
Primitive functions included in this list will not be called
diff --git a/lisp/emacs-lisp/comp.el b/lisp/emacs-lisp/comp.el
index 73764eb1d79..9d537cbcfa7 100644
--- a/lisp/emacs-lisp/comp.el
+++ b/lisp/emacs-lisp/comp.el
@@ -2783,6 +2783,14 @@ comp-call-optim-form-call
(symbol-function callee)
(cl-assert (byte-code-function-p callee))
callee))
+ ;; Below call to `subrp' returns nil on an advised
+ ;; primitive F, so that we do not optimize calls to F
+ ;; with the funcall trampoline removal below. But if F
+ ;; is advised while we compile its call, it is very
+ ;; likely to be advised also when that call is executed.
+ ;; And in that case an "unoptimized" call to F is
+ ;; actually cheaper since it avoids the call to the
+ ;; intermediate native trampoline (bug#67005).
(subrp (subrp f))
(comp-func-callee (comp-func-in-unit callee)))
(cond
diff --git a/lisp/emacs-lisp/nadvice.el b/lisp/emacs-lisp/nadvice.el
index 42027c01491..9f2b42f5765 100644
--- a/lisp/emacs-lisp/nadvice.el
+++ b/lisp/emacs-lisp/nadvice.el
@@ -509,8 +509,6 @@ advice-add
<<>>"
;; TODO:
;; - record the advice location, to display in describe-function.
- ;; - change all defadvice in lisp/**/*.el.
- ;; - obsolete advice.el.
(let* ((f (symbol-function symbol))
(nf (advice--normalize symbol f)))
(unless (eq f nf) (fset symbol nf))
diff --git a/lisp/loadup.el b/lisp/loadup.el
index 07895228d0d..3b58d5fb9b7 100644
--- a/lisp/loadup.el
+++ b/lisp/loadup.el
@@ -393,6 +393,15 @@
;; from the repository. It is generated just after temacs is built.
(load "leim/leim-list.el" t)
+;; Actively disallow advised functions during preload since:
+;; - advices in Emacs's core are generally considered bad style;
+;; - `Snarf-documentation' looses docstrings of primitives advised
+;; during preload (bug#66032#20).
+(mapatoms
+ (lambda (f)
+ (and (advice--p (symbol-function f))
+ (error "Preload advice on %s" f))))
+
;; If you want additional libraries to be preloaded and their
;; doc strings kept in the DOC file rather than in core,
;; you may load them with a "site-load.el" file.
--
2.30.2
- bug#67005: 30.0.50; improve nadivce/comp/trampoline handling, (continued)
- bug#67005: 30.0.50; improve nadivce/comp/trampoline handling, Andrea Corallo, 2023/11/23
- bug#67005: 30.0.50; improve nadivce/comp/trampoline handling, Stefan Monnier, 2023/11/23
- bug#67005: 30.0.50; improve nadivce/comp/trampoline handling, Stefan Monnier, 2023/11/23
- bug#67005: 30.0.50; improve nadivce/comp/trampoline handling, Andrea Corallo, 2023/11/23
- bug#67005: 30.0.50; improve nadivce/comp/trampoline handling, Jens Schmidt, 2023/11/23
- bug#67005: 30.0.50; improve nadivce/comp/trampoline handling, Stefan Monnier, 2023/11/23
- bug#67005: 30.0.50; improve nadivce/comp/trampoline handling, Jens Schmidt, 2023/11/25
- bug#67005: 30.0.50; improve nadivce/comp/trampoline handling, Stefan Monnier, 2023/11/25
- bug#67005: 30.0.50; improve nadivce/comp/trampoline handling,
Jens Schmidt <=
- bug#67005: 30.0.50; improve nadivce/comp/trampoline handling, Jens Schmidt, 2023/11/23
- bug#67005: 30.0.50; improve nadivce/comp/trampoline handling, Eli Zaretskii, 2023/11/22
- bug#67005: 30.0.50; improve nadivce/comp/trampoline handling, Andrea Corallo, 2023/11/16
- bug#67005: 30.0.50; improve nadivce/comp/trampoline handling, Stefan Monnier, 2023/11/17
- bug#67005: 30.0.50; improve nadivce/comp/trampoline handling, Andrea Corallo, 2023/11/17
- bug#67005: 30.0.50; improve nadivce/comp/trampoline handling, Stefan Monnier, 2023/11/17
- bug#67005: 30.0.50; improve nadivce/comp/trampoline handling, Andrea Corallo, 2023/11/20
- bug#67005: 30.0.50; improve nadivce/comp/trampoline handling, Stefan Monnier, 2023/11/20
- bug#67005: 30.0.50; improve nadivce/comp/trampoline handling, Eli Zaretskii, 2023/11/18
- bug#67005: 30.0.50; improve nadivce/comp/trampoline handling, Andrea Corallo, 2023/11/20