GNU bug report logs - #19390
25.0.50; `package-activate' is too slow

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 19390 in the body.
You can then email your comments to 19390 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: bug-gnu-emacs <at> gnu.org
Subject: 25.0.50; `package-activate' is too slow
Date: Mon, 15 Dec 2014 19:34:50 +0200

Before c13baa10d55ec863d3ceaea48c6b2959ece98198, `package-initialize'
takes ~100 ms on my machine, with 92 packages.

After this revision, it takes ~7.7 seconds.

Provided there's no obvious low-hanging fruit for performance
improvement, we should at least avoid exercising the new logic when
Emacs is just launching. There's simply nothing to reload yet.

In GNU Emacs 25.0.50.1 (x86_64-unknown-linux-gnu, GTK+ Version 3.10.8)
 of 2014-12-14 on axl
Repository revision: eaf25ad549dc5a9b26089f588e0a80268708a3d1
Windowing system distributor `The X.Org Foundation', version 11.0.11501000
System Description:	Ubuntu 14.04.1 LTS

Message #8 received at 19390 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: 19390 <at> debbugs.gnu.org
Subject: Re: bug#19390: Acknowledgement (25.0.50; `package-activate' is too
 slow)
Date: Tue, 16 Dec 2014 02:41:58 +0200

Here's one possible fix. We don't deal with the slowness of 
`package--list-loaded-files' at all, and instead only load them if 
`package-activate' had been called with non-nil second argument (which 
only happens now if the package is installed).

This also changes the way `package-activate' calls itself recursively on 
dependencies: instead of always passing the version as the second 
argument, pass the same value of FORCE it's been called with. Passing 
the version seems to be a remnant of older code.


diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
index cd948ae..394efc3 100644
--- a/lisp/emacs-lisp/package.el
+++ b/lisp/emacs-lisp/package.el
@@ -516,7 +516,7 @@ Return the max version (as a string) if the package 
is held at a lower version."
              force))
           (t (error "Invalid element in `package-load-list'")))))

-(defun package-activate-1 (pkg-desc)
+(defun package-activate-1 (pkg-desc &optional force)
   (let* ((name (package-desc-name pkg-desc))
 	 (pkg-dir (package-desc-dir pkg-desc))
          (pkg-dir-dir (file-name-as-directory pkg-dir)))
@@ -527,7 +527,7 @@ Return the max version (as a string) if the package 
is held at a lower version."
     (let* ((old-lp load-path)
            (autoloads-file (expand-file-name
                             (format "%s-autoloads" name) pkg-dir))
-           (loaded-files-list (package--list-loaded-files pkg-dir)))
+           (loaded-files-list (and force (package--list-loaded-files 
pkg-dir))))
       (with-demoted-errors (format "Error loading %s: %%s" name)
         (load autoloads-file nil t))
       (when (and (eq old-lp load-path)
@@ -636,14 +636,14 @@ If FORCE is true, (re-)activate it if it's already 
activated."
              (fail (catch 'dep-failure
                      ;; Activate its dependencies recursively.
                      (dolist (req (package-desc-reqs pkg-vec))
-                       (unless (package-activate (car req) (cadr req))
+                       (unless (package-activate (car req) force)
                          (throw 'dep-failure req))))))
 	(if fail
 	    (warn "Unable to activate package `%s'.
 Required package `%s-%s' is unavailable"
 		  package (car fail) (package-version-join (cadr fail)))
 	  ;; If all goes well, activate the package itself.
-	  (package-activate-1 pkg-vec)))))))
+	  (package-activate-1 pkg-vec force)))))))

 (defun define-package (_name-string _version-string
                                     &optional _docstring _requirements

Message #11 received at 19390 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Artur Malabarba <bruce.connor.am <at> gmail.com>
Cc: 19390 <at> debbugs.gnu.org
Subject: Re: bug#19390: 25.0.50; `package-activate' is too slow
Date: Tue, 16 Dec 2014 14:32:16 +0200

Arthur, do you experience this problem?

What's your opinion on the fix?

(http://debbugs.gnu.org/19390)

Message #14 received at 19390 <at> debbugs.gnu.org (full text, mbox):

From: Artur Malabarba <bruce.connor.am <at> gmail.com>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 19390 <at> debbugs.gnu.org
Subject: Re: bug#19390: 25.0.50; `package-activate' is too slow
Date: Tue, 16 Dec 2014 11:26:15 -0200

I didn't notice anything as aggravating as the report, but I agree my
latest patch is a performance concern.

The proposed patch looks good, just a couple of notes:

1. I'd use RELOAD for the name of the optional argument.
2. Since this function now takes a second (somewhat non-trivial
argument), we should add a docstring to it:

"Activate package given by PKG-DESC, even if it was already active.
If RELOAD is non-nil, also `load' any files inside the package which
correspond to previously loaded files (those returned by
`package--list-loaded-files').

This is called internally by `package-activate'."

Message #17 received at 19390 <at> debbugs.gnu.org (full text, mbox):

From: Artur Malabarba <bruce.connor.am <at> gmail.com>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 19390 <at> debbugs.gnu.org
Subject: Re: bug#19390: 25.0.50; `package-activate' is too slow
Date: Tue, 16 Dec 2014 11:42:09 -0200

As for low-hanging fruits in `package--list-loaded-files', there are
two I can think of.

1. If `load-history' stored `file-truename's (relative to "/"),
instead of just absolute file names, then that would avoid a lot of
calls to `file-truename'. This, I suspect, is the bigger performance
issue.
2. Creating the `history' variable as below is a problem.

(mapcar (lambda (x) (file-name-sans-extension
                                  (file-truename (car x))))
                    load-history)

This lambda is called a lot more times than necessary, because most
entries in load-history are useless here. But we need to do this to be
able to reliably compare file names. Ideally, there would be another
variable, like `load-history-truename', which is modified in parellel
with `load-history' and *only* stores the truename of files listed in
`load-history' (no need to store all that extra information). Then we
could just use this variable unmodified.

Implementing 2. would also fix 1.

2014-12-16 11:26 GMT-02:00 Artur Malabarba <bruce.connor.am <at> gmail.com>:
> I didn't notice anything as aggravating as the report, but I agree my
> latest patch is a performance concern.
>
> The proposed patch looks good, just a couple of notes:
>
> 1. I'd use RELOAD for the name of the optional argument.
> 2. Since this function now takes a second (somewhat non-trivial
> argument), we should add a docstring to it:
>
> "Activate package given by PKG-DESC, even if it was already active.
> If RELOAD is non-nil, also `load' any files inside the package which
> correspond to previously loaded files (those returned by
> `package--list-loaded-files').
>
> This is called internally by `package-activate'."

Message #20 received at 19390 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: bruce.connor.am <at> gmail.com
Cc: 19390 <at> debbugs.gnu.org
Subject: Re: bug#19390: 25.0.50; `package-activate' is too slow
Date: Tue, 16 Dec 2014 16:31:29 +0200

On 12/16/2014, Artur Malabarba wrote:

> I didn't notice anything as aggravating as the report, but I agree my
> latest patch is a performance concern.

Do you maybe have fewer packages installed?

> avoid a lot of
> calls to `file-truename'. This, I suspect, is the bigger performance
> issue.

Any particular reason you're using `file-truename', instead of 
`expand-file-name'? Replacing the former with the latter already gives 
~twofold performance improvement.

> 2. Creating the `history' variable as below is a problem.
>
> (mapcar (lambda (x) (file-name-sans-extension
>                                    (file-truename (car x))))
>                      load-history)

With the above change, this piece of code is relatively fast.

For instance, (package--list-loaded-files default-directory) in the Helm 
directory takes 140 ms here, whereas

            (mapcar (lambda (x) (file-name-sans-extension
                                 (expand-file-name (car x))))
             load-history)

only takes ~10 ms. Still not fast enough not to need the other patch, of 
course.

Message #23 received at 19390 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Artur Malabarba <bruce.connor.am <at> gmail.com>
Cc: 19390 <at> debbugs.gnu.org
Subject: Re: bug#19390: 25.0.50; `package-activate' is too slow
Date: Tue, 16 Dec 2014 18:53:42 +0200

That aside, it's too bad not the whole of discussion leading to this
implementation is public.

Have the following alternative implementation options been considered?

Since we only actually want to reload when upgrading, or reinstalling
packages, maybe some key logic can move into `package-delete'.

For instance:

1. Instead of scanning through the whole load-history when activating a
package, we could have a list of paths that belonged to packages that we
uninstalled during the current session. It would be collected in
`package-delete', and it would certainly be empty at startup.

2. Instead of saving paths, remove elements from the `features' list
when a package is deleted. Then when a dependency of this package is
installed (or even autoloaded [0]), it would load the new files, because
they're not in features.

[0] IIUC, the current approach doesn't cover this situation.

Message #26 received at 19390 <at> debbugs.gnu.org (full text, mbox):

From: Artur Malabarba <bruce.connor.am <at> gmail.com>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 19390 <at> debbugs.gnu.org
Subject: Re: bug#19390: 25.0.50; `package-activate' is too slow
Date: Tue, 16 Dec 2014 19:35:38 -0200

2014-12-16 12:31 GMT-02:00 Dmitry Gutov > On 12/16/2014, Artur Malabarba wrote:
>
>> I didn't notice anything as aggravating as the report, but I agree my
>> latest patch is a performance concern.
>
> Do you maybe have fewer packages installed?

Perhaps.
But the reason is beyond the point, :-) I agree this patch needs
performance improvements.

> Any particular reason you're using file-truename', instead of
>expand-file-name'? Replacing the former with the latter already gives
> ~twofold performance improvement.

Yes, I originally used `expand-file-name', but
`package--list-loaded-files' would miss some files if there was a
symlink in the path. (I specifically observed this, it's not just
theory).
The problem is that a file returned by `find-libary' could be the same
as another file contained in  `load-history' but have a different name
(because of symlinks).


>> 2. Creating the `history' variable as below is a problem.
>>
>> (mapcar (lambda (x) (file-name-sans-extension
>>                                    (file-truename (car x))))
>>                      load-history)
>
>
> With the above change, this piece of code is relatively fast.
>
> For instance, (package--list-loaded-files default-directory) in the Helm
> directory takes 140 ms here, whereas
>
>             (mapcar (lambda (x) (file-name-sans-extension
>                                  (expand-file-name (car x))))
>              load-history)
>
> only takes ~10 ms. Still not fast enough not to need the other patch, of
> course.

Yes, I agree `file-truename' is slow. The foolproof way would be to
have a variable that stores the `file-truename's, so we wouldn't have
to calculate them.
Using `expand-file-name' is a valid way to improve performance here,
but it won't *always* reload the files for all users.

Message #29 received at 19390 <at> debbugs.gnu.org (full text, mbox):

From: Artur Malabarba <bruce.connor.am <at> gmail.com>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 19390 <at> debbugs.gnu.org
Subject: Re: bug#19390: 25.0.50; `package-activate' is too slow
Date: Tue, 16 Dec 2014 20:00:00 -0200

2014-12-16 14:53 GMT-02:00 Dmitry Gutov <dgutov <at> yandex.ru>:
> That aside, it's too bad not the whole of discussion leading to this
> implementation is public.

The discussion started here:
http://lists.gnu.org/archive/html/emacs-devel/2014-10/msg00567.html
Oddly enough, I can't find the end of it. =/
I'll be glad to explain any of the considerations.

> Have the following alternative implementation options been considered?
> Since we only actually want to reload when upgrading, or reinstalling
> packages, maybe some key logic can move into `package-delete'.

Besides the points I mention below, there's one edge case where this
could be a problem. Built-in packages can be upgraded too, and they
can't be deleted.

> For instance:
>
> 1. Instead of scanning through the whole load-history when activating a
> package, we could have a list of paths that belonged to packages that we
> uninstalled during the current session. It would be collected in
> `package-delete', and it would certainly be empty at startup.

The way it's done right now, package-delete is called after
package-install during upgrades, so that would need to change in order
for this implementation to work.
I would rather ask on the devel-list before making a change like this
(I don't know if there's a particular reason behind the original
choice), but other than that, that sounds like a good solution.

> 2. Instead of saving paths, remove elements from the `features' list
> when a package is deleted. Then when a dependency of this package is
> installed (or even autoloaded [0]), it would load the new files, because
> they're not in features.

Again, this relies on us deleting before installing, but I prefer you
first idea. Removing elements from `features' could have unintended
consequences. We're pretending the package isn't loaded even though it
is (all its functions and variables are bound).

>
> [0] IIUC, the current approach doesn't cover this situation.

The current approach does cover autoloads. The autoloads file of a
package is always `load'ed when a package is activated (which always
happens when a package is installed, even after an upgrade). Calling
`load' on a file which contains a new definition for an already
defined autoload (which is what happens on upgrades) redefines the
autoload to the new definition.

Message #32 received at 19390 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Artur Malabarba <bruce.connor.am <at> gmail.com>
Cc: 19390 <at> debbugs.gnu.org, Dmitry Gutov <dgutov <at> yandex.ru>
Subject: Re: bug#19390: 25.0.50; `package-activate' is too slow
Date: Tue, 16 Dec 2014 18:16:48 -0500

> Yes, I originally used `expand-file-name', but
> `package--list-loaded-files' would miss some files if there was a
> symlink in the path. (I specifically observed this, it's not just
> theory).

Do you remember the scenario where you saw that?  Normally, the
load-history contains the file names using the directory names that
appear in load-path, so while they're not "canonical", as long as we
stick to using file name relative to directories in `load-path' it
seems that the cases where two files are really the same and we don't
detect it should be rather rare.


        Stefan

Message #35 received at 19390 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 19390 <at> debbugs.gnu.org, Artur Malabarba <bruce.connor.am <at> gmail.com>
Subject: Re: bug#19390: 25.0.50; `package-activate' is too slow
Date: Tue, 16 Dec 2014 18:19:14 -0500

> Since we only actually want to reload when upgrading, or reinstalling
> packages, maybe some key logic can move into `package-delete'.

package-delete sounds clearly wrong, since we may very well install
a new version without removing the old version.

> 2. Instead of saving paths, remove elements from the `features' list
> when a package is deleted. Then when a dependency of this package is
> installed (or even autoloaded [0]), it would load the new files, because
> they're not in features.

Eagerly reloading files is much more effective than just removing them
from `features'.  E.g. in the case of autoloaded functions.


        Stefan

Message #38 received at 19390 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>, 
 Artur Malabarba <bruce.connor.am <at> gmail.com>
Cc: 19390 <at> debbugs.gnu.org
Subject: Re: bug#19390: 25.0.50; `package-activate' is too slow
Date: Wed, 17 Dec 2014 01:27:09 +0200

On 12/17/2014 01:16 AM, Stefan Monnier wrote:
> Do you remember the scenario where you saw that?  Normally, the
> load-history contains the file names using the directory names that
> appear in load-path, so while they're not "canonical",

Actually, it seems to me that load-history does contain canonical names. 
At least, when the file being loaded is a symlink, the resulting record 
in load-history contains the path of the file it points to. Same for a 
real file in a symlinked dir.

Message #41 received at 19390 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 19390 <at> debbugs.gnu.org, Artur Malabarba <bruce.connor.am <at> gmail.com>
Subject: Re: bug#19390: 25.0.50; `package-activate' is too slow
Date: Wed, 17 Dec 2014 01:40:58 +0200

On 12/17/2014 01:19 AM, Stefan Monnier wrote:

> package-delete sounds clearly wrong, since we may very well install
> a new version without removing the old version.

Oh, right. `package-install', then? It can check if another version of 
the given package is already installed, and if so that do that thing.

> Eagerly reloading files is much more effective than just removing them
> from `features'.  E.g. in the case of autoloaded functions.

How about calling `unload-feature', too? It sounds nice on paper, and 
this might be a good (first?) use for it.

Message #44 received at 19390 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 19390 <at> debbugs.gnu.org, Artur Malabarba <bruce.connor.am <at> gmail.com>
Subject: Re: bug#19390: 25.0.50; `package-activate' is too slow
Date: Tue, 16 Dec 2014 18:55:20 -0500

>> Do you remember the scenario where you saw that?  Normally, the
>> load-history contains the file names using the directory names that
>> appear in load-path, so while they're not "canonical",
> Actually, it seems to me that load-history does contain canonical names.
> At least, when the file being loaded is a symlink, the resulting record in
> load-history contains the path of the file it points to.  Same for a real
> file in a symlinked dir.

Hmm... OK, then that would explain why he needed to use file-truename.


        Stefan

Message #47 received at 19390 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 19390 <at> debbugs.gnu.org, Artur Malabarba <bruce.connor.am <at> gmail.com>
Subject: Re: bug#19390: 25.0.50; `package-activate' is too slow
Date: Tue, 16 Dec 2014 18:56:57 -0500

> How about calling `unload-feature', too? It sounds nice on paper, and this
> might be a good (first?) use for it.

Yes, it sounds nice on paper, but in my experience unload-feature is
much too unreliable to foist it upon users like that.


        Stefan

Message #50 received at 19390 <at> debbugs.gnu.org (full text, mbox):

From: Artur Malabarba <bruce.connor.am <at> gmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 19390 <at> debbugs.gnu.org, Dmitry Gutov <dgutov <at> yandex.ru>
Subject: Re: bug#19390: 25.0.50; `package-activate' is too slow
Date: Tue, 16 Dec 2014 22:40:07 -0200

[Message part 1 (text/plain, inline)]

On 16 Dec 2014 21:55, "Stefan Monnier" <monnier <at> iro.umontreal.ca> wrote:
>
> >> Do you remember the scenario where you saw that?  Normally, the
> >> load-history contains the file names using the directory names that
> >> appear in load-path, so while they're not "canonical",
> > Actually, it seems to me that load-history does contain canonical names.
> > At least, when the file being loaded is a symlink, the resulting record
in
> > load-history contains the path of the file it points to.  Same for a
real
> > file in a symlinked dir.
>
> Hmm... OK, then that would explain why he needed to use file-truename.

Actually, this means I may have overused file-truename. If load-history
already stores canonical names, then we only need to call truename on the
return value of find library, not on the elements of load-history.

If nobody else checks, I'll check tomorrow when I get a chance.

[Message part 2 (text/html, inline)]

Message #53 received at 19390 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: bruce.connor.am <at> gmail.com
Cc: 19390 <at> debbugs.gnu.org
Subject: Re: bug#19390: 25.0.50; `package-activate' is too slow
Date: Wed, 17 Dec 2014 03:05:37 +0200

On 12/17/2014 12:00 AM, Artur Malabarba wrote:

> The discussion started here:
> http://lists.gnu.org/archive/html/emacs-devel/2014-10/msg00567.html
> Oddly enough, I can't find the end of it. =/

Thanks, the part available there already explained the more complex part 
(why sorting).

> The current approach does cover autoloads. The autoloads file of a
> package is always `load'ed when a package is activated (which always
> happens when a package is installed, even after an upgrade). Calling
> `load' on a file which contains a new definition for an already
> defined autoload (which is what happens on upgrades) redefines the
> autoload to the new definition.

I see.

Message #56 received at 19390 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: bruce.connor.am <at> gmail.com, Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 19390 <at> debbugs.gnu.org
Subject: Re: bug#19390: 25.0.50; `package-activate' is too slow
Date: Wed, 17 Dec 2014 03:17:31 +0200

On 12/17/2014 02:40 AM, Artur Malabarba wrote:

> Actually, this means I may have overused file-truename. If load-history
> already stores canonical names, then we only need to call truename on
> the return value of find library, not on the elements of load-history.

Look like.

> If nobody else checks, I'll check tomorrow when I get a chance.

I'll commit the fix soon (it gives a nice performance boost already), 
but please go ahead and double-check it tomorrow.

Message #59 received at 19390 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: bruce.connor.am <at> gmail.com, Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 19390 <at> debbugs.gnu.org
Subject: Re: bug#19390: 25.0.50; `package-activate' is too slow
Date: Wed, 17 Dec 2014 04:41:49 +0200

Speaking of non-radical optimizations, this takes the packages 
activation time of 2.1 s (after the last small patch) down to 300 ms.

diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
index 60beebd..30b5fb2 100644
--- a/lisp/emacs-lisp/package.el
+++ b/lisp/emacs-lisp/package.el
@@ -527,7 +527,7 @@ Return the max version (as a string) if the package 
is held at a lower version."
     (let* ((old-lp load-path)
            (autoloads-file (expand-file-name
                             (format "%s-autoloads" name) pkg-dir))
-           (loaded-files-list (package--list-loaded-files pkg-dir)))
+           (loaded-files-list (package--list-loaded-files name pkg-dir)))
       (with-demoted-errors (format "Error loading %s: %%s" name)
         (load autoloads-file nil t))
       (when (and (eq old-lp load-path)
@@ -555,36 +555,36 @@ Return the max version (as a string) if the 
package is held at a lower version."
     ;; Don't return nil.
     t))

-(defun package--list-loaded-files (dir)
+(defun package--list-loaded-files (name dir)
   "Recursively list all files in DIR which correspond to loaded features.
 Returns the `file-name-sans-extension' of each file, relative to
 DIR, sorted by most recently loaded last."
-  (let* ((history (mapcar (lambda (x) (file-name-sans-extension (car x)))
-                    load-history))
-         (dir (file-truename dir))
-         ;; List all files that have already been loaded.
-         (list-of-conflicts
-          (remove
-           nil
-           (mapcar
-               (lambda (x) (let* ((file (file-relative-name x dir))
-                             ;; Previously loaded file, if any.
-                             (previous
-                              (ignore-errors
-                                (file-name-sans-extension
-                                 (file-truename (find-library-name 
file)))))
-                             (pos (when previous (member previous 
history))))
-                        ;; Return (RELATIVE-FILENAME . HISTORY-POSITION)
-                        (when pos
-                          (cons (file-name-sans-extension file) (length 
pos)))))
-             (directory-files-recursively dir "\\`[^\\.].*\\.el\\'")))))
-    ;; Turn the list of (FILENAME . POS) back into a list of features. 
 Files in
-    ;; subdirectories are returned relative to DIR (so not actually 
features).
-    (let ((default-directory (file-name-as-directory dir)))
-      (mapcar (lambda (x) (file-truename (car x)))
-        (sort list-of-conflicts
-              ;; Sort the files by ascending HISTORY-POSITION.
-              (lambda (x y) (< (cdr x) (cdr y))))))))
+  (let* ((old-dir-re (concat "\\`"
+                             (regexp-quote (file-truename 
package-user-dir))
+                             "/" (regexp-quote (symbol-name name))))
+         (filtered-history (cl-loop for entry in load-history
+                                    for file = (car entry)
+                                    when (string-match-p old-dir-re file)
+                                    collect (file-name-sans-extension 
file)))
+         (files (directory-files-recursively dir "\\`[^\\.].*\\.el\\'"))
+         (files-re (concat "/"
+                           (regexp-opt
+                            (mapcar (lambda (x)
+                                      (file-name-sans-extension
+                                       (file-relative-name x dir)))
+                                    files))
+                           "\\'"))
+         list-of-conflicts)
+    ;; List all the matching files from the load history, in
+    ;; historical order.
+    (dolist (file filtered-history)
+      (when (string-match files-re file)
+        (cl-pushnew (substring (match-string 0 file) 1)
+                    list-of-conflicts
+                    :test #'equal)))
+    ;; Files in subdirectories are returned relative to DIR (so not
+    ;; actually features).
+    list-of-conflicts))

 (defun package-built-in-p (package &optional min-version)
   "Return true if PACKAGE is built-in to Emacs.

Message #62 received at 19390 <at> debbugs.gnu.org (full text, mbox):

From: Artur Malabarba <bruce.connor.am <at> gmail.com>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 19390 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#19390: 25.0.50; `package-activate' is too slow
Date: Wed, 17 Dec 2014 08:47:57 -0200

[Message part 1 (text/plain, inline)]

On 17 Dec 2014 00:41, "Dmitry Gutov" <dgutov <at> yandex.ru> wrote:
> +  (let* ((old-dir-re (concat "\\`"
> +                             (regexp-quote (file-truename
package-user-dir))
> +                             "/" (regexp-quote (symbol-name name))))
> +         (filtered-history (cl-loop for entry in load-history
> +                                    for file = (car entry)
> +                                    when (string-match-p old-dir-re file)
> +                                    collect (file-name-sans-extension
file)))

This will fail if the previously loaded version wasn't installed in the
package-user-dir,  which, again, is the case for built-in packages. This
also includes manually installed packages, which we may or may not care
about supporting.

[Message part 2 (text/html, inline)]

Message #65 received at 19390 <at> debbugs.gnu.org (full text, mbox):

From: Artur Malabarba <bruce.connor.am <at> gmail.com>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>, 19390 <at> debbugs.gnu.org
Subject: Re: bug#19390: 25.0.50; `package-activate' is too slow
Date: Wed, 17 Dec 2014 12:33:43 -0200

FWIW, I just checked an indeed it seems load-history stores file-truenames.
So that patch is all good.

2014-12-16 23:17 GMT-02:00 Dmitry Gutov <dgutov <at> yandex.ru>:
> On 12/17/2014 02:40 AM, Artur Malabarba wrote:
>
>> Actually, this means I may have overused file-truename. If load-history
>> already stores canonical names, then we only need to call truename on
>> the return value of find library, not on the elements of load-history.
>
>
> Look like.
>
>> If nobody else checks, I'll check tomorrow when I get a chance.
>
>
> I'll commit the fix soon (it gives a nice performance boost already), but
> please go ahead and double-check it tomorrow.

Message #68 received at 19390 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Artur Malabarba <bruce.connor.am <at> gmail.com>
Cc: 19390 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#19390: 25.0.50; `package-activate' is too slow
Date: Thu, 18 Dec 2014 02:14:22 +0200

Artur Malabarba <bruce.connor.am <at> gmail.com> writes:

> This will fail if the previously loaded version wasn't installed in the
> package-user-dir,  which, again, is the case for built-in packages. This
> also includes manually installed packages, which we may or may not care
> about supporting.

Good point, thanks. Here's the patch without the initial filtering part.
Still an improvement over the current code (0.6s vs 2.1s on my machine).

(And it drops the use of `file-name-sans-extension').

We could also check the `provide' value in each entry's alist, to make
sure of the match, probably at no major cost.

diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
index 60beebd..90bb514 100644
--- a/lisp/emacs-lisp/package.el
+++ b/lisp/emacs-lisp/package.el
@@ -559,32 +559,26 @@ Return the max version (as a string) if the package is held at a lower version."
   "Recursively list all files in DIR which correspond to loaded features.
 Returns the `file-name-sans-extension' of each file, relative to
 DIR, sorted by most recently loaded last."
-  (let* ((history (mapcar (lambda (x) (file-name-sans-extension (car x)))
-                    load-history))
-         (dir (file-truename dir))
-         ;; List all files that have already been loaded.
-         (list-of-conflicts
-          (remove
-           nil
-           (mapcar
-               (lambda (x) (let* ((file (file-relative-name x dir))
-                             ;; Previously loaded file, if any.
-                             (previous
-                              (ignore-errors
-                                (file-name-sans-extension
-                                 (file-truename (find-library-name file)))))
-                             (pos (when previous (member previous history))))
-                        ;; Return (RELATIVE-FILENAME . HISTORY-POSITION)
-                        (when pos
-                          (cons (file-name-sans-extension file) (length pos)))))
-             (directory-files-recursively dir "\\`[^\\.].*\\.el\\'")))))
-    ;; Turn the list of (FILENAME . POS) back into a list of features.  Files in
-    ;; subdirectories are returned relative to DIR (so not actually features).
-    (let ((default-directory (file-name-as-directory dir)))
-      (mapcar (lambda (x) (file-truename (car x)))
-        (sort list-of-conflicts
-              ;; Sort the files by ascending HISTORY-POSITION.
-              (lambda (x y) (< (cdr x) (cdr y))))))))
+  (let* ((files (directory-files-recursively dir "\\`[^\\.].*\\.el\\'"))
+         (files-re (concat "/"
+                           (regexp-opt
+                            (mapcar (lambda (x)
+                                      (file-relative-name x dir))
+                                    files)
+                            t)
+                           "c?\\'"))
+         list-of-conflicts)
+    ;; List all the matching files from the load history, in
+    ;; historical order.
+    (dolist (entry load-history)
+      (let ((file (car entry)))
+        (when (string-match files-re file)
+          (cl-pushnew (substring (match-string 1 file) 0 -3)
+                      list-of-conflicts
+                      :test #'equal))))
+    ;; Files in subdirectories are returned relative to DIR (so not
+    ;; actually features).
+    list-of-conflicts))
 
 (defun package-built-in-p (package &optional min-version)
   "Return true if PACKAGE is built-in to Emacs.

Message #71 received at 19390 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 19390 <at> debbugs.gnu.org, Artur Malabarba <bruce.connor.am <at> gmail.com>
Subject: Re: bug#19390: 25.0.50; `package-activate' is too slow
Date: Wed, 17 Dec 2014 20:38:14 -0500

> Good point, thanks. Here's the patch without the initial filtering part.
> Still an improvement over the current code (0.6s vs 2.1s on my machine).

Just a side note: the most important optimization is to make sure that
the activation of all the installed packages at startup time is done
without going through this "reload previously loaded files".

IIUC, this is the first optimization that was proposed in this thread,
and is the main one to install (regardless of the others).  Has it
been installed?


        Stefan

Message #74 received at 19390 <at> debbugs.gnu.org (full text, mbox):

From: Artur Malabarba <bruce.connor.am <at> gmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 19390 <at> debbugs.gnu.org, Dmitry Gutov <dgutov <at> yandex.ru>
Subject: Re: bug#19390: 25.0.50; `package-activate' is too slow
Date: Thu, 18 Dec 2014 00:11:23 -0200

[Message part 1 (text/plain, inline)]

On 17 Dec 2014 23:38, "Stefan Monnier" <monnier <at> iro.umontreal.ca> wrote:
>
> > Good point, thanks. Here's the patch without the initial filtering part.
> > Still an improvement over the current code (0.6s vs 2.1s on my machine).
>
> Just a side note: the most important optimization is to make sure that
> the activation of all the installed packages at startup time is done
> without going through this "reload previously loaded files".
>
> IIUC, this is the first optimization that was proposed in this thread,
> and is the main one to install (regardless of the others).  Has it
> been installed?

I agree. And no, IIUC this hasn't been implemented yet. I suggested a
couple of style improvements and haven't heard back.
If nobody has the time to do these improvements right now, then please
apply the originally suggested patch and I'll make the improvements later,
next time I have a few minutes.

[Message part 2 (text/html, inline)]

Message #77 received at 19390 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: bruce.connor.am <at> gmail.com, Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 19390 <at> debbugs.gnu.org
Subject: Re: bug#19390: 25.0.50; `package-activate' is too slow
Date: Thu, 18 Dec 2014 12:37:21 +0200

On 12/18/2014 04:11 AM, Artur Malabarba wrote:

> I agree. And no, IIUC this hasn't been implemented yet. I suggested a
> couple of style improvements and haven't heard back.

Sorry, got sidetracked. Installed with suggestions, except for the last 
docstring line (too obvious IMO).

Still, I think we'd rather not spend too much time on reloading packages 
when upgrading, so please consider my latest patch.

Aside from it, if we compare with the alternative implementation 
suggestions, the current one reloads all dependencies, even those that 
haven't been (re)installed during the current session.

That comes to about ~200 ms if the package in question only depends on 
Helm (44 files). Someone should try that with Org installed from ELPA.

Message #80 received at 19390 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 19390 <at> debbugs.gnu.org, bruce.connor.am <at> gmail.com
Subject: Re: bug#19390: 25.0.50; `package-activate' is too slow
Date: Thu, 18 Dec 2014 09:30:15 -0500

> Aside from it, if we compare with the alternative implementation
> suggestions, the current one reloads all dependencies, even those that
> haven't been (re)installed during the current session.

It sounds serious, but I don't understand what you're referring to.
Can you give an example?


        Stefan

Message #83 received at 19390 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 19390 <at> debbugs.gnu.org, bruce.connor.am <at> gmail.com
Subject: Re: bug#19390: 25.0.50; `package-activate' is too slow
Date: Thu, 18 Dec 2014 16:47:20 +0200

Stefan Monnier <monnier <at> iro.umontreal.ca> writes:

>> Aside from it, if we compare with the alternative implementation
>> suggestions, the current one reloads all dependencies, even those that
>> haven't been (re)installed during the current session.
>
> It sounds serious, but I don't understand what you're referring to.
> Can you give an example?

It reloads all dependencies of the package that is currently being
installed.

Try this:

(advice-add 'package-activate-1 :before
            (lambda (pkg-desc &optional reload)
              (message "package-activate-1 called with %s %s"
                       (package-desc-name pkg-desc) reload))
            '((name . "Parrot arguments")))

Then install company (if your haven't yet), and then, with Melpa in
package archives, install company-math. You'll see this in *Messages*:

package-activate-1 called with company force
package-activate-1 called with math-symbol-lists force
package-activate-1 called with company-math force

Message #86 received at 19390 <at> debbugs.gnu.org (full text, mbox):

From: Artur Malabarba <bruce.connor.am <at> gmail.com>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 19390 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#19390: 25.0.50; `package-activate' is too slow
Date: Thu, 18 Dec 2014 13:15:16 -0200

[Message part 1 (text/plain, inline)]

On 18 Dec 2014 08:37, "Dmitry Gutov" <dgutov <at> yandex.ru> wrote:
>
> On 12/18/2014 04:11 AM, Artur Malabarba wrote:
>
>> I agree. And no, IIUC this hasn't been implemented yet. I suggested a
>> couple of style improvements and haven't heard back.
>
>
> Sorry, got sidetracked. Installed with suggestions, except for the last
docstring line (too obvious IMO).

Awesome :-)

> Still, I think we'd rather not spend too much time on reloading packages
when upgrading, so please consider my latest patch.

There's a bit of a small flaw with that approach, it's the reason I used
find-library.
If you just check load files against their names, you could find a wrong
file that has the same name as a feature (we require files in the load path
to be uniquely named, but load-history contains all files, not just those
in the load path).

It's an edge case, and my opinion is that a good performance improvement is
more important than that. But it seems like the 2 biggest performance
improvements have already been made (the package initialize, and the file
true name), so I wonder if it's worth it.

> Aside from it, if we compare with the alternative implementation
suggestions, the current one reloads all dependencies, even those that
haven't been (re)installed during the current session.

Is that so? Reading your patch, and from what I understand of the current
implementation, they only reload packages on installation. So they
shouldn't reload dependencies that weren't upgraded.

[Message part 2 (text/html, inline)]

Message #89 received at 19390 <at> debbugs.gnu.org (full text, mbox):

From: Artur Malabarba <bruce.connor.am <at> gmail.com>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>, 19390 <at> debbugs.gnu.org
Subject: Re: bug#19390: 25.0.50; `package-activate' is too slow
Date: Thu, 18 Dec 2014 13:39:51 -0200

I see what you meant now. The RELOAD argument should not be passed to
package-activate-1 when activating dependencies. That should fix the
issue.

The dependency will already have been reloaded when (if) it was upgraded.

2014-12-18 12:47 GMT-02:00 Dmitry Gutov <dgutov <at> yandex.ru>:
> Stefan Monnier <monnier <at> iro.umontreal.ca> writes:
>
>>> Aside from it, if we compare with the alternative implementation
>>> suggestions, the current one reloads all dependencies, even those that
>>> haven't been (re)installed during the current session.
>>
>> It sounds serious, but I don't understand what you're referring to.
>> Can you give an example?
>
> It reloads all dependencies of the package that is currently being
> installed.
>
> Try this:
>
> (advice-add 'package-activate-1 :before
>             (lambda (pkg-desc &optional reload)
>               (message "package-activate-1 called with %s %s"
>                        (package-desc-name pkg-desc) reload))
>             '((name . "Parrot arguments")))
>
> Then install company (if your haven't yet), and then, with Melpa in
> package archives, install company-math. You'll see this in *Messages*:
>
> package-activate-1 called with company force
> package-activate-1 called with math-symbol-lists force
> package-activate-1 called with company-math force

Message #92 received at 19390 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: bruce.connor.am <at> gmail.com
Cc: 19390 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#19390: 25.0.50; `package-activate' is too slow
Date: Thu, 18 Dec 2014 17:45:16 +0200

On 12/18/2014 05:15 PM, Artur Malabarba wrote:

> There's a bit of a small flaw with that approach, it's the reason I used
> find-library.
> If you just check load files against their names, you could find a wrong
> file that has the same name as a feature (we require files in the load
> path to be uniquely named, but load-history contains all files, not just
> those in the load path).

Like mentioned, we can also check against the `provide' values in each 
load-history element we find matching. Shouldn't be a perceptible 
performance hit.

> It's an edge case, and my opinion is that a good performance improvement
> is more important than that. But it seems like the 2 biggest performance
> improvements have already been made (the package initialize, and the
> file true name), so I wonder if it's worth it.

200ms per package initialization still seems a lot to me (even if it's 
only for certain packages). I also happen to think that the suggested 
code is a bit easier to understand, but it's up to you.

Message #95 received at 19390 <at> debbugs.gnu.org (full text, mbox):

From: Artur Malabarba <bruce.connor.am <at> gmail.com>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>, 19390 <at> debbugs.gnu.org
Subject: Re: bug#19390: 25.0.50; `package-activate' is too slow
Date: Thu, 18 Dec 2014 13:47:57 -0200

In retrospect, the cleanest way to do do all this would have been to
build this reloading into `require' and `provide' (something would
have to be done with autoloads too). Then package.el could go back to
not worrying about it, and most of this discussion would be moot.

The only problem I see with this is that we might have (much smaller)
performance issues with `package-initialize` again, since `require'
has no way of knowing whether it was invoked from inside
`package-initialize'. The issue would be smaller because handling it
inside `require' could be done more intelligently.

2014-12-18 13:39 GMT-02:00 Artur Malabarba <bruce.connor.am <at> gmail.com>:
> I see what you meant now. The RELOAD argument should not be passed to
> package-activate-1 when activating dependencies. That should fix the
> issue.
>
> The dependency will already have been reloaded when (if) it was upgraded.
>
> 2014-12-18 12:47 GMT-02:00 Dmitry Gutov <dgutov <at> yandex.ru>:
>> Stefan Monnier <monnier <at> iro.umontreal.ca> writes:
>>
>>>> Aside from it, if we compare with the alternative implementation
>>>> suggestions, the current one reloads all dependencies, even those that
>>>> haven't been (re)installed during the current session.
>>>
>>> It sounds serious, but I don't understand what you're referring to.
>>> Can you give an example?
>>
>> It reloads all dependencies of the package that is currently being
>> installed.
>>
>> Try this:
>>
>> (advice-add 'package-activate-1 :before
>>             (lambda (pkg-desc &optional reload)
>>               (message "package-activate-1 called with %s %s"
>>                        (package-desc-name pkg-desc) reload))
>>             '((name . "Parrot arguments")))
>>
>> Then install company (if your haven't yet), and then, with Melpa in
>> package archives, install company-math. You'll see this in *Messages*:
>>
>> package-activate-1 called with company force
>> package-activate-1 called with math-symbol-lists force
>> package-activate-1 called with company-math force

Message #98 received at 19390 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: bruce.connor.am <at> gmail.com
Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>, 19390 <at> debbugs.gnu.org
Subject: Re: bug#19390: 25.0.50; `package-activate' is too slow
Date: Thu, 18 Dec 2014 17:49:54 +0200

On 12/18/2014 05:39 PM, Artur Malabarba wrote:
> I see what you meant now. The RELOAD argument should not be passed to
> package-activate-1 when activating dependencies. That should fix the
> issue.

Right! As long as packages are activated in the right order when 
installed together, this will be a good fix.

Message #101 received at 19390 <at> debbugs.gnu.org (full text, mbox):

From: Artur Malabarba <bruce.connor.am <at> gmail.com>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 19390 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#19390: 25.0.50; `package-activate' is too slow
Date: Thu, 18 Dec 2014 14:14:44 -0200

[Message part 1 (text/plain, inline)]

I see what you mean now. The RELOAD argument should not be passed when
activating dependencies.
The dependency will already have been reloaded when (if) it was upgraded.

Artur Malabarba
On 18 Dec 2014 12:47, "Dmitry Gutov" <dgutov <at> yandex.ru> wrote:

> Stefan Monnier <monnier <at> iro.umontreal.ca> writes:
>
> >> Aside from it, if we compare with the alternative implementation
> >> suggestions, the current one reloads all dependencies, even those that
> >> haven't been (re)installed during the current session.
> >
> > It sounds serious, but I don't understand what you're referring to.
> > Can you give an example?
>
> It reloads all dependencies of the package that is currently being
> installed.
>
> Try this:
>
> (advice-add 'package-activate-1 :before
>             (lambda (pkg-desc &optional reload)
>               (message "package-activate-1 called with %s %s"
>                        (package-desc-name pkg-desc) reload))
>             '((name . "Parrot arguments")))
>
> Then install company (if your haven't yet), and then, with Melpa in
> package archives, install company-math. You'll see this in *Messages*:
>
> package-activate-1 called with company force
> package-activate-1 called with math-symbol-lists force
> package-activate-1 called with company-math force
>

[Message part 2 (text/html, inline)]

Message #104 received at 19390 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: bruce.connor.am <at> gmail.com
Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>, 19390 <at> debbugs.gnu.org
Subject: Re: bug#19390: 25.0.50; `package-activate' is too slow
Date: Thu, 18 Dec 2014 18:15:57 +0200

On 12/18/2014 05:47 PM, Artur Malabarba wrote:
> In retrospect, the cleanest way to do do all this would have been to
> build this reloading into `require' and `provide' (something would

I dunno, seems like if `require' has to consult `find-library-name' each 
time, it would lower its performance.

Maybe even perceptibly, since it's called a lot.

Message #107 received at 19390 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: bruce.connor.am <at> gmail.com
Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>, 19390 <at> debbugs.gnu.org
Subject: Re: bug#19390: 25.0.50; `package-activate' is too slow
Date: Thu, 18 Dec 2014 18:45:11 +0200

On 12/18/2014 05:39 PM, Artur Malabarba wrote:
> The RELOAD argument should not be passed to package-activate-1 when activating dependencies.

And done. Please consider the performance patch at your leisure.

Message #110 received at 19390 <at> debbugs.gnu.org (full text, mbox):

From: Artur Malabarba <bruce.connor.am <at> gmail.com>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 19390 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#19390: 25.0.50; `package-activate' is too slow
Date: Thu, 18 Dec 2014 15:39:30 -0200

[Message part 1 (text/plain, inline)]

On 18 Dec 2014 13:45, "Dmitry Gutov" <dgutov <at> yandex.ru> wrote:
>
> On 12/18/2014 05:15 PM, Artur Malabarba wrote:
>
>> There's a bit of a small flaw with that approach, it's the reason I used
>> find-library.
>> If you just check load files against their names, you could find a wrong
>> file that has the same name as a feature (we require files in the load
>> path to be uniquely named, but load-history contains all files, not just
>> those in the load path).
>
>
> Like mentioned, we can also check against the `provide' values in each
load-history element we find matching. Shouldn't be a perceptible
performance hit.
>

If it's trivial to do, that's certainly good.

>> It's an edge case, and my opinion is that a good performance improvement
>> is more important than that. But it seems like the 2 biggest performance
>> improvements have already been made (the package initialize, and the
>> file true name), so I wonder if it's worth it.
>
>
> 200ms per package initialization still seems a lot to me (even if it's
only for certain packages). I also happen to think that the suggested code
is a bit easier to understand, but it's up to you.

During package-initialize these things add up, so that's certainly a lot.
But during a regular upgrade, what fraction of the total load time does
that amount to? Even for large packages like helm I think this percentage
should be small. But

[Message part 2 (text/html, inline)]

Message #113 received at 19390 <at> debbugs.gnu.org (full text, mbox):

From: Artur Malabarba <bruce.connor.am <at> gmail.com>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 19390 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#19390: 25.0.50; `package-activate' is too slow
Date: Thu, 18 Dec 2014 15:50:28 -0200

[Message part 1 (text/plain, inline)]

On 18 Dec 2014 14:15, "Dmitry Gutov" <dgutov <at> yandex.ru> wrote:
>
> On 12/18/2014 05:47 PM, Artur Malabarba wrote:
>>
>> In retrospect, the cleanest way to do do all this would have been to
>> build this reloading into `require' and `provide' (something would
>
>
> I dunno, seems like if `require' has to consult `find-library-name' each
time, it would lower its performance.
>
> Maybe even perceptibly, since it's called a lot.

There's a way we can implement this so that find library and file-truename
have to be called exactly once per require (and file-truename is called
once per provide).
Yes, there will be a performance hit, but one could argue that is the way
require should have been done from the start (the "right way").

I don't think this performance hit will be noticeable during regular
package loading. How expensive is it to call find-library and file-truename
once per require, compared to the time it takes to actually load (possibly
byte-compile) the entire library that's calling these requires?

The performance difference will certainly be noticeable during
package-initialize. Like I mentioned above, that's an important issue.

[Message part 2 (text/html, inline)]

Message #116 received at 19390 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: bruce.connor.am <at> gmail.com
Cc: 19390 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#19390: 25.0.50; `package-activate' is too slow
Date: Thu, 18 Dec 2014 19:52:18 +0200

On 12/18/2014 07:39 PM, Artur Malabarba wrote:

> During package-initialize these things add up, so that's certainly a
> lot. But during a regular upgrade, what fraction of the total load time
> does that amount to? Even for large packages like helm I think this
> percentage should be small. But

                                 ^^
The message cuts off here.

Message #119 received at 19390 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: bruce.connor.am <at> gmail.com
Cc: 19390 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#19390: 25.0.50; `package-activate' is too slow
Date: Thu, 18 Dec 2014 19:54:31 +0200

On 12/18/2014 07:50 PM, Artur Malabarba wrote:

> I don't think this performance hit will be noticeable during regular
> package loading.

It'll probably slow down Emacs startup: `require' will have to call 
`find-library-name' for each common feature that someone depends on, 
each time a dependant is loaded.

Message #122 received at 19390 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Artur Malabarba <bruce.connor.am <at> gmail.com>
Cc: 19390 <at> debbugs.gnu.org, Dmitry Gutov <dgutov <at> yandex.ru>
Subject: Re: bug#19390: 25.0.50; `package-activate' is too slow
Date: Thu, 18 Dec 2014 13:37:39 -0500

> build this reloading into `require' and `provide' (something would
> have to be done with autoloads too).

No, that's too high-level.


        Stefan

Message #125 received at 19390 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 19390 <at> debbugs.gnu.org, bruce.connor.am <at> gmail.com
Subject: Re: bug#19390: 25.0.50; `package-activate' is too slow
Date: Thu, 18 Dec 2014 13:40:37 -0500

>> I don't think this performance hit will be noticeable during regular
>> package loading.
> It'll probably slow down Emacs startup: `require' will have to call
> `find-library-name' for each common feature that someone depends on, each
> time a dependant is loaded.

And more importantly, we suffer the slow down when require is supposed to
be fast (i.e. when the package has already been loaded), so the relative
speed impact is not negligible.

And doing it in `require' doesn't catch all cases, because it's done too
lazily and doesn't account for packages loaded via autoloads.  Doing the
reloads eagerly when a package is (re)activated is the better choice.


        Stefan

Message #128 received at 19390 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 19390 <at> debbugs.gnu.org, bruce.connor.am <at> gmail.com
Subject: Re: bug#19390: 25.0.50; `package-activate' is too slow
Date: Thu, 18 Dec 2014 13:42:08 -0500

> 200ms per package initialization still seems a lot to me (even if it's only
> for certain packages).

We should only pay this price at the moment we upgrade a library, so the
cost should be negligible compared to downloading&byte-compiling.


        Stefan

Message #131 received at 19390 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 19390 <at> debbugs.gnu.org, bruce.connor.am <at> gmail.com
Subject: Re: bug#19390: 25.0.50; `package-activate' is too slow
Date: Thu, 18 Dec 2014 20:45:52 +0200

On 12/18/2014 08:42 PM, Stefan Monnier wrote:

> We should only pay this price at the moment we upgrade a library, so the
> cost should be negligible compared to downloading&byte-compiling.

That is true.

Message #134 received at 19390 <at> debbugs.gnu.org (full text, mbox):

From: Artur Malabarba <bruce.connor.am <at> gmail.com>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 19390 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#19390: 25.0.50; `package-activate' is too slow
Date: Thu, 18 Dec 2014 17:01:37 -0200

Sorry. The rest of the message was just babling anyway.
I was just saying the performance hit of the current implementation is
negligible when upgrading.
Which is the same thing Stefan is saying.

2014-12-18 15:52 GMT-02:00 Dmitry Gutov <dgutov <at> yandex.ru>:
> On 12/18/2014 07:39 PM, Artur Malabarba wrote:
>
>> During package-initialize these things add up, so that's certainly a
>> lot. But during a regular upgrade, what fraction of the total load time
>> does that amount to? Even for large packages like helm I think this
>> percentage should be small. But
>
>
>                                  ^^
> The message cuts off here.

Message #137 received at 19390 <at> debbugs.gnu.org (full text, mbox):

From: Artur Malabarba <bruce.connor.am <at> gmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 19390 <at> debbugs.gnu.org, Dmitry Gutov <dgutov <at> yandex.ru>
Subject: Re: bug#19390: 25.0.50; `package-activate' is too slow
Date: Thu, 18 Dec 2014 17:13:31 -0200

2014-12-18 16:40 GMT-02:00 Stefan Monnier <monnier <at> iro.umontreal.ca>:
>>> I don't think this performance hit will be noticeable during regular
>>> package loading.
>> It'll probably slow down Emacs startup: `require' will have to call
>> `find-library-name' for each common feature that someone depends on, each
>> time a dependant is loaded.
>
> And more importantly, we suffer the slow down when require is supposed to
> be fast (i.e. when the package has already been loaded), so the relative
> speed impact is not negligible.

Again, is this slow down really that much?
It is a single call to `find-library'.

Yes, it is slow compared to the current speed of `require'ing an
already loaded package, but is it slow in perspective with what else
is being done?
`require' is only ever called as part of loading package (or doing
some other processing). Is a single call to `find-library' really that
slow compared to the rest of the package that will be loaded (or
byte-compiled) after this require?

>
> And doing it in `require' doesn't catch all cases, because it's done too
> lazily and doesn't account for packages loaded via autoloads.  Doing the
> reloads eagerly when a package is (re)activated is the better choice.

Yes, that's why I said we'd need to do something with `autoload's. Why
do you think that's too high level?

When `autoload' is called, if the function in question is completely
defined (instead of not defined or just autoloaded), `autoload' will
check if the file it was given is a different file from the one the
function was defined in (when we know that file).
If it is then all is fine, if it wasn't then it loads the new file.

Message #142 received at 19390-done <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: bruce.connor.am <at> gmail.com
Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>, 19390-done <at> debbugs.gnu.org
Subject: Re: bug#19390: 25.0.50; `package-activate' is too slow
Date: Thu, 18 Dec 2014 21:49:25 +0200

On 12/18/2014 09:01 PM, Artur Malabarba wrote:
> Sorry. The rest of the message was just babling anyway.
> I was just saying the performance hit of the current implementation is
> negligible when upgrading.
> Which is the same thing Stefan is saying.

Oh well. I also wrote that my implementation is shorter and somewhat 
easier to read, but since no one else seems to think so, closing.

Message #145 received at 19390 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Artur Malabarba <bruce.connor.am <at> gmail.com>
Cc: 19390 <at> debbugs.gnu.org, Dmitry Gutov <dgutov <at> yandex.ru>
Subject: Re: bug#19390: 25.0.50; `package-activate' is too slow
Date: Thu, 18 Dec 2014 23:31:30 -0500

> `require' is only ever called as part of loading package (or doing
> some other processing).

There are a few places where we `require' inside a function that can be
called repeatedly, on the assumption that in most cases, this `require'
will be "instantaneous".  I'd rather not re-consider this
performance choice.

I'm OK with slowing down require like you suggested (that was actually
my original suggestion to handle the byte-compilation breakage during
package upgrade), but then we'd only want to do it in some specific
contexts (during byte-compilation) rather than all the time.

> When `autoload' is called, if the function in question is completely
> defined (instead of not defined or just autoloaded), `autoload' will
> check if the file it was given is a different file from the one the
> function was defined in (when we know that file).
> If it is then all is fine, if it wasn't then it loads the new file.

So we first add handling for `require', then another for `autoload',
... in the end I can't believe it can be simpler than what we have
(which has 0 cost in normal use, and a very small extra cost during
package upgrade).


        Stefan

Message #148 received at 19390 <at> debbugs.gnu.org (full text, mbox):

From: Artur Malabarba <bruce.connor.am <at> gmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 19390 <at> debbugs.gnu.org, Dmitry Gutov <dgutov <at> yandex.ru>
Subject: Re: bug#19390: 25.0.50; `package-activate' is too slow
Date: Fri, 19 Dec 2014 10:08:08 -0200

[Message part 1 (text/plain, inline)]

On 19 Dec 2014 02:31, "Stefan Monnier" <monnier <at> iro.umontreal.ca> wrote:
>
> > `require' is only ever called as part of loading package (or doing
> > some other processing).
>
> There are a few places where we `require' inside a function that can be
> called repeatedly, on the assumption that in most cases, this `require'
> will be "instantaneous".  I'd rather not re-consider this
> performance choice.
>
> I'm OK with slowing down require like you suggested (that was actually
> my original suggestion to handle the byte-compilation breakage during
> package upgrade), but then we'd only want to do it in some specific
> contexts (during byte-compilation) rather than all the time.
>
> > When `autoload' is called, if the function in question is completely
> > defined (instead of not defined or just autoloaded), `autoload' will
> > check if the file it was given is a different file from the one the
> > function was defined in (when we know that file).
> > If it is then all is fine, if it wasn't then it loads the new file.
>
> So we first add handling for `require', then another for `autoload',
> ... in the end I can't believe it can be simpler than what we have
> (which has 0 cost in normal use, and a very small extra cost during
> package upgrade).

In terms of lines of code, I think it would end up simpler, despite having
to patch 3 functions instead of 1. Still, I understand it's too late now to
be slowing down `require'. If there's code in Emacs core that relies on its
speed, God knows how much code like that is floating around on the other
Elpas or the wiki.

Better stick with the version we have now.

[Message part 2 (text/html, inline)]

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.