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

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

[Stefan Kangas]

[-- Attachment #1: Type: text/plain, Size: 236 bytes --]

Severity: wishlist

Please find attach a patch to add support to package.el for native
compilation of packages on installation.

I've done some minimal testing and it seems to do the job.

(This is intended for the native-comp branch.)

[-- Attachment #2: 0001-Support-native-compilation-of-packages-on-install.patch --]
[-- Type: text/x-diff, Size: 1543 bytes --]

From 9a0da21a6989b20f593ec2b27a48eb4ef90561b7 Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefan@marxist.se>
Date: Mon, 16 Nov 2020 03:28:39 +0100
Subject: [PATCH] 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.
---
 lisp/emacs-lisp/package.el | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
index a381ca01f3..54b42db181 100644
--- a/lisp/emacs-lisp/package.el
+++ b/lisp/emacs-lisp/package.el
@@ -968,6 +968,7 @@ package-unpack
         ;; E.g. for multi-package installs, we should first install all packages
         ;; and then compile them.
         (package--compile new-desc)
+        (package--native-compile 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 +1053,12 @@ package--compile
         (load-path load-path))
     (byte-recompile-directory (package-desc-dir pkg-desc) 0 t)))
 
+(defun package--native-compile (pkg-desc)
+  (when (and (featurep 'nativecomp)
+             (native-comp-available-p))
+    (let ((warning-minimum-level :error))
+      (native-compile-async (package-desc-dir pkg-desc) t 'late))))
+
 ;;;; Inferring package from current buffer
 (defun package-read-from-string (str)
   "Read a Lisp expression from STR.
-- 
2.29.2

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

[Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Stefan Kangas <stefan@marxist.se> writes:

Hi Stefan,

thanks for the patch

> Severity: wishlist
>
> Please find attach a patch to add support to package.el for native
> compilation of packages on installation.
>
> I've done some minimal testing and it seems to do the job.
>
> (This is intended for the native-comp branch.)
>
> From 9a0da21a6989b20f593ec2b27a48eb4ef90561b7 Mon Sep 17 00:00:00 2001
> From: Stefan Kangas <stefan@marxist.se>
> Date: Mon, 16 Nov 2020 03:28:39 +0100
> Subject: [PATCH] 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.
> ---
>  lisp/emacs-lisp/package.el | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
> index a381ca01f3..54b42db181 100644
> --- a/lisp/emacs-lisp/package.el
> +++ b/lisp/emacs-lisp/package.el
> @@ -968,6 +968,7 @@ package-unpack
>          ;; E.g. for multi-package installs, we should first install all packages
>          ;; and then compile them.
>          (package--compile new-desc)
> +        (package--native-compile 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 +1053,12 @@ package--compile
>          (load-path load-path))
>      (byte-recompile-directory (package-desc-dir pkg-desc) 0 t)))
>  
> +(defun package--native-compile (pkg-desc)
> +  (when (and (featurep 'nativecomp)
> +             (native-comp-available-p))
> +    (let ((warning-minimum-level :error))
> +      (native-compile-async (package-desc-dir pkg-desc) t 'late))))

Late load assume the current bytecode is already loaded when the native
load will happen.  This because late load is designed to be issued when
some bytecode file is loaded and no native code alternative is found.

We should probably issue the async compilation here without late load
and in case the bytecode is being loaded before native compilation was
done just patch the kind of load stored into `comp-files-queue'.  ATM if
the stored load property and new one are not matching we complain (See
comp.el:3528).

So yeah non 100% straight forward :)

That said I think we should have a customize to decided if we want
package to eager command native compilation.

I may never used some of the installed files and prefer the current
solution for that, or maybe I may just like to have the compilation
happening in a more diluted fashion and on demand (my personal taste).

Thanks

  Andrea

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

[Stefan Kangas]

Hi Andrea,

Andrea Corallo <akrl@sdf.org> writes:

> Late load assume the current bytecode is already loaded when the native
> load will happen.  This because late load is designed to be issued when
> some bytecode file is loaded and no native code alternative is found.

Thanks, I had trouble figuring out what this parameter meant.  Maybe we
could improve its documentation.

BTW, what is the purpose of the LOAD parameter here?  Is it just a
convenience feature to allow a user to load the file after compiling?
I'm asking because that option was recently made obsolete for
`byte-compile-file'.

> We should probably issue the async compilation here without late load
> and in case the bytecode is being loaded before native compilation was
> done just patch the kind of load stored into `comp-files-queue'.  ATM if
> the stored load property and new one are not matching we complain (See
> comp.el:3528).
>
> So yeah non 100% straight forward :)

So when loading a byte-compiled file, Fload should check if there exists
an entry for this file in `comp-files-queue', and if there is one,
change its load property to `lazy'?  Do I understand you correctly?
Sorry if I'm not very clear, I'm still trying to understand all this.

> That said I think we should have a customize to decided if we want
> package to eager command native compilation.
>
> I may never used some of the installed files and prefer the current
> solution for that, or maybe I may just like to have the compilation
> happening in a more diluted fashion and on demand (my personal taste).

OK, I'll add such an option.

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

[Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Stefan Kangas <stefan@marxist.se> writes:

> Hi Andrea,
>
> Andrea Corallo <akrl@sdf.org> writes:
>
>> Late load assume the current bytecode is already loaded when the native
>> load will happen.  This because late load is designed to be issued when
>> some bytecode file is loaded and no native code alternative is found.
>
> Thanks, I had trouble figuring out what this parameter meant.  Maybe we
> could improve its documentation.

Absolutley.

> BTW, what is the purpose of the LOAD parameter here?  Is it just a
> convenience feature to allow a user to load the file after compiling?
> I'm asking because that option was recently made obsolete for
> `byte-compile-file'.

The main purpose of that parameter is to serve deferred compilation.

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?

>> We should probably issue the async compilation here without late load
>> and in case the bytecode is being loaded before native compilation was
>> done just patch the kind of load stored into `comp-files-queue'.  ATM if
>> the stored load property and new one are not matching we complain (See
>> comp.el:3528).
>>
>> So yeah non 100% straight forward :)
>
> So when loading a byte-compiled file, Fload should check if there exists
> an entry for this file in `comp-files-queue', and if there is one,
> change its load property to `lazy'?  Do I understand you correctly?
> Sorry if I'm not very clear, I'm still trying to understand all this.

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.

>> That said I think we should have a customize to decided if we want
>> package to eager command native compilation.
>>
>> I may never used some of the installed files and prefer the current
>> solution for that, or maybe I may just like to have the compilation
>> happening in a more diluted fashion and on demand (my personal taste).
>
> OK, I'll add such an option.

Great

Thanks

  Andrea

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

[Stefan Kangas]

[-- Attachment #1: Type: text/plain, Size: 1881 bytes --]

Andrea Corallo <akrl@sdf.org> 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.

[-- Attachment #2: 0001-compile-async-Don-t-error-out-if-late-loading-after-.patch --]
[-- Type: text/x-diff, Size: 1827 bytes --]

From 5c7e414f99962a9e1481a1311fa967f3c7df4b69 Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefan@marxist.se>
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


[-- Attachment #3: 0002-Support-native-compilation-of-packages-on-install.patch --]
[-- Type: text/x-diff, Size: 2689 bytes --]

From cd2965916119dc60e3d3457fd96662d562430e16 Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefan@marxist.se>
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


[-- Attachment #4: 0003-lisp-emacs-lisp-comp.el-native-compile-async-Doc-fix.patch --]
[-- Type: text/x-diff, Size: 1923 bytes --]

From bd570f03a3b3dcb546b02e83faf263f27e82d05c Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefan@marxist.se>
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


[-- Attachment #5: 0004-Make-load-argument-of-native-compile-async-internal.patch --]
[-- Type: text/x-diff, Size: 2634 bytes --]

From 3d7427ff59e901872fe7f34d1c28f77e48d86c03 Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefan@marxist.se>
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

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

[Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Stefan Kangas <stefan@marxist.se> writes:

> Andrea Corallo <akrl@sdf.org> 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:

That depends if we want to remove the normal load as an option.

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

The issue is that this does a different thing, if foo.el was never byte
compiled you will endup with only the non compiled (nor byte or native)
definitions as the load happes before the async compilation has finished.

I understand and share your motivation of simplifying the interface,
OTOH I think for the user might be handy to say compile and load when
finished without having to rely on the comp-async hooks.

> 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'd prefer to keep it as late.  Late load is a mechanism that is not
strictly related to bytecode (even if now we issue it in relation to
that) and in the code is referred as late-load in multiple palces I
believe.  Also late is shorter than after-bytecode :)

>> 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.
>

Following up with patch comments.

Thanks

  Andrea

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

[Stefan Kangas]

Andrea Corallo <akrl@sdf.org> writes:

>>     (native-compile-async "foo.el")
>>     (load "foo.el")
>
> The issue is that this does a different thing, if foo.el was never byte
> compiled you will endup with only the non compiled (nor byte or native)
> definitions as the load happes before the async compilation has finished.

So the eln wouldn't be loaded once it is done compiling, unless you
specify the explicit LOAD parameter?  If this is the case, I think I
agree with you that we should definitely keep the LOAD parameter.

However, perhaps we could make the LOAD parameter a simple boolean in
the user-facing `native-compile-async' function?  I.e., we hide away
`late' for internal use only.

> I'd prefer to keep it as late.  Late load is a mechanism that is not
> strictly related to bytecode (even if now we issue it in relation to
> that) and in the code is referred as late-load in multiple palces I
> believe.  Also late is shorter than after-bytecode :)

OK, sounds good to me.

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

[Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Stefan Kangas <stefan@marxist.se> writes:

> Andrea Corallo <akrl@sdf.org> writes:
>
>>>     (native-compile-async "foo.el")
>>>     (load "foo.el")
>>
>> The issue is that this does a different thing, if foo.el was never byte
>> compiled you will endup with only the non compiled (nor byte or native)
>> definitions as the load happes before the async compilation has finished.
>
> So the eln wouldn't be loaded once it is done compiling, unless you
> specify the explicit LOAD parameter?

Correct, either you or deferred compilation has to call
async-native-compile specifying the load.

> If this is the case, I think I
> agree with you that we should definitely keep the LOAD parameter.
>
> However, perhaps we could make the LOAD parameter a simple boolean in
> the user-facing `native-compile-async' function?  I.e., we hide away
> `late' for internal use only.

Agree that's probably the best option.

Thanks

  Andrea

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

[Stefan Kangas]

[-- Attachment #1: Type: text/plain, Size: 289 bytes --]

Andrea Corallo <akrl@sdf.org> writes:

>> However, perhaps we could make the LOAD parameter a simple boolean in
>> the user-facing `native-compile-async' function?  I.e., we hide away
>> `late' for internal use only.
>
> Agree that's probably the best option.

How does the attached look?

[-- Attachment #2: 0004-Make-load-argument-of-native-compile-async-internal.patch --]
[-- Type: text/x-diff, Size: 2743 bytes --]

From 6ac20230fae2b700d301cd35238b283f287a6e81 Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefan@marxist.se>
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 | 20 ++++++++++++++++++--
 src/comp.c              |  6 +++---
 2 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/lisp/emacs-lisp/comp.el b/lisp/emacs-lisp/comp.el
index 2f1e8965c1..9bebc5ff1b 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,23 @@ native-compile-async
     (when (zerop (comp-async-runnings))
       (comp-run-async-workers))))
 
+;;;###autoload
+(defun native-compile-async (paths &optional recursively load)
+  "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."
+  ;; Normalize: we only want to pass t or nil, never e.g. 'late.
+  (setq load (not (not load)))
+  (native--compile-async-internal paths recursively load))
+
 (provide 'comp)
 
 ;;; comp.el ends here
diff --git a/src/comp.c b/src/comp.c
index 6ddfad528b..89f8a4f3cd 100644
--- a/src/comp.c
+++ b/src/comp.c
@@ -4677,13 +4677,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

bug#44676: [PATCH 4/4] Support native compilation of packages on install

[Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Stefan Kangas <stefan@marxist.se> writes:

> Andrea Corallo <akrl@sdf.org> writes:
>
>>> However, perhaps we could make the LOAD parameter a simple boolean in
>>> the user-facing `native-compile-async' function?  I.e., we hide away
>>> `late' for internal use only.
>>
>> Agree that's probably the best option.
>
> How does the attached look?

Hi Stefan,

> From 6ac20230fae2b700d301cd35238b283f287a6e81 Mon Sep 17 00:00:00 2001
> From: Stefan Kangas <stefan@marxist.se>
> 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 | 20 ++++++++++++++++++--
>  src/comp.c              |  6 +++---
>  2 files changed, 21 insertions(+), 5 deletions(-)
>
> diff --git a/lisp/emacs-lisp/comp.el b/lisp/emacs-lisp/comp.el
> index 2f1e8965c1..9bebc5ff1b 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)

For constistency with `native--compile' and the other functions in the
file containing -- I'd go for `native--compile-async' here.

>    "Compile PATHS asynchronously.
>  PATHS is one path or a list of paths to files or directories.
>  
> @@ -3553,6 +3552,23 @@ native-compile-async
>      (when (zerop (comp-async-runnings))
>        (comp-run-async-workers))))
>  
> +;;;###autoload
> +(defun native-compile-async (paths &optional recursively load)
> +  "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."
> +  ;; Normalize: we only want to pass t or nil, never e.g. 'late.
> +  (setq load (not (not load)))

Nit, for my taste I'd rather use let or &aux, or probably just put `(not
(not load))` directly in the following function call.

> +  (native--compile-async-internal paths recursively load))
> +
>  (provide 'comp)
>  
>  ;;; comp.el ends here
> diff --git a/src/comp.c b/src/comp.c
> index 6ddfad528b..89f8a4f3cd 100644
> --- a/src/comp.c
> +++ b/src/comp.c
> @@ -4677,13 +4677,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
>      {

Sorry for the nit picking.  LGTM with these two.

Thanks

  Andrea

bug#44676: [PATCH 4/4] Support native compilation of packages on install

[Stefan Kangas]

tags 44676 fixed
close 44676
thanks

Andrea Corallo <akrl@sdf.org> writes:

> Sorry for the nit picking.  LGTM with these two.

Your comments are appreciated and welcome.  I've now fixed your comments
and pushed to feature/native-comp.  Closing this bug.

Thanks!

bug#44676: [PATCH 4/4] Support native compilation of packages on install

[Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Great, thanks

  Andrea

bug#44676: [PATCH 1/4] Support native compilation of packages on install

[Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Stefan Kangas <stefan@marxist.se> writes:

> From 5c7e414f99962a9e1481a1311fa967f3c7df4b69 Mon Sep 17 00:00:00 2001
> From: Stefan Kangas <stefan@marxist.se>
> 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,

I'd mention "deferred compilation" in place of "byte-compiler" as I
believe is more accurate.  This is the mechanism that triggers it and
also the customize that controls it.

> +          ;; 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

LGTM, okay with that nit.

Thanks

  Andrea

bug#44676: [PATCH 2/4] Support native compilation of packages on install

[Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Stefan Kangas <stefan@marxist.se> writes:

> From cd2965916119dc60e3d3457fd96662d562430e16 Mon Sep 17 00:00:00 2001
> From: Stefan Kangas <stefan@marxist.se>
> 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'.

This reminds me I've still not written a single NEWS entry for this
branch :/

>  +++
>  *** 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")

I don't have a strong feeling but I'm really not convinced this should
be t by default.  Despite personal preferences the nil equivalent of
this is what we already do for vast majority of the .el distributed with
Emacs. WDYT?

>  (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.

Otherwise LGTM

  Andrea

bug#44676: [PATCH 2/4] Support native compilation of packages on install

[Stefan Kangas]

Andrea Corallo <akrl@sdf.org> writes:

>> +(defcustom package-native-compile t
>> +  "Non-nil means to native compile packages on installation."
>> +  :type '(boolean)
>> +  :risky t
>> +  :version "28.1")
>
> I don't have a strong feeling but I'm really not convinced this should
> be t by default.  Despite personal preferences the nil equivalent of
> this is what we already do for vast majority of the .el distributed with
> Emacs. WDYT?

I don't have a strong opinion.

Since I recompile Emacs frequently, I would likely want to set this to
nil, but I assume most users would just use whatever comes with their
distro.  So perhaps a t value makes more sense for most users.

That said, I'm happy with introducing the variable set to a nil value.
We have time to change it later when we can think about it some more,
and get experience with it.  So I'll change it as you suggest (and
update the NEWS entry accordingly).

> Otherwise LGTM

Thanks.

bug#44676: [PATCH 2/4] Support native compilation of packages on install

[Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Stefan Kangas <stefan@marxist.se> writes:

> Andrea Corallo <akrl@sdf.org> writes:
>
>>> +(defcustom package-native-compile t
>>> +  "Non-nil means to native compile packages on installation."
>>> +  :type '(boolean)
>>> +  :risky t
>>> +  :version "28.1")
>>
>> I don't have a strong feeling but I'm really not convinced this should
>> be t by default.  Despite personal preferences the nil equivalent of
>> this is what we already do for vast majority of the .el distributed with
>> Emacs. WDYT?
>
> I don't have a strong opinion.
>
> Since I recompile Emacs frequently, I would likely want to set this to
> nil, but I assume most users would just use whatever comes with their
> distro.  So perhaps a t value makes more sense for most users.
>
> That said, I'm happy with introducing the variable set to a nil value.
> We have time to change it later when we can think about it some more,
> and get experience with it.  So I'll change it as you suggest (and
> update the NEWS entry accordingly).

My suggestion is to start with nil for coherency, we can always change
the default later if we feel.

  Andrea

bug#44676: [PATCH 3/4] Support native compilation of packages on install

[Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Stefan Kangas <stefan@marxist.se> writes:

> From bd570f03a3b3dcb546b02e83faf263f27e82d05c Mon Sep 17 00:00:00 2001
> From: Stefan Kangas <stefan@marxist.se>
> 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)

LGTM thanks for the nice clean-up