unofficial mirror of emacs-devel@gnu.org 
 help / color / mirror / code / Atom feed
From: Stefan Monnier <monnier@iro.umontreal.ca>
To: Alan Mackenzie <acm@muc.de>
Cc: Glenn Morris <rgm@gnu.org>, Chong Yidong <cyd@stupidchicken.com>,
	emacs-devel@gnu.org
Subject: Re: Refactoring of emacs-lisp/autoload.el
Date: Tue, 12 Aug 2008 16:09:04 -0400	[thread overview]
Message-ID: <jwvskta815n.fsf-monnier+emacs@gnu.org> (raw)
In-Reply-To: <20080812162333.GA7999@muc.de> (Alan Mackenzie's message of "Tue,  12 Aug 2008 16:23:33 +0000")

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




  reply	other threads:[~2008-08-12 20:09 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2008-08-12 16:23 Refactoring of emacs-lisp/autoload.el Alan Mackenzie
2008-08-12 20:09 ` Stefan Monnier [this message]
2008-08-12 20:58   ` Glenn Morris
2008-08-13 14:13     ` Alan Mackenzie
2008-08-13 16:11       ` Glenn Morris
2008-08-13 14:11   ` Alan Mackenzie
2008-08-13 20:31     ` Stefan Monnier
2008-08-14 13:17       ` Alan Mackenzie
2008-08-14 17:52         ` Stefan Monnier
2008-09-01 21:55           ` Alan Mackenzie

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: https://www.gnu.org/software/emacs/

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=jwvskta815n.fsf-monnier+emacs@gnu.org \
    --to=monnier@iro.umontreal.ca \
    --cc=acm@muc.de \
    --cc=cyd@stupidchicken.com \
    --cc=emacs-devel@gnu.org \
    --cc=rgm@gnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).