Re: 28.0.50; Proposal: slightly more efficient package-quickstart.el

[Arthur Miller]

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

>> Not so much, but it is not so much about noticable difference, more
>> about not performing unnecessary computation.
I agree that simplicity and code clarity is important, on many
levels. But maybe we can have the cake and it it too, as you said 
for wdired?

> But Emacs is more often limited by manpower than CPU, so the impact on
> code complexity should not be discounted.
>
>> As the loop progresses, for every dir N, the loop does N-1 path
>> comparisons. These are unnecessary comparisons since when we know for
>> sure that the string is not added yet. Trend in Emacs seems to go
>> towards having more and more packages installed. This removes
>> N(N-1)) comparisons.
>
> I know.  But you have to get to a list of at least 2000 elements
> before this N² starts making a noticable (0.2s) difference [tested on
> an old 1GHz Cortex-A8.  ]

Of course. I agree that number of packages has to be big to see the
noticable difference. I don't know though how high it shold be. I do a
lot of testing in my own init file, but I can't say anything for sure,
since I do some weird stuff that are not representable of "normal" Emacs
usage.

> By the time the user has 2000 packages installed, there are several
> other performance problems that will be more noticeable, I'm afraid.
> E.g. the length of `load-path` itself will be a source of slowdown.
>
> I know 200 packages is fairly common nowadays, so 1000 or more in not an
> outlandish idea, and there are indeed performance issues that come up
> when we have many packages, but I suspect that we'll need more thorough
> changes to tackle this, and in any case it should be driven by concrete
> measurements, otherwise we'll waste our time optimizing details that
> don't actually matter.
Last weekend I tested actually myself to restructure how my packages are
loaded. I noticed that init time increased after I added ~100 packages,
just for test, and I didn't required anything of that into Emacs. So I
tested the idea to put all .elc file into a single place and skipp in
entirety this monstrosity of load-path that results after 200 packages
are loaded. I got it to work to a degree, it least I got running Emacs,
native compiler not complaining and most packages loaded, but I also got
some cyclic dependency, notably for dired and semantic of all things,
that actually rendered entire session unusable for the most part. I'll
leave that for another day when I have some more time.

>>> Beside the hypothetical risk that the regexp matches an `add-to-list` to
>>> an unrelated variable, I think this risks introducing real bugs for
>>> packages which use (add-to-list 'load-path <subdir>) to add
>>> *sub*directories.
>> Yes, I was aware of it, but I am not sure how to write the regex, to
>> avoid that risk.
>
> I don't think you can avoid that problem by fixing the regexp, but
> rather by `read`ing the sexp and looking at the directory it adds to
> make sure it is indeed one of the ones you're adding "by hand".
The problem is that .* matches everything but new line as I learned form
Emacs Wiki. After fixing regexp to match the newline, it blows the regex
stack as warned on the wiki page.

I have encountered another problem, I hope you can explain it, because I
don't understand what is going on. When I run this piece which actually
tries to match adding to load path: 

(when (re-search-forward rx-path-beg nil t)
            (goto-char (line-beginning-position))
            (setq temp-point (point))
            (forward-sexp)
            (when (search-backward file nil t 1)
              (goto-char temp-point)
              (kill-sexp)))

In search backward, regardless of which bound I give to
re-search-backward or just search-backward, the search failes. My
original thought was to use (line-beginning-position) and I also tested
this temp-point. Interesting is that same code works fine from M-:. If I
setq those variables I need to run that little piece from M-: and position
point in a spot before a statement to remove, it works fine, but in the
defun it always fails so I had to run it with nil for the bound. Is it
something I don't understand there or a bug? 

(when (re-search-forward rx-path-beg nil t)
            (goto-char (line-beginning-position))
            (setq temp-point (point))
            (forward-sexp)
            (when (search-backward file (line-beginning-position)  t 1)
              (goto-char temp-point)
              (kill-sexp)))

> Or maybe a better approach is to do something like what we do with
> `Info-directory-list`, tho it might also be tricky to "get it right".
>
>>> Of course, we can fix those problems, but unless there's a compelling
>>> performance argument, I think it's a waste of time.
>> It's quite simple to do,
>
> It's not if you want to handle correctly the corner cases, as this email
> discussion shows.
>
>> I don't know, at least in theory :).
>
> Maybe there's a theoretical gain.  But there's a very real practical
> loss in time spent getting the code to work correctly and maintaining it
> in the future.

I understand what you mean, it is up to you to decide. The patch does add
few lines, but it is not some tricky, advanced code hard to
understand. I could also refactor some code out of
package-quickstart-refresh into a smaller support defun if that would 
make things easier to maintain.

By the way, I haven't even touched on those empty let closures, that
really just produce work for garbage collector in most cases. I think I
have seen 2 packages that used some of those two vars that get declared
in those let statements. :)

package.patch
Description: Text Data