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

classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Emacs - Bugs mailing list
Stefan Kangas <[hidden email]> 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 <[hidden email]>
> 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



Reply | Threaded
Open this post in threaded view
|

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

Emacs - Bugs mailing list
Stefan Kangas <[hidden email]> writes:

> Hi Andrea,
>
> Andrea Corallo <[hidden email]> 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



Reply | Threaded
Open this post in threaded view
|

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

Stefan Kangas
Andrea Corallo <[hidden email]> 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.

0001-compile-async-Don-t-error-out-if-late-loading-after-.patch (2K) Download Attachment
0002-Support-native-compilation-of-packages-on-install.patch (3K) Download Attachment
0003-lisp-emacs-lisp-comp.el-native-compile-async-Doc-fix.patch (2K) Download Attachment
0004-Make-load-argument-of-native-compile-async-internal.patch (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Emacs - Bugs mailing list
Stefan Kangas <[hidden email]> writes:

> Andrea Corallo <[hidden email]> 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



Reply | Threaded
Open this post in threaded view
|

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

Emacs - Bugs mailing list
In reply to this post by Stefan Kangas
Stefan Kangas <[hidden email]> writes:

> From 5c7e414f99962a9e1481a1311fa967f3c7df4b69 Mon Sep 17 00:00:00 2001
> From: Stefan Kangas <[hidden email]>
> 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



Reply | Threaded
Open this post in threaded view
|

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

Emacs - Bugs mailing list
In reply to this post by Stefan Kangas
Stefan Kangas <[hidden email]> writes:

> From cd2965916119dc60e3d3457fd96662d562430e16 Mon Sep 17 00:00:00 2001
> From: Stefan Kangas <[hidden email]>
> 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



Reply | Threaded
Open this post in threaded view
|

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

Emacs - Bugs mailing list
In reply to this post by Stefan Kangas
Stefan Kangas <[hidden email]> writes:

> From bd570f03a3b3dcb546b02e83faf263f27e82d05c Mon Sep 17 00:00:00 2001
> From: Stefan Kangas <[hidden email]>
> 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



Reply | Threaded
Open this post in threaded view
|

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

Stefan Kangas
In reply to this post by Emacs - Bugs mailing list
Andrea Corallo <[hidden email]> 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.



Reply | Threaded
Open this post in threaded view
|

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

Stefan Kangas
In reply to this post by Emacs - Bugs mailing list
Andrea Corallo <[hidden email]> 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.



Reply | Threaded
Open this post in threaded view
|

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

Emacs - Bugs mailing list
In reply to this post by Stefan Kangas
Stefan Kangas <[hidden email]> writes:

> Andrea Corallo <[hidden email]> 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



Reply | Threaded
Open this post in threaded view
|

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

Emacs - Bugs mailing list
In reply to this post by Stefan Kangas
Stefan Kangas <[hidden email]> writes:

> Andrea Corallo <[hidden email]> 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



Reply | Threaded
Open this post in threaded view
|

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

Stefan Kangas
In reply to this post by Emacs - Bugs mailing list
Andrea Corallo <[hidden email]> 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?

0004-Make-load-argument-of-native-compile-async-internal.patch (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Emacs - Bugs mailing list
Great, thanks

  Andrea