Re: space leak from `values'

Re: space leak from `values'

[Kim F. Storm]

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

> > I bet most people don't know that `values' even exists, let alone what
> > conses onto it!
> 
> Completely agreed.

I know it -- I just fail to see why it is useful.

> 
> > It seems basically unclean to let something like that grow
> > unboundedly, even if it doesn't keep a significant part of the heap
> > live, and especially as it's pretty obscure.
> 
> I'd vote for removing it, but IIRC some people argued for keeping it last
> time this came up.  So I suggest we replace the lisp with a ring.


Here is a patch which reduces the values list to max 10 elements (not
configurable -- should it be?)

The major part of the patch is the addition of a new C-level function
`setnthcdr' which is used by other C and lisp level code to truncate
the `values' list.

This function seems generally useful so I think we should add it 
in any case...

> Still, it should have a different name to avoid surprises (like
> `values-of-last-interactive-evaluations').

Or `last-eval-results'.  However, I didn't change that.


Index: src/fns.c
===================================================================
RCS file: /cvsroot/emacs/emacs/src/fns.c,v
retrieving revision 1.372
diff -c -r1.372 fns.c
*** src/fns.c	6 Jul 2004 19:36:56 -0000	1.372
--- src/fns.c	28 Jul 2004 23:02:02 -0000
***************
*** 1415,1420 ****
--- 1415,1447 ----
    return Fcar (Fnthcdr (n, list));
  }
  
+ DEFUN ("setnthcdr", Fsetnthcdr, Ssetnthcdr, 2, 3, 0,
+        doc: /* Set cdr of Nth element of LIST to VALUE (nil if omitted), returns the result.
+ If N is negative, count from end of LIST.
+ If list has less than N elements, do not modify list.  */)
+   (n, list, value)
+      Lisp_Object n, list, value;
+ {
+   register Lisp_Object elt;
+   register int num;
+   CHECK_NUMBER (n);
+   num = XINT (n);
+   if (num < 0)
+     num += XFASTINT (Flength (list));
+   if (num <= 0)
+     return Qnil;
+   for (elt = list; --num > 0 && !NILP (elt);)
+     {
+       QUIT;
+       if (! CONSP (elt))
+ 	wrong_type_argument (Qlistp, elt);
+       elt = XCDR (elt);
+     }
+   if (CONSP (elt))
+     XSETCDR (elt, value);
+   return list;
+ }
+ 
  DEFUN ("elt", Felt, Selt, 2, 2, 0,
         doc: /* Return element of SEQUENCE at index N.  */)
       (sequence, n)
***************
*** 5715,5720 ****
--- 5742,5748 ----
    defsubr (&Ssubstring_no_properties);
    defsubr (&Snthcdr);
    defsubr (&Snth);
+   defsubr (&Ssetnthcdr);
    defsubr (&Selt);
    defsubr (&Smember);
    defsubr (&Smemq);
Index: src/lisp.h
===================================================================
RCS file: /cvsroot/emacs/emacs/src/lisp.h,v
retrieving revision 1.501
diff -c -r1.501 lisp.h
*** src/lisp.h	30 Jun 2004 13:09:05 -0000	1.501
--- src/lisp.h	28 Jul 2004 23:02:04 -0000
***************
*** 2271,2276 ****
--- 2271,2277 ----
  extern Lisp_Object substring_both P_ ((Lisp_Object, int, int, int, int));
  EXFUN (Fnth, 2);
  EXFUN (Fnthcdr, 2);
+ EXFUN (Fsetnthcdr, 3);
  EXFUN (Fmemq, 2);
  EXFUN (Fassq, 2);
  EXFUN (Fassoc, 2);
Index: src/lread.c
===================================================================
RCS file: /cvsroot/emacs/emacs/src/lread.c,v
retrieving revision 1.322
diff -c -r1.322 lread.c
*** src/lread.c	26 Apr 2004 21:28:40 -0000	1.322
--- src/lread.c	28 Jul 2004 23:02:03 -0000
***************
*** 1377,1383 ****
  
        if (printflag)
  	{
! 	  Vvalues = Fcons (val, Vvalues);
  	  if (EQ (Vstandard_output, Qt))
  	    Fprin1 (val, Qnil);
  	  else
--- 1377,1384 ----
  
        if (printflag)
  	{
! 	  Vvalues = Fsetnthcdr (make_number (10),
! 				Fcons (val, Vvalues), Qnil);
  	  if (EQ (Vstandard_output, Qt))
  	    Fprin1 (val, Qnil);
  	  else
Index: lisp/simple.el
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/simple.el,v
retrieving revision 1.651
diff -c -r1.651 simple.el
*** lisp/simple.el	23 Jul 2004 11:52:03 -0000	1.651
--- lisp/simple.el	28 Jul 2004 23:02:05 -0000
***************
*** 825,836 ****
  	 current-prefix-arg))
  
    (if (null eval-expression-debug-on-error)
!       (setq values (cons (eval eval-expression-arg) values))
      (let ((old-value (make-symbol "t")) new-value)
        ;; Bind debug-on-error to something unique so that we can
        ;; detect when evaled code changes it.
        (let ((debug-on-error old-value))
! 	(setq values (cons (eval eval-expression-arg) values))
  	(setq new-value debug-on-error))
        ;; If evaled code has changed the value of debug-on-error,
        ;; propagate that change to the global binding.
--- 825,836 ----
  	 current-prefix-arg))
  
    (if (null eval-expression-debug-on-error)
!       (setq values (setnthcdr 10 (cons (eval eval-expression-arg) values)))
      (let ((old-value (make-symbol "t")) new-value)
        ;; Bind debug-on-error to something unique so that we can
        ;; detect when evaled code changes it.
        (let ((debug-on-error old-value))
! 	(setq values (setnthcdr 10 (cons (eval eval-expression-arg) values)))
  	(setq new-value debug-on-error))
        ;; If evaled code has changed the value of debug-on-error,
        ;; propagate that change to the global binding.
Index: lisp/emacs-lisp/edebug.el
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/emacs-lisp/edebug.el,v
retrieving revision 3.69
diff -c -r3.69 edebug.el
*** lisp/emacs-lisp/edebug.el	10 Jun 2004 04:18:04 -0000	3.69
--- lisp/emacs-lisp/edebug.el	28 Jul 2004 23:02:07 -0000
***************
*** 3716,3722 ****
  		      'read-expression-history)))
    (princ
     (edebug-outside-excursion
!     (setq values (cons (edebug-eval edebug-expr) values))
      (concat (edebug-safe-prin1-to-string (car values))
              (eval-expression-print-format (car values))))))
  
--- 3716,3722 ----
  		      'read-expression-history)))
    (princ
     (edebug-outside-excursion
!     (setq values (setnthcdr 10 (cons (edebug-eval edebug-expr) values)))
      (concat (edebug-safe-prin1-to-string (car values))
              (eval-expression-print-format (car values))))))
  
Index: lisp/emacs-lisp/pp.el
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/emacs-lisp/pp.el,v
retrieving revision 1.23
diff -c -r1.23 pp.el
*** lisp/emacs-lisp/pp.el	28 Jun 2004 23:08:31 -0000	1.23
--- lisp/emacs-lisp/pp.el	28 Jul 2004 23:02:07 -0000
***************
*** 100,106 ****
  instead.  The value is also consed onto the front of the list
  in the variable `values'."
    (interactive "xPp-eval: ")
!   (setq values (cons (eval expression) values))
    (let* ((old-show-function temp-buffer-show-function)
  	 ;; Use this function to display the buffer.
  	 ;; This function either decides not to display it at all
--- 100,106 ----
  instead.  The value is also consed onto the front of the list
  in the variable `values'."
    (interactive "xPp-eval: ")
!   (setq values (setnthcdr 10 (cons (eval expression) values)))
    (let* ((old-show-function temp-buffer-show-function)
  	 ;; Use this function to display the buffer.
  	 ;; This function either decides not to display it at all


-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: space leak from `values'

[Miles Bader]

storm@cua.dk (Kim F. Storm) writes:
> + DEFUN ("setnthcdr", Fsetnthcdr, Ssetnthcdr, 2, 3, 0,
> +        doc: /* Set cdr of Nth element of LIST to VALUE (nil if omitted), returns the result.

What's wrong with (setcdr (nthcdr (1- N) LIST) VALUE) ?

-Miles
-- 
.Numeric stability is probably not all that important when you're guessing.

Re: space leak from `values'

[Kim F. Storm]

Miles Bader <miles@lsi.nec.co.jp> writes:

> storm@cua.dk (Kim F. Storm) writes:
> > + DEFUN ("setnthcdr", Fsetnthcdr, Ssetnthcdr, 2, 3, 0,
> > +        doc: /* Set cdr of Nth element of LIST to VALUE (nil if omitted), returns the result.
> 
> What's wrong with (setcdr (nthcdr (1- N) LIST) VALUE) ?

Read the third line of the doc string:

> If list has less than N elements, do not modify list.

Your suggestion fails if N is less than the length of the list:

(let ((list '(1 2 3 4))
      (n 10)     
      (value nil))
  (setcdr (nthcdr (1- n) list) value))

With (my version of) setnthcdr you replace
    (if (> (length kill-ring) kill-ring-max)
	(setcdr (nthcdr (1- kill-ring-max) kill-ring) nil)))
by
    (setnthcdr kill-ring-max kill-ring)

And besides, I need this at the C level (but I can of course
write the necessary code in-line).


BTW, my first idea was to call this function `chop' so that (chop N
LIST) would simply truncate LIST to a maximum of N elements -- but
then I realized that setnthcdr with the optional VALUE arg would be
more generic (and potentially more useful).

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: space leak from `values'

[David Kastrup]

storm@cua.dk (Kim F. Storm) writes:

> Miles Bader <miles@lsi.nec.co.jp> writes:
> 
> > storm@cua.dk (Kim F. Storm) writes:
> > > + DEFUN ("setnthcdr", Fsetnthcdr, Ssetnthcdr, 2, 3, 0,
> > > +        doc: /* Set cdr of Nth element of LIST to VALUE (nil if omitted), returns the result.
> > 
> > What's wrong with (setcdr (nthcdr (1- N) LIST) VALUE) ?
> 
> Read the third line of the doc string:
> 
> > If list has less than N elements, do not modify list.
> 
> Your suggestion fails if N is less than the length of the list:
> 
> (let ((list '(1 2 3 4))
>       (n 10)     
>       (value nil))
>   (setcdr (nthcdr (1- n) list) value))
> 
> With (my version of) setnthcdr you replace
>     (if (> (length kill-ring) kill-ring-max)
> 	(setcdr (nthcdr (1- kill-ring-max) kill-ring) nil)))
> by
>     (setnthcdr kill-ring-max kill-ring)

It must also be noted that length requires a lot of prerequisites:
that the list is not circular, ends with a cons, and so on.  For
example, (length '(3 4 5 . 6)) throws an exception.

One could write this as

(setcdr (or (nthcdr (1- n) list) (cons nil nil)) value)

or as

(let ((tail (nthcdr (1- n) list)))
  (and (consp tail)
       (setcdr tail value)))

or something to avoid the length call, however.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: space leak from `values'

[Miles Bader]

storm@cua.dk (Kim F. Storm) writes:
>> What's wrong with (setcdr (nthcdr (1- N) LIST) VALUE) ?
>
> Read the third line of the doc string:
>
>> If list has less than N elements, do not modify list.
>
> Your suggestion fails if N is less than the length of the list:

It's trivial to handle though that -- just see if the result of `nthcdr'
is nil, and only do the `setcdr' if not.  Adding a C function for this
rather rare operation just seems kinda like pointless bloat...

-Miles
-- 
Run away!  Run away!

Re: space leak from `values'

[Kim F. Storm]

Miles Bader <miles@lsi.nec.co.jp> writes:

> storm@cua.dk (Kim F. Storm) writes:
> >> What's wrong with (setcdr (nthcdr (1- N) LIST) VALUE) ?
> >
> > Read the third line of the doc string:
> >
> >> If list has less than N elements, do not modify list.
> >
> > Your suggestion fails if N is less than the length of the list:
> 
> It's trivial to handle though that -- just see if the result of `nthcdr'
> is nil, and only do the `setcdr' if not.  Adding a C function for this
> rather rare operation just seems kinda like pointless bloat...

Rare ?  Pointless ?

Truncating a list to a specific maximum is quite common if you ask me.

ido.el:    (if (> (length ido-work-directory-list) ido-max-work-directory-list)
ido.el:    (if (> (length ido-work-file-list) ido-max-work-file-list)
ido.el:	  (if (> (length ido-dir-file-cache) ido-max-dir-file-cache)
isearch.el:	    (if (> (length regexp-search-ring) regexp-search-ring-max)
isearch.el:	  (if (> (length search-ring) search-ring-max)
menu-bar.el:  (if (> (length (cdr yank-menu)) kill-ring-max)
menu-bar.el:	     (if (> (length buffers) buffers-menu-max-size)
simple.el:    (if (> (length kill-ring) kill-ring-max)
calc/calc-aent.el:	(when (> (length kill-ring) kill-ring-max)
calendar/calendar.el:          (if (> (length calendar-mark-ring) (1+ mark-ring-max))
emulation/cua-rect.el:    (if (> (length cua--undo-list) cua-undo-max)
emulation/viper-cmd.el:  (if (> (length kill-ring) kill-ring-max)
net/eudc.el:	     (> (length servers) eudc-max-servers-to-query))
progmodes/ada-xref.el:  (if (> (length ada-xref-pos-ring) ada-xref-pos-ring-max)
progmodes/xscheme.el:  (if (> (length xscheme-expressions-ring) xscheme-expressions-ring-max)
textmodes/bibtex.el:      (if (> (length bibtex-field-kill-ring) bibtex-field-kill-ring-max)
textmodes/bibtex.el:      (if (> (length bibtex-entry-kill-ring) bibtex-entry-kill-ring-max)

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: space leak from `values'

[Richard Stallman]

I did not realize there were so many places where this was done.
However, (setcdr (or (nthcdr N LIST) (list nil)) nil) is pretty
simple.  I am not sure we need a new function to replace that
even if that is done 20 times in Emacs.

Re: space leak from `values'

[Kim F. Storm]

Referring to my proposal for adding setnthcdr, and the number of
places where a list is currently truncated in the lisp files,

Richard Stallman <rms@gnu.org> writes:

> I did not realize there were so many places where this was done.
> However, (setcdr (or (nthcdr N LIST) (list nil)) nil) is pretty
> simple.  I am not sure we need a new function to replace that
> even if that is done 20 times in Emacs.

Since the only practical use of the "setnthcdr" function is to simply
truncate a list to a specific length, I suggest a new function
truncate-list (to be defined in subr.el):

(defun truncate-list (list n)
  "Truncate list LIST to have a maximum of N elements.
If LIST has less than N element, do nothing."
   (if (setq list (nthcdr (1- n) list))
       (setcdr list nil)))

Using that function is both clearer and more efficient than
how it's done currently.

e.g. instead of

    (setq kill-ring (cons string kill-ring))
    (if (> (length kill-ring) kill-ring-max)
	(setcdr (nthcdr (1- kill-ring-max) kill-ring) nil)))

just write

    (truncate-list (setq kill-ring (cons string kill-ring))
                   kill-ring-max)


Also, it is much simpler to document truncate-list than the more
generic setnthcdr.

--
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: space leak from `values'

[Richard Stallman]

I just don't see enough need for truncate-list
to justify the new feature.  The code used now is simple
enough.

Re: space leak from `values'

[Richard Stallman]

    And besides, I need this at the C level (but I can of course
    write the necessary code in-line).

Unless we see broader need for this, please do write it in line.
That way we save the hassle of documenting it in the manual, etc.