bug#4446: remove ###autoload from ediff-hook.el

bug#4446: remove ###autoload from ediff-hook.el

[Dan Nicolaescu]

The ###autoload in ediff-hook.el is not needed, this file is in the
dumped image, so the autoload's only effect is to make loaddefs.el
bigger and increase the size of the dumped image.

The comment in the file says that the autoload is needed for XEmacs, but
that's not a good reason to keep it in the Emacs CVS, it's a one line
change to a file that almost never changes.

OK to remove the autoload line?

bug#4446: remove ###autoload from ediff-hook.el

[Leo]

On 2009-09-16 12:18 +0100, Dan Nicolaescu wrote:
> The ###autoload in ediff-hook.el is not needed, this file is in the
> dumped image, so the autoload's only effect is to make loaddefs.el
> bigger and increase the size of the dumped image.
>
> The comment in the file says that the autoload is needed for XEmacs, but
> that's not a good reason to keep it in the Emacs CVS, it's a one line
> change to a file that almost never changes.
>
> OK to remove the autoload line?

That is a big chunk of autoload.

bug#4446: remove ###autoload from ediff-hook.el

[Stefan Monnier]

> The ###autoload in ediff-hook.el is not needed, this file is in the
> dumped image, so the autoload's only effect is to make loaddefs.el
> bigger and increase the size of the dumped image.

> The comment in the file says that the autoload is needed for XEmacs, but
> that's not a good reason to keep it in the Emacs CVS, it's a one line
> change to a file that almost never changes.

> OK to remove the autoload line?

Actually, it reminds me that I faced a similar situation recently.
I did remove the autoload cookie, but in retrospect I think it was the
wrong thing to do.  We should instead teach our build system to skip
preloaded files when creating the loaddefs.el file.


        Stefan

bug#4446: remove ###autoload from ediff-hook.el

[Dan Nicolaescu]

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

  > > The ###autoload in ediff-hook.el is not needed, this file is in the
  > > dumped image, so the autoload's only effect is to make loaddefs.el
  > > bigger and increase the size of the dumped image.
  > 
  > > The comment in the file says that the autoload is needed for XEmacs, but
  > > that's not a good reason to keep it in the Emacs CVS, it's a one line
  > > change to a file that almost never changes.
  > 
  > > OK to remove the autoload line?
  > 
  > Actually, it reminds me that I faced a similar situation recently.
  > I did remove the autoload cookie, but in retrospect I think it was the
  > wrong thing to do.  We should instead teach our build system to skip
  > preloaded files when creating the loaddefs.el file.

I got rid of most of the autoloads in the preloaded files a couple of
years ago, this must have fallen through the cracks.
Skipping register.el while creating loaddefs.el would be wrong, nothing
else would create the register bindings then.
And there's 3 autoloads more in composite.el, that's all the autoloads
in the unconditionally preloaded files.
Given that there's so few autoloads left, it does not seem like a good
idea to change the build system instead of just removing them.

More, for the specific case of ediff-hook.el, IMO the best thing to do
is got fold the emacs specific contents of that file into menu-bar.el
and get rid of it.  Almost all of the menu creation happens in menu-bar.el...

bug#4446: remove ###autoload from ediff-hook.el

[Glenn Morris]

Dan Nicolaescu wrote:

> The ###autoload in ediff-hook.el is not needed
[...]
> OK to remove the autoload line?

The last time this question was asked, the ediff maintainer asked for
it not to be removed:

http://lists.gnu.org/archive/html/emacs-devel/2008-03/msg00896.html

bug#4446: remove ###autoload from ediff-hook.el

[Dan Nicolaescu]

Glenn Morris <rgm@gnu.org> writes:

  > Dan Nicolaescu wrote:
  > 
  > > The ###autoload in ediff-hook.el is not needed
  > [...]
  > > OK to remove the autoload line?
  > 
  > The last time this question was asked, the ediff maintainer asked for
  > it not to be removed:
  > 
  > http://lists.gnu.org/archive/html/emacs-devel/2008-03/msg00896.html

Hmm, I forgot about this.
But removing a single line in a file that hardly ever changes does not
seem like a bit maintenance burden.
On the other hand the autoload adds ~10KB to all emacs downloads, plus a
few symbols that are defined and never ever used (but need to be GCed
every single time for ALL users).

bug#4446: remove ###autoload from ediff-hook.el

[Stefan Monnier]

>> > The ###autoload in ediff-hook.el is not needed
>> [...]
>> > OK to remove the autoload line?
>> 
>> The last time this question was asked, the ediff maintainer asked for
>> it not to be removed:
>> 
>> http://lists.gnu.org/archive/html/emacs-devel/2008-03/msg00896.html

> Hmm, I forgot about this.
> But removing a single line in a file that hardly ever changes does not
> seem like a bit maintenance burden.

That's only because Michael is the only maintainer who stood strong.
Many other preloaded files would be happy to keep
their ;;;###autoload cookies.  They're a form of documentation and make
it easier to switch back&forth between preloaded and non-preloaded.

> On the other hand the autoload adds ~10KB to all emacs downloads, plus a
> few symbols that are defined and never ever used (but need to be GCed
> every single time for ALL users).

That's why those cookies should be skipped for preloaded files.


        Stefan

bug#4446: remove ###autoload from ediff-hook.el

[Glenn Morris]

Stefan Monnier wrote:

> That's why those cookies should be skipped for preloaded files.

Any ideas on how to achieve this?

The list of preloaded files is not readily obtainable until after
emacs has been dumped, and dumping requires loaddefs.el, so it's kind
of circular.

I can only think of something cheesy, like evaling loadup.el with
`load' temporarily redefined.

bug#4446: remove ###autoload from ediff-hook.el

[Stefan Monnier]

>> That's why those cookies should be skipped for preloaded files.
> Any ideas on how to achieve this?
> The list of preloaded files is not readily obtainable until after
> Emacs has been dumped, and dumping requires loaddefs.el, so it's kind
> of circular.

Luckily, ldefs-boot.el saves the day:  Tada!


        Stefan

bug#4446: remove ###autoload from ediff-hook.el

[Glenn Morris]

Stefan Monnier wrote:

> Luckily, ldefs-boot.el saves the day:  Tada!

Huh, for some reason I thought ldefs-boot wasn't used anymore during
bootstrap, but it is. I thought this was therefore going to be
straightforward, but because loadup.el sets purify-flag nil in
bootstrap-emacs, when the autoloads are made, preloaded-file-list only
contains loadup.el.

bug#4446: remove ###autoload from ediff-hook.el

[Stefan Monnier]

>> Luckily, ldefs-boot.el saves the day:  Tada!
> Huh, for some reason I thought ldefs-boot wasn't used anymore during
> bootstrap, but it is. I thought this was therefore going to be
> straightforward, but because loadup.el sets purify-flag nil in
> bootstrap-emacs, when the autoloads are made, preloaded-file-list only
> contains loadup.el.

BTW, the list of preloaded files is already encoded in src/Makefile.in.
Or you could otherwise extract it statically with something like

   sed -n -e 's/.*(load "\(.*\)".*/\1/p' lisp/loadup.el


-- Stefan

bug#4446: remove ###autoload from ediff-hook.el

[Dan Nicolaescu]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

  > >> Luckily, ldefs-boot.el saves the day:  Tada!
  > > Huh, for some reason I thought ldefs-boot wasn't used anymore during
  > > bootstrap, but it is. I thought this was therefore going to be
  > > straightforward, but because loadup.el sets purify-flag nil in
  > > bootstrap-emacs, when the autoloads are made, preloaded-file-list only
  > > contains loadup.el.
  > 
  > BTW, the list of preloaded files is already encoded in src/Makefile.in.
  > Or you could otherwise extract it statically with something like
  > 
  >    sed -n -e 's/.*(load "\(.*\)".*/\1/p' lisp/loadup.el

This fails, it won't generate autoloads for emacs-lisp/easymenu.el, and
those autoloads are needed.

bug#4446: remove ###autoload from ediff-hook.el

[Stefan Monnier]

>> > Huh, for some reason I thought ldefs-boot wasn't used anymore during
>> > bootstrap, but it is. I thought this was therefore going to be
>> > straightforward, but because loadup.el sets purify-flag nil in
>> > bootstrap-emacs, when the autoloads are made, preloaded-file-list only
>> > contains loadup.el.
>> BTW, the list of preloaded files is already encoded in src/Makefile.in.
>> Or you could otherwise extract it statically with something like
>> sed -n -e 's/.*(load "\(.*\)".*/\1/p' lisp/loadup.el
> This fails, it won't generate autoloads for emacs-lisp/easymenu.el, and
> those autoloads are needed.

Then you want the list that's in src/Makefile.in, since it includes the
distinction between those files that are preloaded on all systems vs
those that are system-dependent.


        Stefan

bug#4446: remove ###autoload from ediff-hook.el

[Glenn Morris]

Stefan Monnier wrote:

> Then you want the list that's in src/Makefile.in, since it includes the
> distinction between those files that are preloaded on all systems vs
> those that are system-dependent.

That's a good suggestion, thanks.

I found that setting preloaded-file-list in bootstrap-emacs does not
work anyway. At that stage files are being loaded uncompiled, so extra
files (eg cl) end up being added to preloaded-file-list due to
eval-when-compile.

Anyway, here's an attempt at your suggestion. The final version
probably should not print the "skipped" message.


Index: Makefile.in
===================================================================
RCS file: /sources/emacs/emacs/lisp/Makefile.in,v
retrieving revision 1.193
diff -c -c -w -r1.193 Makefile.in
*** Makefile.in	16 Sep 2009 03:10:17 -0000	1.193
--- Makefile.in	26 Sep 2009 19:16:12 -0000
***************
*** 131,143 ****
  
  # The chmod +w is to handle env var CVSREAD=1.  Files named
  # are identified by being the value of `generated-autoload-file'.
! autoloads: $(LOADDEFS) doit
  	chmod +w $(lisp)/ps-print.el $(lisp)/emulation/tpu-edt.el \
  	  $(lisp)/emacs-lisp/cl-loaddefs.el $(lisp)/mail/rmail.el \
  	  $(lisp)/dired.el $(lisp)/ibuffer.el
  	wd=$(lisp); $(setwins_almost); \
  	echo Directories: $$wins; \
! 	$(emacs) -l autoload --eval '(setq generated-autoload-file "$(lisp)/loaddefs.el")' -f batch-update-autoloads $$wins
  
  # This is required by the bootstrap-emacs target in ../src/Makefile, so
  # we know that if we have an emacs executable, we also have a subdirs.el.
--- 131,146 ----
  
  # The chmod +w is to handle env var CVSREAD=1.  Files named
  # are identified by being the value of `generated-autoload-file'.
! # The Makefile dependency is to make any missing-file error more explicit.
! autoloads: $(LOADDEFS) ../src/Makefile doit
  	chmod +w $(lisp)/ps-print.el $(lisp)/emulation/tpu-edt.el \
  	  $(lisp)/emacs-lisp/cl-loaddefs.el $(lisp)/mail/rmail.el \
  	  $(lisp)/dired.el $(lisp)/ibuffer.el
  	wd=$(lisp); $(setwins_almost); \
  	echo Directories: $$wins; \
! 	preload=`sed -n -e '/^lisp=/ s/$${lispsource}//g p' ../src/Makefile | \
! 	  sed 's/^lisp= //'`; \
! 	$(emacs) -l autoload --eval "(setq generated-autoload-file \"$(lisp)/loaddefs.el\" autoload-excludes \"$${preload}\")" -f batch-update-autoloads $$wins
  
  # This is required by the bootstrap-emacs target in ../src/Makefile, so
  # we know that if we have an emacs executable, we also have a subdirs.el.
Index: autoload.el
===================================================================
RCS file: /sources/emacs/emacs/lisp/emacs-lisp/autoload.el,v
retrieving revision 1.143
diff -c -c -w -r1.143 autoload.el
*** autoload.el	5 Jan 2009 03:20:44 -0000	1.143
--- autoload.el	26 Sep 2009 19:16:22 -0000
***************
*** 58,63 ****
--- 58,66 ----
  read and an autoload made for it.  If there is further text on the line,
  that text will be copied verbatim to `generated-autoload-file'.")
  
+ (defvar autoload-excludes nil
+   "If non-nil, list of absolute file names not to scan for autoloads.")
+ 
  (defconst generate-autoload-section-header "\f\n;;;### "
    "String that marks the form at the start of a new file's autoload section.")
  
***************
*** 347,353 ****
            relfile
            ;; nil until we found a cookie.
            output-start)
! 
        (with-current-buffer (or visited
                                 ;; It is faster to avoid visiting the file.
                                 (autoload-find-file file))
--- 350,357 ----
            relfile
            ;; nil until we found a cookie.
            output-start)
!       (if (member absfile autoload-excludes)
!       	  (message "Generating autoloads for %s...skipped" file)
        (with-current-buffer (or visited
                                 ;; It is faster to avoid visiting the file.
                                 (autoload-find-file file))
***************
*** 452,458 ****
            (message "Generating autoloads for %s...done" file))
          (or visited
              ;; We created this buffer, so we should kill it.
!             (kill-buffer (current-buffer))))
        ;; If the entries were added to some other buffer, then the file
        ;; doesn't add entries to OUTFILE.
        (or (not output-start) otherbuf))))
--- 456,462 ----
            (message "Generating autoloads for %s...done" file))
          (or visited
              ;; We created this buffer, so we should kill it.
!             (kill-buffer (current-buffer)))))
        ;; If the entries were added to some other buffer, then the file
        ;; doesn't add entries to OUTFILE.
        (or (not output-start) otherbuf))))
***************
*** 649,654 ****
--- 653,670 ----
  (defun batch-update-autoloads ()
    "Update loaddefs.el autoloads in batch mode.
  Calls `update-directory-autoloads' on the command line arguments."
+   ;; For use during the Emacs build process only.  We do the file-name
+   ;; expansion here rather than in lisp/Makefile in order to keep the
+   ;; shell command line short.  (Long lines are an issue on some systems.)
+   (if (stringp autoload-excludes)
+       (setq autoload-excludes
+ 	    (mapcar
+ 	     (lambda (file)
+ 	       (concat
+ 		(expand-file-name (file-name-sans-extension file)
+ 				  (file-name-directory generated-autoload-file))
+ 		".el"))
+ 	     (split-string autoload-excludes))))
    (let ((args command-line-args-left))
      (setq command-line-args-left nil)
      (apply 'update-directory-autoloads args)))

bug#4446: remove ###autoload from ediff-hook.el

[Stefan Monnier]

> I found that setting preloaded-file-list in bootstrap-emacs does not
> work anyway. At that stage files are being loaded uncompiled, so extra
> files (eg cl) end up being added to preloaded-file-list due to
> eval-when-compile.

> Anyway, here's an attempt at your suggestion. The final version
> probably should not print the "skipped" message.

That looks pretty good.  There's still a "minor" problem that
lisp/makefile.w32 also needs update.

BTW, now that I think about it: is it really necessary to skip those
files?  I mean, what is the impact of having such redundant autoloads in
loaddefs.el?


        Stefan

bug#4446: remove ###autoload from ediff-hook.el

[Dan Nicolaescu]

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

  > > I found that setting preloaded-file-list in bootstrap-emacs does not
  > > work anyway. At that stage files are being loaded uncompiled, so extra
  > > files (eg cl) end up being added to preloaded-file-list due to
  > > eval-when-compile.
  > 
  > > Anyway, here's an attempt at your suggestion. The final version
  > > probably should not print the "skipped" message.
  > 
  > That looks pretty good.  There's still a "minor" problem that
  > lisp/makefile.w32 also needs update.
  > 
  > BTW, now that I think about it: is it really necessary to skip those
  > files?  I mean, what is the impact of having such redundant autoloads in
  > loaddefs.el?

From memory as I can't verify at the moment: the download size of the
emacs source is increased by > 10K, and the binary is 10KB bigger.

All of that is due to the single autoload line in ediff-hook.el, so the
can be solved by just removing that single line, or by killing the
ediff-hook.el file and moving the emacs specific code in menu-bar.el
(where it logically belongs anyway).

bug#4446: remove ###autoload from ediff-hook.el

[Stefan Monnier]

>> BTW, now that I think about it: is it really necessary to skip those
>> files?  I mean, what is the impact of having such redundant autoloads in
>> loaddefs.el?

>> From memory as I can't verify at the moment: the download size of the
> emacs source is increased by > 10K, and the binary is 10KB bigger.

I understand where the 10K of download-size comew from, but do you know
why it also impacts the final binary size?  I mean: the objects and
variables created by these redundant autoloads should be overwritten by
the subsequent load of the actual files, so the only impact they should
have is if the GC ends up unable to reuse the corresponding memory cells
(and unable to return them to the system), right?  Or are these
chunks of autoload-data placed in the pure-space?


        Stefan

bug#4446: remove ###autoload from ediff-hook.el

[Dan Nicolaescu]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

  > >> BTW, now that I think about it: is it really necessary to skip those
  > >> files?  I mean, what is the impact of having such redundant autoloads in
  > >> loaddefs.el?
  > 
  > >> From memory as I can't verify at the moment: the download size of the
  > > emacs source is increased by > 10K, and the binary is 10KB bigger.
  > 
  > I understand where the 10K of download-size comew from, but do you know
  > why it also impacts the final binary size?  I mean: the objects and
  > variables created by these redundant autoloads should be overwritten by
  > the subsequent load of the actual files, so the only impact they should
  > have is if the GC ends up unable to reuse the corresponding memory cells
  > (and unable to return them to the system), right?  Or are these
  > chunks of autoload-data placed in the pure-space?

Sorry, I don't know.

Another thing that I observed (and hope not to forget until I can
investigate properly): a lot of free conses and strings appear after the
"finding pointers to doc strings" phase in loadup.el.  

It seems that we might have to try to avoid allocating and setting the
doc strings while dumping.

bug#4446: marked as done (remove ###autoload from ediff-hook.el)

[Emacs bug Tracking System]

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

Your message dated Mon, 05 Oct 2009 23:14:05 -0400
with message-id <w7iqetxlhe.fsf@fencepost.gnu.org>
and subject line Re: bug#4446: remove ###autoload from ediff-hook.el
has caused the Emacs bug report #4446,
regarding remove ###autoload from ediff-hook.el
to be marked as done.

This means that you claim that the problem has been dealt with.
If this is not the case it is now your responsibility to reopen the
bug report if necessary, and/or fix the problem forthwith.

(NB: If you are a system administrator and have no idea what this
message is talking about, this may indicate a serious mail system
misconfiguration somewhere. Please contact owner@emacsbugs.donarmstrong.com
immediately.)


-- 
4446: http://emacsbugs.donarmstrong.com/cgi-bin/bugreport.cgi?bug=4446
Emacs Bug Tracking System
Contact owner@emacsbugs.donarmstrong.com with problems

[-- Attachment #2: Type: message/rfc822, Size: 3183 bytes --]

From: Dan Nicolaescu <dann@ics.uci.edu>
To: bug-gnu-emacs <bug-gnu-emacs@gnu.org>
Subject: remove ###autoload from ediff-hook.el
Date: Wed, 16 Sep 2009 04:18:25 -0700 (PDT)
Message-ID: <200909161118.n8GBIPDV012011@godzilla.ics.uci.edu>


The ###autoload in ediff-hook.el is not needed, this file is in the
dumped image, so the autoload's only effect is to make loaddefs.el
bigger and increase the size of the dumped image.

The comment in the file says that the autoload is needed for XEmacs, but
that's not a good reason to keep it in the Emacs CVS, it's a one line
change to a file that almost never changes.

OK to remove the autoload line?



[-- Attachment #3: Type: message/rfc822, Size: 2359 bytes --]

From: Glenn Morris <rgm@gnu.org>
To: 4446-done@emacsbugs.donarmstrong.com
Subject: Re: bug#4446: remove ###autoload from ediff-hook.el
Date: Mon, 05 Oct 2009 23:14:05 -0400
Message-ID: <w7iqetxlhe.fsf@fencepost.gnu.org>


  * emacs-lisp/autoload.el (autoload-excludes): New variable.
  (autoload-generate-file-autoloads): Skip files in autoload-excludes.
  (batch-update-autoloads): Process a string value of autoload-excludes,
  set during the build process.
  * Makefile.in (autoloads): Skip preloaded files.  (Bug#4446)