Subject: Re: bug#44676: [PATCH] Support native compilation of packages on install

From: Stefan Kangas <[email-masked]>
Date: Thu, 19 Nov 2020 14:34:52 -0800

Andrea Corallo <[email-masked]> writes:

>> BTW, what is the purpose of the LOAD parameter here?
>
> The main purpose of that parameter is to serve deferred compilation.

Thanks for the explanation, that clarifies things.

This gets me thinking, could we change the LOAD parameter to be called
LATE instead with valid values nil or non-nil?  Or is there a specific
reason why one would want to use LOAD set to t instead of just, say:

    (native-compile-async "foo.el")
    (load "foo.el")

Maybe the above example is a bit contrived, but I'm mostly trying to
understand if we have an opportunity to simplify the interface.

Oh, and come to think of it, could we just rename "late" load to
something more descriptive (for example "after-bytecode") or would that
be tremendously ugly?  :-)

> I think is simpler, we should from package just issue the compilation
> without any kind of 'load'.  Then around comp.el:3528 instead of
> complaining in case of a compilation with late load issued on a file
> with no load property just fixup the missing the late load in
> `comp-files-queue'.
>
> This will cover the case where package ask for the compilation but
> before is completed the user load the elc and deferred compilation issue
> another compilation with late load.

OK, thanks.  Please see my patches below.

> When we want to replace bytecode function definitions we perform this
> special kind of load that I called 'late'.  During this load instead of
> executing all top level forms of the original files we execute only
> function definitions (paying attention to have these effective only if
> the bytecode definition was not changed in the meanwhile).
>
> We should probably make this paramenter private (as for the syncronous
> variant `native-compile') as should be really for internal use only,
> WDYT?

Yes, that sounds very reasonable.  See the fourth patch below.
From 5c7e414f99962a9e1481a1311fa967f3c7df4b69 Mon Sep 17 00:00:00 2001
From: Stefan Kangas <[email-masked]>
Date: Thu, 19 Nov 2020 22:10:20 +0100
Subject: [PATCH 1/4] compile-async: Don't error out if late loading after
 normal load

* lisp/emacs-lisp/comp.el (native-compile-async): Update
comp-files-queue when an explicit late load is specified.  (Bug#44676)
---
 lisp/emacs-lisp/comp.el | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/lisp/emacs-lisp/comp.el b/lisp/emacs-lisp/comp.el
index cc5922c61c..a9a07535a4 100644
--- a/lisp/emacs-lisp/comp.el
+++ b/lisp/emacs-lisp/comp.el
@@ -3511,14 +3511,12 @@ native-compile-async
                        (list "Path not a file nor directory" path)))))
     (dolist (file files)
       (if-let ((entry (cl-find file comp-files-queue :key #'car :test #'string=)))
-          ;; When no load is specified (plain async compilation) we
-          ;; consider valid the one previously queued, otherwise we
-          ;; check for coherence (bug#40602).
-          (cl-assert (or (null load)
-                         (eq load (cdr entry)))
-                     nil "Trying to queue %s with LOAD %s but this is already \
-queued with LOAD %"
-                     file load (cdr entry))
+          ;; Most likely the byte-compiler has requested a late load,
+          ;; so update `comp-files-queue' to reflect that.
+          (unless (or (null load)
+                      (eq load (cdr entry)))
+            (cl-substitute (cons file load) (car entry) comp-files-queue
+                           :key #'car :test #'string=))
         ;; Make sure we are not already compiling `file' (bug#40838).
         (unless (or (gethash file comp-async-compilations)
                     ;; Also exclude files from deferred compilation if
-- 
2.29.2

From cd2965916119dc60e3d3457fd96662d562430e16 Mon Sep 17 00:00:00 2001
From: Stefan Kangas <[email-masked]>
Date: Thu, 19 Nov 2020 22:09:37 +0100
Subject: [PATCH 2/4] Support native compilation of packages on install

* lisp/emacs-lisp/package.el (package-unpack)
(package--native-compile): Native compile packages on install, if the
feature is available.  (Bug#44676)
(package-native-compile): New defcustom.
---
 etc/NEWS                   |  4 ++++
 lisp/emacs-lisp/package.el | 17 +++++++++++++++++
 2 files changed, 21 insertions(+)

diff --git a/etc/NEWS b/etc/NEWS
index 7aa5488250..8d90466312 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -823,6 +823,10 @@ equivalent to '(map (:sym sym))'.
 
 ** Package
 
+*** Native compile packages after installation.
+This happens automatically but can be turned off by customizing the
+user option `package-native-compile'.
+
 +++
 *** New commands to filter the package list.
 The filter command key bindings are as follows:
diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
index a381ca01f3..b03a7c5321 100644
--- a/lisp/emacs-lisp/package.el
+++ b/lisp/emacs-lisp/package.el
@@ -389,6 +389,12 @@ package-selected-packages
   :version "25.1"
   :type '(repeat symbol))
 
+(defcustom package-native-compile t
+  "Non-nil means to native compile packages on installation."
+  :type '(boolean)
+  :risky t
+  :version "28.1")
+
 (defcustom package-menu-async t
   "If non-nil, package-menu will use async operations when possible.
 Currently, only the refreshing of archive contents supports
@@ -968,6 +974,8 @@ package-unpack
         ;; E.g. for multi-package installs, we should first install all packages
         ;; and then compile them.
         (package--compile new-desc)
+        (when package-native-compile
+          (package--native-compile-async new-desc))
         ;; After compilation, load again any files loaded by
         ;; `activate-1', so that we use the byte-compiled definitions.
         (package--load-files-for-activation new-desc :reload)))
@@ -1052,6 +1060,15 @@ package--compile
         (load-path load-path))
     (byte-recompile-directory (package-desc-dir pkg-desc) 0 t)))
 
+(defun package--native-compile-async (pkg-desc)
+  "Native compile installed package PKG-DESC asynchronously.
+This assumes that `pkg-desc' has already been activated with
+`package-activate-1'."
+  (when (and (featurep 'nativecomp)
+             (native-comp-available-p))
+    (let ((warning-minimum-level :error))
+      (native-compile-async (package-desc-dir pkg-desc) t))))
+
 ;;;; Inferring package from current buffer
 (defun package-read-from-string (str)
   "Read a Lisp expression from STR.
-- 
2.29.2

From bd570f03a3b3dcb546b02e83faf263f27e82d05c Mon Sep 17 00:00:00 2001
From: Stefan Kangas <[email-masked]>
Date: Thu, 19 Nov 2020 22:11:17 +0100
Subject: [PATCH 3/4] * lisp/emacs-lisp/comp.el (native-compile-async): Doc
 fix.

---
 lisp/emacs-lisp/comp.el | 25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/lisp/emacs-lisp/comp.el b/lisp/emacs-lisp/comp.el
index a9a07535a4..e48a1cfc18 100644
--- a/lisp/emacs-lisp/comp.el
+++ b/lisp/emacs-lisp/comp.el
@@ -3489,13 +3489,28 @@ batch-byte-native-compile-for-bootstrap
 (defun native-compile-async (paths &optional recursively load)
   "Compile PATHS asynchronously.
 PATHS is one path or a list of paths to files or directories.
-`comp-async-jobs-number' specifies the number of (commands) to
-run simultaneously.  If RECURSIVELY, recurse into subdirectories
-of given directories.
-LOAD can be nil t or 'late."
+
+If optional argument RECURSIVELY is non-nil, recurse into
+subdirectories of given directories.
+
+If optional argument LOAD is non-nil, request to load the file
+after compiling.
+
+The variable `comp-async-jobs-number' specifies the number
+of (commands) to run simultaneously.
+
+LOAD can also be the symbol `late'.  This is used internally if
+the byte code has already been loaded when this function is
+called.  It means that we requests the special kind of load,
+necessary in that situation, called \"late\" loading.
+
+During a \"late\" load instead of executing all top level forms
+of the original files, only function definitions are
+loaded (paying attention to have these effective only if the
+bytecode definition was not changed in the meanwhile)."
   (comp-ensure-native-compiler)
   (unless (member load '(nil t late))
-    (error "LOAD must be nil t or 'late"))
+    (error "LOAD must be nil, t or 'late"))
   (unless (listp paths)
     (setf paths (list paths)))
   (let (files)
-- 
2.29.2

From 3d7427ff59e901872fe7f34d1c28f77e48d86c03 Mon Sep 17 00:00:00 2001
From: Stefan Kangas <[email-masked]>
Date: Thu, 19 Nov 2020 22:18:50 +0100
Subject: [PATCH 4/4] Make load argument of native-compile-async internal

* lisp/emacs-lisp/comp.el (native--compile-async-internal): New
defun extracted from native-compile-async.
(native-compile-async): Remove load argument and use above new defun.
* src/comp.c (maybe_defer_native_compilation): Use above new
defun.  (Bug#44676)
---
 lisp/emacs-lisp/comp.el | 18 ++++++++++++++++--
 src/comp.c              |  6 +++---
 2 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/lisp/emacs-lisp/comp.el b/lisp/emacs-lisp/comp.el
index e48a1cfc18..6cdfd60c04 100644
--- a/lisp/emacs-lisp/comp.el
+++ b/lisp/emacs-lisp/comp.el
@@ -3485,8 +3485,7 @@ batch-byte-native-compile-for-bootstrap
         (`(,tempfile . ,target-file)
          (rename-file tempfile target-file t))))))
 
-;;;###autoload
-(defun native-compile-async (paths &optional recursively load)
+(defun native--compile-async-internal (paths &optional recursively load)
   "Compile PATHS asynchronously.
 PATHS is one path or a list of paths to files or directories.
 
@@ -3553,6 +3552,21 @@ native-compile-async
     (when (zerop (comp-async-runnings))
       (comp-run-async-workers))))
 
+;;;###autoload
+(defun native-compile-async (paths &optional recursively)
+  "Compile PATHS asynchronously.
+PATHS is one path or a list of paths to files or directories.
+
+If optional argument RECURSIVELY is non-nil, recurse into
+subdirectories of given directories.
+
+If optional argument LOAD is non-nil, request to load the file
+after compiling.
+
+The variable `comp-async-jobs-number' specifies the number
+of (commands) to run simultaneously. "
+  (native--compile-async-internal paths recursively))
+
 (provide 'comp)
 
 ;;; comp.el ends here
diff --git a/src/comp.c b/src/comp.c
index 5b0f58b1a4..8145db8b9f 100644
--- a/src/comp.c
+++ b/src/comp.c
@@ -4669,13 +4669,13 @@ maybe_defer_native_compilation (Lisp_Object function_name,
       /* Comp already loaded.  */
       if (!NILP (delayed_sources))
 	{
-	  CALLN (Ffuncall, intern_c_string ("native-compile-async"),
+	  CALLN (Ffuncall, intern_c_string ("native--compile-async-internal"),
 		 delayed_sources, Qnil, Qlate);
 	  delayed_sources = Qnil;
 	}
       Fputhash (function_name, definition, Vcomp_deferred_pending_h);
-      CALLN (Ffuncall, intern_c_string ("native-compile-async"), src, Qnil,
-	     Qlate);
+      CALLN (Ffuncall, intern_c_string ("native--compile-async-internal"),
+	     src, Qnil, Qlate);
     }
   else
     {
-- 
2.29.2