the ...-unload-hook convention doesn't work

the ...-unload-hook convention doesn't work

[Luc Teirlinck]

Dave Love wrote:

    I'm confused by this, since I'm sure it worked when I implemented it,
    but the installed code never seems to have been right since it's
    calling a function as if it were a hook list.

It now is supposed to be a normal hook.  From (elisp)Unloading:

     If `FEATURE-unload-hook' is defined, it is run as a normal hook
     before restoring the previous definitions, _instead of_ the usual
     hook-removing actions.

But how could this have broken things?  `run-hooks' is supposed to be
able to handle a single function instead of a list, so there should be
no problems with compatibility in as far as line 143 is concerned.
Are there concrete examples of problems arising from either defining
FEATURE-unload-hook as a function (even though, according to the Elisp
manual, this is now considered obsolete) or defining it as a normal
hook?

Sincerely,

Luc.

Re: the ...-unload-hook convention doesn't work

[Dave Love]

Luc Teirlinck <teirllm@dms.auburn.edu> writes:

> It now is supposed to be a normal hook.  From (elisp)Unloading:
>
>      If `FEATURE-unload-hook' is defined, it is run as a normal hook
>      before restoring the previous definitions, _instead of_ the usual
>      hook-removing actions.

I can't check the history now, but if it's been changed it hasn't been
done properly.  All the examples in Emacs are simple defuns.

(I suspect various files which need the hook are missing it.  I've
previously updated them when I've noticed, but apparently not tested
the results properly.)

Granted, the naming `-hook' when it isn't a normal hook was wrong, but
the idea was to only need to define a function, not a function plus a
variable.  I think such usage should be supported in addition to a
normal hook.

> But how could this have broken things?  `run-hooks' is supposed to be
> able to handle a single function instead of a list, so there should be
> no problems with compatibility in as far as line 143 is concerned.

I think you misunderstand.

(progn 
  (defun foo-func ()
    (setq foo 'run))
  (let ((foo 'not-run))
    (run-hooks 'foo-func)
    foo))
  => not-run

> Are there concrete examples of problems arising from either defining
> FEATURE-unload-hook as a function (even though, according to the Elisp
> manual, this is now considered obsolete) or defining it as a normal
> hook?

Yes -- as I said, the mechanism doesn't work.  You can end up with an
unusable Emacs if you unload some things.

Re: the ...-unload-hook convention doesn't work

[Richard Stallman]

    I can't check the history now, but if it's been changed it hasn't been
    done properly.  All the examples in Emacs are simple defuns.

It would not be consistent to call a symbol whose names is ...-hook
as a function.  So I think we should fix those files to use the
feature properly.

The feature is used properly in eshell.

Re: the ...-unload-hook convention doesn't work

[Dave Love]

Richard Stallman <rms@gnu.org> writes:

>     I can't check the history now, but if it's been changed it hasn't been
>     done properly.  All the examples in Emacs are simple defuns.
>
> It would not be consistent to call a symbol whose names is ...-hook
> as a function.

Yes, as I said, but I believe you approved it.  There are (or were)
many such inconsistencies.

> So I think we should fix those files to use the
> feature properly.
>
> The feature is used properly in eshell.

I disagree.  For some reason, the tree I used find in is missing
lisp/eshell, but in a 21.3 tree, at least, the feature clearly isn't
being used properly in eshell for other reasons.

The -unload-hook variables shouldn't be customized; they shouldn't be
nil if they're relevant; at least some are incorrectly named (and
documented to relate to features that don't exist).  While unloading
will work for the most part, it's because of unload-feature's
heuristics, not because the hook functions are effective.
E.g. unloading `em-ls' will not execute a hook to revert the fset
`insert-directory'.  (And incidentally, the `fset' for that should
surely be `defalias'.)

Re: the ...-unload-hook convention doesn't work

[Luc Teirlinck]

I can not but wonder what we exactly want to achieve with `unload-feature'.
Is there any hope of ever making it safe enough to be called in an
actual editing session in which non-trivial work is invested and will
_continue_ to be invested after unloading a feature?  I do not believe
that it currently comes even remotely close to that standard.

The worst thing is that even reloading the feature often can not undo
the damage:

ELISP> (fboundp 'macroexpand)
t
ELISP> (require 'cl)
cl
ELISP> (fboundp 'macroexpand)
t
ELISP> (unload-feature 'cl) ;; Long output flushed
ELISP> (require 'cl)
cl
ELISP> (fboundp 'macroexpand)
nil
ELISP> 

Sincerely,

Luc.

Re: the ...-unload-hook convention doesn't work

[Richard Stallman]

It makes no sense to think about unload-feature in terms
of the cl package.  That is a very hard case.  Other cases
will work before that one does.

However, the motive for unload-feature had to do with
reclaiming memory.  With the other changes in size of machines,
size of disks, and size of Emacs address space, maybe that is
pointless.  Maybe we should eliminate unload-feature just
to make things simpler and avoid spending time on it.

Re: the ...-unload-hook convention doesn't work

[Kevin Rodgers]

Richard Stallman wrote:

> However, the motive for unload-feature had to do with
> reclaiming memory.  With the other changes in size of machines,
> size of disks, and size of Emacs address space, maybe that is
> pointless.  Maybe we should eliminate unload-feature just
> to make things simpler and avoid spending time on it.

unload-feature is also good for cleaning up the global namespace, so I
hope it continues to be supported.  If *-unload-hook is the problem,
perhaps just that should be deprecated.


-- 
Kevin Rodgers

Re: the ...-unload-hook convention doesn't work

[Dave Love]

Richard Stallman <rms@gnu.org> writes:

> It makes no sense to think about unload-feature in terms
> of the cl package.

I don't think that's actually true; it's just not worth the effort,
and isn't an issue when you follow the Emacs conventions.  (I did toy
with the idea of making it work.)

> However, the motive for unload-feature had to do with
> reclaiming memory.

It's also rather useful during development.

> With the other changes in size of machines,
> size of disks, and size of Emacs address space, maybe that is
> pointless.

Emacs memory requirements, coupled with its poor GC, are certainly
significant in the situations I typically use it.  That was even a
motivation for me working on the GC.  Since no one else appears to be
interested, I assume this doesn't apply to developers, but perhaps
they don't have a sufficiently wide perspective.  (I'm currently in 32
MB.)

As it happens, I do occasionally unload a Gnus feature which leaks.
That typically reduces the number of used conses from ~1M to ~300k and
means GC then doesn't typically thrash on my work system though it
leaves a lot of unused ones.

-- 
The ultimate computing slogan is "Your Mileage May Vary".
  -- Richard O' Keefe

Re: the ...-unload-hook convention doesn't work

[Simon Josefsson]

Dave Love <d.love@dl.ac.uk> writes:

> As it happens, I do occasionally unload a Gnus feature which leaks.
> That typically reduces the number of used conses from ~1M to ~300k and
> means GC then doesn't typically thrash on my work system though it
> leaves a lot of unused ones.

Which feature is this?  Any hints on finding leaking code, in general?
I have been using the following to print out large symbols:

(mapatoms (lambda (sym)
	    (let ((len (ignore-errors (length (symbol-value sym)))))
	      (if (and len (> len 500) (not (= len 507904)))
		  (insert (format "%s: %s\n" sym len))))))

But it doesn't seem to work well.  I have been getting complaints
about exhausting the memory from Emacs without that code generating
anything interesting.  My machine has 1 GB RAM, and emacs is typically
around 100MB but can grow to 300MB or so when I do something major in
Gnus.

Essentially: how do I find out what is taking up all memory in Emacs?
Perhaps a package that report these things would be handy.

Re: the ...-unload-hook convention doesn't work

[Richard Stallman]

    Essentially: how do I find out what is taking up all memory in Emacs?
    Perhaps a package that report these things would be handy.

I wish I knew.  However, I am surprised your mapatoms runs out
of space.  It looks like all it will do is add a line
to the buffer for a fairly small subset of symbols.

It would be interesting to debug were most of the consing goes
inside that mapatoms.  Maybe running that code with a breakpoint
in malloc would show you where.

It might be due to undo entries.

Re: the ...-unload-hook convention doesn't work

[Simon Josefsson]

Richard Stallman <rms@gnu.org> writes:

>     Essentially: how do I find out what is taking up all memory in Emacs?
>     Perhaps a package that report these things would be handy.
>
> I wish I knew.  However, I am surprised your mapatoms runs out
> of space.  It looks like all it will do is add a line
> to the buffer for a fairly small subset of symbols.

Yes.  Sorry, I was unclear, the mapatoms didn't cause the memory
warning, I was trying to use it diagnose an earlier out of memory
condition, caused by (most likely) Gnus.

Re: the ...-unload-hook convention doesn't work

[Dave Love]

Simon Josefsson <jas@extundo.com> writes:

> Which feature is this?

gnus-nocem (though it's probably not be doing me much good anyway
without reading usenet).

> Any hints on finding leaking code, in general?

In that case I guess it fails to purge a hash table, which I couldn't
easily follow.

> I have been using the following to print out large symbols:
>
> (mapatoms (lambda (sym)
> 	    (let ((len (ignore-errors (length (symbol-value sym)))))
> 	      (if (and len (> len 500) (not (= len 507904)))
> 		  (insert (format "%s: %s\n" sym len))))))
>
> But it doesn't seem to work well.

You need dependency info tracing all slots from obarrays to start
with, and then you won't account for data kept live through C code, at
least (e.g. text properties).

> I have been getting complaints
> about exhausting the memory from Emacs without that code generating
> anything interesting.  My machine has 1 GB RAM, and emacs is typically
> around 100MB but can grow to 300MB or so when I do something major in
> Gnus.

The message is probably misleading in that case -- see all its uses in
alloc.c (?).

The limit on what a Lisp pointer can address is 128MB (or 256 -- I
can't remember) on a 32-bit system.  I added a comment to that effect
somewhere.  The actual data in strings, for instance, aren't
necessarily in the same address space, but as far as I remember,
probably contribute to that limit because they're malloced from the
same region of (end of) the machine's address space (whereas mmapped
space is at the other end, at least under Linux).  See (probably) the
same comment as above.

It's made worse because pages allocated in the big bag don't get freed
at least until they're completely emptied by GC -- see unused conses
&c in the GC statistics.

(A different sort of leak -- buffer space -- can occur on systems like
Solaris which don't use the relocating allocator, and/or the mmap
mechanism -- I forget.  I don't remember whether the buffer space is
in the Lisp pointer range in that case or not.  I thought rms vetoed
the solution, but it's still in the version of TODO I have).

> Essentially: how do I find out what is taking up all memory in Emacs?
> Perhaps a package that report these things would be handy.

You can't, without serious work on the internals.

Replacing the GC, and perhaps the object representation, per TODO
could/would address all these issues to a greater or lesser extent.  I
assume there's a policy against that now since the old one is being
tinkered with.  Implementing the TODO item on `long long' would remove
the addressing limitation at the expense of larger data, at least
using GCC.

Re: the ...-unload-hook convention doesn't work

[Dave Love]

Luc Teirlinck <teirllm@dms.auburn.edu> writes:

> I can not but wonder what we exactly want to achieve with `unload-feature'.
> Is there any hope of ever making it safe enough to be called in an
> actual editing session in which non-trivial work is invested and will
> _continue_ to be invested after unloading a feature?

Yes, with code that obeys the Emacs conventions in the Lisp manual.  I
frequently use it, which is why I worked on it initially.

> I do not believe
> that it currently comes even remotely close to that standard.
>
> The worst thing is that even reloading the feature often can not undo
> the damage:
>
> ELISP> (fboundp 'macroexpand)
> t
> ELISP> (require 'cl)
> cl
> ELISP> (fboundp 'macroexpand)
> t
> ELISP> (unload-feature 'cl) ;; Long output flushed
> ELISP> (require 'cl)
> cl
> ELISP> (fboundp 'macroexpand)
> nil
> ELISP> 

One of the conventions is to avoid CL at runtime.  Another one is not
to redefine Emacs functions as CL does.  The package probably could
actually make itself sanely unloadable by using the -unload-hook
feature if anyone cared to work on it.

In the event that a package has done something like this, it should
have a documented way to fiddle the feature list to control what gets
unbound.  I realized that when dealing with code that needs to
redefine several things which are currently problematic in Emacs and
would eventually have got round to suggesting the following.  (You can
currently do it knowing that `flist' is the right name until someone
changed that.)

--- loadhist.el.~1.26.~	Mon Oct  6 11:55:29 2003
+++ loadhist.el	Mon Jan 12 15:15:34 2004
@@ -120,7 +120,17 @@
 (defun unload-feature (feature &optional force)
   "Unload the library that provided FEATURE, restoring all its autoloads.
 If the feature is required by any other loaded code, and prefix arg FORCE
-is nil, raise an error."
+is nil, raise an error.
+
+This function tries to undo modifications made by the package to
+hooks.  Packages may define a hook FEATURE-unload-hook that is called
+instead of the normal heuristics for doing this.  Such a hook should
+undo all the relevant global state changes that may have been made by
+loading the package or executing functions in it.  It has access to
+the package's feature list (before anything is unbound) in the
+variable `feature-list' and could remove features from it in the event
+that the package has done something normally-ill-advised, such as
+redefining an Emacs function."
   (interactive (list (read-feature "Feature: ") current-prefix-arg))
   (if (not (featurep feature))
       (error "%s is not a currently loaded feature" (symbol-name feature)))
@@ -130,8 +140,8 @@
 	(if dependents
 	    (error "Loaded libraries %s depend on %s"
 		   (prin1-to-string dependents) file))))
-  (let* ((flist (feature-symbols feature))
-         (file (car flist))
+  (let* ((feature-list (feature-symbols feature))
+         (file (car feature-list))
          (unload-hook (intern-soft (concat (symbol-name feature)
                                            "-unload-hook"))))
     ;; Try to avoid losing badly when hooks installed in critical
@@ -155,10 +165,10 @@
                       (string-match "-hooks?\\'" (symbol-name x)))
                  (and (boundp x)       ; Known abnormal hooks etc.
                       (memq x unload-feature-special-hooks)))
-	     (dolist (y (cdr flist))
+	     (dolist (y (cdr feature-list))
 	       (remove-hook x y))))))
     (if (fboundp 'elp-restore-function)	; remove ELP stuff first
-	(dolist (elt (cdr flist))
+	(dolist (elt (cdr feature-list))
 	  (if (symbolp elt)
 	      (elp-restore-function elt))))
     (mapc
@@ -183,7 +193,7 @@
 		(fmakunbound x)
 		(let ((aload (get x 'autoload)))
 		  (if aload (fset x (cons 'autoload aload))))))))
-     (cdr flist))
+     (cdr feature-list))
     ;; Delete the load-history element for this file.
     (let ((elt (assoc file load-history)))
       (setq load-history (delq elt load-history)))))

Re: the ...-unload-hook convention doesn't work

[Richard Stallman]

    > It would not be consistent to call a symbol whose names is ...-hook
    > as a function.

    Yes, as I said, but I believe you approved it.

I believe I approved running that symbol as a normal hook.
That is how it should work.  The incorrect usage of defining
this symbol as a function never worked.  We should just fix the
incorrect usage.

Re: the ...-unload-hook convention doesn't work

[Dave Love]

Richard Stallman <rms@gnu.org> writes:

> I believe I approved running that symbol as a normal hook.
> That is how it should work.

As I said, I don't disagree on the principle now.

> The incorrect usage of defining
> this symbol as a function never worked.

That's how I said it seemed, but I was told things had changed.

Re: the ...-unload-hook convention doesn't work

[Richard Stallman]

Would you like to change the files that incorrectly define the symbol
as a function, to define it as a normal hook instead?

the ...-unload-hook convention doesn't work

[Dave Love]

I'm confused by this, since I'm sure it worked when I implemented it,
but the installed code never seems to have been right since it's
calling a function as if it were a hook list.

*** loadhist.el.~1.26.~	Mon Oct  6 11:55:29 2003
--- loadhist.el	Wed Dec  3 20:23:43 2003
***************
*** 140,146 ****
      ;; First off, provide a clean way for package `foo' to arrange
      ;; this by defining `foo-unload-hook'.
      (if unload-hook
!         (run-hooks unload-hook)
        ;; Otherwise, do our best.  Look through the obarray for symbols
        ;; which seem to be hook variables or special hook functions and
        ;; remove anything from them which matches the feature-symbols
--- 140,146 ----
      ;; First off, provide a clean way for package `foo' to arrange
      ;; this by defining `foo-unload-hook'.
      (if unload-hook
!         (funcall unload-hook)
        ;; Otherwise, do our best.  Look through the obarray for symbols
        ;; which seem to be hook variables or special hook functions and
        ;; remove anything from them which matches the feature-symbols