bug#19390: 25.0.50; `package-activate' is too slow

bug#19390: 25.0.50; `package-activate' is too slow

[Dmitry Gutov]

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

bug#19390: Acknowledgement (25.0.50; `package-activate' is too slow)

[Dmitry Gutov]

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

bug#19390: 25.0.50; `package-activate' is too slow

[Dmitry Gutov]

Arthur, do you experience this problem?

What's your opinion on the fix?

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

bug#19390: 25.0.50; `package-activate' is too slow

[Artur Malabarba]

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

bug#19390: 25.0.50; `package-activate' is too slow

[Artur Malabarba]

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@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'."

bug#19390: 25.0.50; `package-activate' is too slow

[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?

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

bug#19390: 25.0.50; `package-activate' is too slow

[Artur Malabarba]

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.

bug#19390: 25.0.50; `package-activate' is too slow

[Stefan Monnier]

> 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

bug#19390: 25.0.50; `package-activate' is too slow

[Dmitry Gutov]

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.

bug#19390: 25.0.50; `package-activate' is too slow

[Stefan Monnier]

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

bug#19390: 25.0.50; `package-activate' is too slow

[Artur Malabarba]

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

On 16 Dec 2014 21:55, "Stefan Monnier" <monnier@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.

[-- Attachment #2: Type: text/html, Size: 1139 bytes --]

bug#19390: 25.0.50; `package-activate' is too slow

[Dmitry Gutov]

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.

bug#19390: 25.0.50; `package-activate' is too slow

[Artur Malabarba]

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

bug#19390: 25.0.50; `package-activate' is too slow

[Dmitry Gutov]

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.

bug#19390: 25.0.50; `package-activate' is too slow

[Artur Malabarba]

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

On 17 Dec 2014 00:41, "Dmitry Gutov" <dgutov@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.

[-- Attachment #2: Type: text/html, Size: 1035 bytes --]

bug#19390: 25.0.50; `package-activate' is too slow

[Dmitry Gutov]

Artur Malabarba <bruce.connor.am@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.

bug#19390: 25.0.50; `package-activate' is too slow

[Stefan Monnier]

> 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

bug#19390: 25.0.50; `package-activate' is too slow

[Artur Malabarba]

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

On 17 Dec 2014 23:38, "Stefan Monnier" <monnier@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.

[-- Attachment #2: Type: text/html, Size: 1133 bytes --]

bug#19390: 25.0.50; `package-activate' is too slow

[Dmitry Gutov]

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.

bug#19390: 25.0.50; `package-activate' is too slow

[Stefan Monnier]

> 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

bug#19390: 25.0.50; `package-activate' is too slow

[Dmitry Gutov]

Stefan Monnier <monnier@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

bug#19390: 25.0.50; `package-activate' is too slow

[Artur Malabarba]

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@yandex.ru>:
> Stefan Monnier <monnier@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

bug#19390: 25.0.50; `package-activate' is too slow

[Artur Malabarba]

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@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@yandex.ru>:
>> Stefan Monnier <monnier@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

bug#19390: 25.0.50; `package-activate' is too slow

[Dmitry Gutov]

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.

bug#19390: 25.0.50; `package-activate' is too slow

[Artur Malabarba]

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

On 18 Dec 2014 14:15, "Dmitry Gutov" <dgutov@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.

[-- Attachment #2: Type: text/html, Size: 1420 bytes --]

bug#19390: 25.0.50; `package-activate' is too slow

[Dmitry Gutov]

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.

bug#19390: 25.0.50; `package-activate' is too slow

[Stefan Monnier]

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

bug#19390: 25.0.50; `package-activate' is too slow

[Artur Malabarba]

2014-12-18 16:40 GMT-02:00 Stefan Monnier <monnier@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.

bug#19390: 25.0.50; `package-activate' is too slow

[Stefan Monnier]

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

bug#19390: 25.0.50; `package-activate' is too slow

[Artur Malabarba]

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

On 19 Dec 2014 02:31, "Stefan Monnier" <monnier@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.

[-- Attachment #2: Type: text/html, Size: 2032 bytes --]

bug#19390: 25.0.50; `package-activate' is too slow

[Stefan Monnier]

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

No, that's too high-level.


        Stefan

bug#19390: 25.0.50; `package-activate' is too slow

[Dmitry Gutov]

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.

bug#19390: 25.0.50; `package-activate' is too slow

[Dmitry Gutov]

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.

bug#19390: 25.0.50; `package-activate' is too slow

[Artur Malabarba]

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

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@yandex.ru> wrote:

> Stefan Monnier <monnier@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
>

[-- Attachment #2: Type: text/html, Size: 1796 bytes --]

bug#19390: 25.0.50; `package-activate' is too slow

[Artur Malabarba]

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

On 18 Dec 2014 08:37, "Dmitry Gutov" <dgutov@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.

[-- Attachment #2: Type: text/html, Size: 1776 bytes --]

bug#19390: 25.0.50; `package-activate' is too slow

[Dmitry Gutov]

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.

bug#19390: 25.0.50; `package-activate' is too slow

[Artur Malabarba]

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

On 18 Dec 2014 13:45, "Dmitry Gutov" <dgutov@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

[-- Attachment #2: Type: text/html, Size: 1746 bytes --]

bug#19390: 25.0.50; `package-activate' is too slow

[Dmitry Gutov]

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.

bug#19390: 25.0.50; `package-activate' is too slow

[Artur Malabarba]

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

bug#19390: 25.0.50; `package-activate' is too slow

[Dmitry Gutov]

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.

bug#19390: 25.0.50; `package-activate' is too slow

[Stefan Monnier]

> 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

bug#19390: 25.0.50; `package-activate' is too slow

[Dmitry Gutov]

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.

bug#19390: 25.0.50; `package-activate' is too slow

[Dmitry Gutov]

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.

bug#19390: 25.0.50; `package-activate' is too slow

[Artur Malabarba]

2014-12-16 14:53 GMT-02:00 Dmitry Gutov <dgutov@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.

bug#19390: 25.0.50; `package-activate' is too slow

[Dmitry Gutov]

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.

bug#19390: 25.0.50; `package-activate' is too slow

[Stefan Monnier]

> 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

bug#19390: 25.0.50; `package-activate' is too slow

[Dmitry Gutov]

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.

bug#19390: 25.0.50; `package-activate' is too slow

[Stefan Monnier]

> 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