Re: [Emacs-diffs] /srv/bzr/emacs/trunk r109157: Compact buffers when idle.

Re: [Emacs-diffs] /srv/bzr/emacs/trunk r109157: Compact buffers when idle.

[Stefan Monnier]

As mentioned in my previous email, I think that the `compact' field of
struct buffer already provides most of the benefits and we don't need
the Elisp/idle part of this patch.


        Stefan


>>>>> "Dmitry" == Dmitry Antipov <dmantipov@yandex.ru> writes:

> ------------------------------------------------------------
> revno: 109157
> committer: Dmitry Antipov <dmantipov@yandex.ru>
> branch nick: trunk
> timestamp: Thu 2012-07-19 12:56:53 +0400
> message:
>   Compact buffers when idle.
>   * lisp/compact.el: New file.
>   * src/buffer.c (compact_buffer, Fcompact_buffer): New function.
>   (syms_of_buffer): Register Fcompact_buffer.
>   * src/alloc.c (Fgarbage_collect): Use compact_buffer.
>   * src/buffer.h (compact_buffer): New prototype.
>   (struct buffer_text): New member.
> added:
>   lisp/compact.el
> modified:
>   lisp/ChangeLog
>   src/ChangeLog
>   src/alloc.c
>   src/buffer.c
>   src/buffer.h

> === modified file 'lisp/ChangeLog'
> --- a/lisp/ChangeLog	2012-07-19 06:24:04 +0000
> +++ b/lisp/ChangeLog	2012-07-19 08:56:53 +0000
> @@ -1,3 +1,8 @@
> +2012-07-19  Dmitry Antipov  <dmantipov@yandex.ru>
> +
> +	Compact buffers when idle.
> +	* compact.el: New file.
> +
>  2012-07-19  Stefan Monnier  <monnier@iro.umontreal.ca>
 
>  	* subr.el (eventp): Presume that if it looks vaguely like an event,

> === added file 'lisp/compact.el'
> --- a/lisp/compact.el	1970-01-01 00:00:00 +0000
> +++ b/lisp/compact.el	2012-07-19 08:56:53 +0000
> @@ -0,0 +1,60 @@
> +;;; compact.el --- compact buffers when idle
> +
> +;; Copyright (C) 2012  Free Software Foundation, Inc.
> +
> +;; Maintainer: FSF
> +;; Package: emacs
> +
> +;; This file is part of GNU Emacs.
> +
> +;; GNU Emacs is free software: you can redistribute it and/or modify
> +;; it under the terms of the GNU General Public License as published by
> +;; the Free Software Foundation, either version 3 of the License, or
> +;; (at your option) any later version.
> +
> +;; GNU Emacs is distributed in the hope that it will be useful,
> +;; but WITHOUT ANY WARRANTY; without even the implied warranty of
> +;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +;; GNU General Public License for more details.
> +
> +;; You should have received a copy of the GNU General Public License
> +;; along with GNU Emacs.  If not, see <http://www.gnu.org/licenses/>.
> +
> +;;; Commentary:
> +
> +;; This package provides the ability to compact buffers when Emacs is idle.
> +;; Initially written by Dmitry Antipov <dmantipov@yandex.ru>.
> +
> +;;; Code:
> +
> +(require 'timer)
> +
> +(defun compact-buffers ()
> +  "Run `compact-buffer' for each buffer except current buffer.
> +Schedule next compaction if `compact-buffers-when-idle' is greater than zero."
> +  (mapc (lambda (buffer) 
> +	  (and (not (eq buffer (current-buffer)))
> +	       (compact-buffer buffer)))
> +	(buffer-list))
> +  (compact-buffers-idle))
> +
> +(defun compact-buffers-idle ()
> +  "Compact buffers if `compact-buffers-when-idle' is greater than zero."
> +  (and (floatp compact-buffers-when-idle)
> +       (> compact-buffers-when-idle 0.0)
> +       (run-with-idle-timer compact-buffers-when-idle nil 'compact-buffers)))
> +
> +(defcustom compact-buffers-when-idle 1.0
> +  "Compact all buffers when Emacs is idle more than this period of time.
> +Compaction is done by truncating `buffer-undo-list' and shrinking the gap.
> +Value less than or equal to zero disables idle compaction."
> +  :type 'float
> +  :group 'alloc
> +  :set (lambda (symbol value)
> +	 (progn (set-default symbol value)
> +		(compact-buffers-idle)))
> +  :version "24.2")
> +
> +(provide 'compact)
> +
> +;;; compact.el ends here

> === modified file 'src/ChangeLog'
> --- a/src/ChangeLog	2012-07-19 03:55:59 +0000
> +++ b/src/ChangeLog	2012-07-19 08:56:53 +0000
> @@ -1,5 +1,14 @@
>  2012-07-19  Dmitry Antipov  <dmantipov@yandex.ru>
 
> +	Buffer compaction primitive which may be used from Lisp.
> +	* buffer.c (compact_buffer, Fcompact_buffer): New function.
> +	(syms_of_buffer): Register Fcompact_buffer.
> +	* alloc.c (Fgarbage_collect): Use compact_buffer.
> +	* buffer.h (compact_buffer): New prototype.
> +	(struct buffer_text): New member.
> +
> +2012-07-19  Dmitry Antipov  <dmantipov@yandex.ru>
> +
>  	New macro to iterate over all buffers, miscellaneous cleanups.
>  	* lisp.h (all_buffers): Remove declaration.
>  	* buffer.h (all_buffers): Add declaration, with comment.

> === modified file 'src/alloc.c'
> --- a/src/alloc.c	2012-07-19 03:55:59 +0000
> +++ b/src/alloc.c	2012-07-19 08:56:53 +0000
> @@ -5413,33 +5413,7 @@
>    /* Don't keep undo information around forever.
>       Do this early on, so it is no problem if the user quits.  */
>    for_each_buffer (nextb)
> -    {
> -      /* If a buffer's undo list is Qt, that means that undo is
> -	 turned off in that buffer.  Calling truncate_undo_list on
> -	 Qt tends to return NULL, which effectively turns undo back on.
> -	 So don't call truncate_undo_list if undo_list is Qt.  */
> -      if (! NILP (nextb->BUFFER_INTERNAL_FIELD (name))
> -	  && ! EQ (nextb->BUFFER_INTERNAL_FIELD (undo_list), Qt))
> -	truncate_undo_list (nextb);
> -
> -      /* Shrink buffer gaps, but skip indirect and dead buffers.  */
> -      if (nextb->base_buffer == 0 && !NILP (nextb->BUFFER_INTERNAL_FIELD (name))
> -	  && ! nextb->text->inhibit_shrinking)
> -	{
> -	  /* If a buffer's gap size is more than 10% of the buffer
> -	     size, or larger than 2000 bytes, then shrink it
> -	     accordingly.  Keep a minimum size of 20 bytes.  */
> -	  int size = min (2000, max (20, (nextb->text->z_byte / 10)));
> -
> -	  if (nextb->text->gap_size > size)
> -	    {
> -	      struct buffer *save_current = current_buffer;
> -	      current_buffer = nextb;
> -	      make_gap (-(nextb->text->gap_size - size));
> -	      current_buffer = save_current;
> -	    }
> -	}
> -    }
> +    compact_buffer (nextb);
 
>    t1 = current_emacs_time ();
 

> === modified file 'src/buffer.c'
> --- a/src/buffer.c	2012-07-19 03:55:59 +0000
> +++ b/src/buffer.c	2012-07-19 08:56:53 +0000
> @@ -1434,14 +1434,59 @@
>    return Qnil;
>  }
 
> -/*
> -  DEFVAR_LISP ("kill-buffer-hook", ..., "\
> -Hook to be run (by `run-hooks', which see) when a buffer is killed.\n\
> -The buffer being killed will be current while the hook is running.\n\
> -
> -Functions run by this hook are supposed to not change the current
> -buffer.  See `kill-buffer'."
> -*/
> +/* Truncate undo list and shrink the gap of BUFFER.  */
> +
> +int
> +compact_buffer (struct buffer *buffer)
> +{
> +  /* Skip dead buffers, indirect buffers and buffers
> +     which aren't changed since last compaction.  */
> +  if (!NILP (buffer->BUFFER_INTERNAL_FIELD (name))
> +      && (buffer->base_buffer == NULL)
> +      && (buffer->text->compact != buffer->text->modiff))
> +    {
> +      /* If a buffer's undo list is Qt, that means that undo is
> +	 turned off in that buffer.  Calling truncate_undo_list on
> +	 Qt tends to return NULL, which effectively turns undo back on.
> +	 So don't call truncate_undo_list if undo_list is Qt.  */
> +      if (!EQ (buffer->BUFFER_INTERNAL_FIELD (undo_list), Qt))
> +	truncate_undo_list (buffer);
> +
> +      /* Shrink buffer gaps.  */
> +      if (!buffer->text->inhibit_shrinking)
> +	{
> +	  /* If a buffer's gap size is more than 10% of the buffer
> +	     size, or larger than 2000 bytes, then shrink it
> +	     accordingly.  Keep a minimum size of 20 bytes.  */
> +	  int size = min (2000, max (20, (buffer->text->z_byte / 10)));
> +
> +	  if (buffer->text->gap_size > size)
> +	    {
> +	      struct buffer *save_current = current_buffer;
> +	      current_buffer = buffer;
> +	      make_gap (-(buffer->text->gap_size - size));
> +	      current_buffer = save_current;
> +	    }
> +	}
> +      buffer->text->compact = buffer->text->modiff;
> +      return 1;
> +    }
> +  return 0;
> +}
> +
> +DEFUN ("compact-buffer", Fcompact_buffer, Scompact_buffer, 0, 1, 0,
> +       doc: /* Compact BUFFER by truncating undo list and shrinking the gap.
> +If buffer is nil, compact current buffer.  Compaction is performed
> +only if buffer was changed since last compaction.  Return t if
> +buffer compaction was performed, and nil otherwise.  */)
> +  (Lisp_Object buffer)
> +{
> +  if (NILP (buffer))
> +    XSETBUFFER (buffer, current_buffer);
> +  CHECK_BUFFER (buffer);
> +  return compact_buffer (XBUFFER (buffer)) ? Qt : Qnil;
> +}
> +
>  DEFUN ("kill-buffer", Fkill_buffer, Skill_buffer, 0, 1, "bKill buffer: ",
>         doc: /* Kill the buffer specified by BUFFER-OR-NAME.
>  The argument may be a buffer or the name of an existing buffer.
> @@ -5992,7 +6037,6 @@
>    defsubr (&Smake_indirect_buffer);
>    defsubr (&Sgenerate_new_buffer_name);
>    defsubr (&Sbuffer_name);
> -/*defsubr (&Sbuffer_number);*/
>    defsubr (&Sbuffer_file_name);
>    defsubr (&Sbuffer_base_buffer);
>    defsubr (&Sbuffer_local_value);
> @@ -6004,6 +6048,7 @@
>    defsubr (&Srename_buffer);
>    defsubr (&Sother_buffer);
>    defsubr (&Sbuffer_enable_undo);
> +  defsubr (&Scompact_buffer);
>    defsubr (&Skill_buffer);
>    defsubr (&Sbury_buffer_internal);
>    defsubr (&Sset_buffer_major_mode);

> === modified file 'src/buffer.h'
> --- a/src/buffer.h	2012-07-19 03:55:59 +0000
> +++ b/src/buffer.h	2012-07-19 08:56:53 +0000
> @@ -436,6 +436,9 @@
 
>      EMACS_INT overlay_modiff;	/* Counts modifications to overlays.  */
 
> +    EMACS_INT compact;		/* Set to modiff each time when compact_buffer
> +				   is called for this buffer.  */
> +
>      /* Minimum value of GPT - BEG since last redisplay that finished.  */
>      ptrdiff_t beg_unchanged;
 
> @@ -903,6 +906,7 @@
>  \f
>  extern void delete_all_overlays (struct buffer *);
>  extern void reset_buffer (struct buffer *);
> +extern int compact_buffer (struct buffer *);
>  extern void evaporate_overlays (ptrdiff_t);
>  extern ptrdiff_t overlays_at (EMACS_INT pos, int extend, Lisp_Object **vec_ptr,
>  			      ptrdiff_t *len_ptr, ptrdiff_t *next_ptr,


> _______________________________________________
> Emacs-diffs mailing list
> Emacs-diffs@gnu.org
> https://lists.gnu.org/mailman/listinfo/emacs-diffs

Re: [Emacs-diffs] /srv/bzr/emacs/trunk r109157: Compact buffers when idle.

[Dmitry Antipov]

On 07/19/2012 02:00 PM, Stefan Monnier wrote:

> As mentioned in my previous email, I think that the `compact' field of
> struct buffer already provides most of the benefits and we don't need
> the Elisp/idle part of this patch.

I hope that someone will find elisp part useful too :-). But it may be reasonable
to have `compact-buffers-when-idle' set to 0.0 by default, e.g. disabled.

Dmitry

Re: [Emacs-diffs] /srv/bzr/emacs/trunk r109157: Compact buffers when idle.

[Stefan Monnier]

>> As mentioned in my previous email, I think that the `compact' field of
>> struct buffer already provides most of the benefits and we don't need
>> the Elisp/idle part of this patch.
> I hope that someone will find elisp part useful too :-)

I can't think of any case where that would provide any noticeable
benefit (whereas I can think of cases where it just wastes resources).
The GC is run fairly regularly, also typically while idle, and does the
compaction.

So until you can provide some concrete and convincing numbers that show
some benefit, I'd ask you to remove that pat of your patch.


        Stefan

Re: [Emacs-diffs] /srv/bzr/emacs/trunk r109157: Compact buffers when idle.

[Dmitry Antipov]

On 07/19/2012 03:54 PM, Stefan Monnier wrote:

> I can't think of any case where that would provide any noticeable
> benefit (whereas I can think of cases where it just wastes resources).

Hm... I expect this to be something exotic, like replace-string
across hundreds of buffers. On the other side, it's quite
typical code refactoring operation. When I need to change function
name across the project, I'm using perl just because Emacs is too slow :-(.

> The GC is run fairly regularly, also typically while idle, and does the
> compaction.

1. Typically it happens when we're executing byte code, or from eval.c,
    when we want to be as fast as possible.

2. I don't understand why idle call to Fgarbage_collect is glued with
    auto-save-timeout. If I don't want to use auto-save, why I shouldn't
    call GC when idle?

> So until you can provide some concrete and convincing numbers that show
> some benefit, I'd ask you to remove that pat of your patch.

Hm... I suppose that 3GHz CPUs makes a lot of small optimizations almost
invisible; but I believe it doesn't mean that they're not needed anymore.
For example, I'm pretty sure that removing Blength bytecode (and so calling
Flength via Funcall) will not introduce any noticeable slowdown for any
real use case (although we can design special benchmark which will
do myriads of (length xxx) and nothing more).

Dmitry

Re: [Emacs-diffs] /srv/bzr/emacs/trunk r109157: Compact buffers when idle.

[Stefan Monnier]

>> I can't think of any case where that would provide any noticeable
>> benefit (whereas I can think of cases where it just wastes resources).
> Hm... I expect this to be something exotic, like replace-string
> across hundreds of buffers. On the other side, it's quite
> typical code refactoring operation. When I need to change function
> name across the project, I'm using perl just because Emacs is too slow :-(.

I see no evidence that calling compact-buffers from an idle timer will
make any difference to this scenario.

>> The GC is run fairly regularly, also typically while idle, and does the
>> compaction.
> 1. Typically it happens when we're executing byte code, or from eval.c,
>    when we want to be as fast as possible.

Again, I don't see how calling compact-buffers from an idle timer will
make any difference to this case.  BTW, while running the GC in those
cases may be a very visible operation, it's not necessarily inefficient
(unless you consider the usual calls to `free' in non-GC languages as
a unneeded inefficiency), it's just that freeing memory is part of the
needed work and we simply allocated too much memory to be able to
postpone this work to some idle time.

> 2. I don't understand why idle call to Fgarbage_collect is glued with
>    auto-save-timeout. If I don't want to use auto-save, why I shouldn't
>    call GC when idle?

I don't know, probably because it was simpler that way.  Feel free to
propose changes in this area.  Again, I don't see how that is relevant
to calling compact-buffers from an idle timer.

>> So until you can provide some concrete and convincing numbers that show
>> some benefit, I'd ask you to remove that pat of your patch.
> Hm... I suppose that 3GHz CPUs makes a lot of small optimizations almost
> invisible; but I believe it doesn't mean that they're not needed anymore.

I'm not opposed to optimizations, but I do want to first see evidence
that it is indeed an optimization and not a pessimization.

Also note that we're not talking removing some optimization that was
needed back then, but about adding a new optimization, even though it
hasn't seemed to be needed for the last 25 years, hence the higher
requirements.


        Stefan