Re: Refactoring of emacs-lisp/autoload.el

[Stefan Monnier]

> #########################################################################
> (i) It is better structured:
>   o - the functions are coherently single purpose to a greater extent than
>     before;
>   o - the output file handling has been separated from source file
>     considerations;
>   o - two `catch' constructs (~120 and ~80 lines) have been eliminated.

Sounds very good.

> (ii) It produces more consistent result in the comment sections of loaddefs.el
>   etc.  In particular:
>   o - The lines that identify the source file now always (rather than just
>     sometimes) give a file name relative to the "starting directory" (usually
>     .../lisp).  E.g.:

> *** calc/calc-loaddefs.el.old   2008-08-12 13:17:32.983291472 +0000
> --- calc/calc-loaddefs.el.new   2008-08-12 15:00:26.468779024 +0000
> ***************
> *** 9,16 ****
>   ;;;;;;  math-read-preprocess-string calcDigit-edit calcDigit-algebraic
>   ;;;;;;  calc-alg-digit-entry calc-do-alg-entry calc-alg-entry 
> calc-algebraic-entry
>   ;;;;;;  calc-auto-algebraic-entry calc-do-calc-eval calc-do-quick-calc)
> ! ;;;;;;  "calc-aent" "calc-aent.el" "397561d73c948bd9256db97c177e84f6")
> ! ;;; Generated autoloads from calc-aent.el
  
>   (autoload 'calc-do-quick-calc "calc-aent" "\
>   Not documented
> --- 9,16 ----
>   ;;;;;;  math-read-preprocess-string calcDigit-edit calcDigit-algebraic
>   ;;;;;;  calc-alg-digit-entry calc-do-alg-entry calc-alg-entry 
> calc-algebraic-entry
>   ;;;;;;  calc-auto-algebraic-entry calc-do-calc-eval calc-do-quick-calc)
> ! ;;;;;;  "calc-aent" "calc/calc-aent.el" "397561d73c948bd9256db97c177e84f6")
> ! ;;; Generated autoloads from calc/calc-aent.el

I'm not sure why you think it's an improvement: the previous behavior
was to use a file name relate to where the name was placed
(i.e. relative to the file in which the entry is created).  In my
opinions, relative file names placed in files should generally be
relative to the file in which they appear rather than relative to the
"top of the project".

Of course, there might be a good reason for your change, so I'm not
necessarily opposed to this.  More specifically, I think in this
particular instance it does not matter either way, so I don't have
a strong opinion.  I'd still be interested to hear your reason for
this change.  Just a personal preference?  Fix an actual problem?
Simplifies the code?

>   o - The final section (which records files which had no autoload symbols) no
>     longer includes any files for which there is a normal section higher up.
>     For example, in lisp/loaddefs.el at the moment, "calc/calc-aent.el"
>     violates this rule.  I have assumed that this is a bug.

No, this was not a bug.  This section is present to speed up the refresh
of the loaddefs.el file, so that files that don't have other entries in
loaddefs.el don't have to be opened&scanned when they haven't changed
since the last time we refreshed loaddefs.el.
So calc/calc-aent.el should probably be mentioned in this list so as to
avoid having to open&scan it when it hasn't changed.

> (iii) The new autoload.el runs quite a lot faster than the old one.  :-)  Here
>   are some comparitive timings, done under fair conditions on my 1.2 GHz
>   Athlon box:

>   OLD:                                    NEW:
>   real    1m11.502s                       real    0m40.729s
>   user    0m55.141s                       user    0m24.519s
>   sys     0m15.981s                       sys     0m15.998s

There are 2 ways to run this code: one is to rebuild loaddefs.el from
scratch, the other is to update a preexisting loaddefs.el (typically
just after "cvs update").  With "make bootstrap" you care about the
first, otherwise you care about the second.  I.e. I care about the
second ;-)

The second case used to be unusably slow and I made it a lot faster
around Emacs-21 (or so), so now it's fast enough: please test it after
changing a handful of files, just to make sure that it's not
significantly slower than before.

>   This is a speedup of ~75%.

That's great.  In parallel bootstrap builds, the loaddefs.el is sometimes
on the critical path, so that's a very welcome improvement.

I can't easily browse your patch with the machine I'm using right now,
so I can't comment on the actual code.  The current code (mostly due to
yours truly, sadly) is not very satisfactory indeed, so as long as the
new code is fully compatible with the various ways autoload.el is used
(i..e not just by Emacs itself but by other external 3rd party
packages), and as long as it doesn't significantly slow down the "no/few
changes" case, I see no reason not to install it,


        Stefan