Re: An idea: combine-change-calls

[Alan Mackenzie <acm@muc.de>]

Hello, Stefan.

On Sun, Mar 25, 2018 at 16:05:40 -0400, Stefan Monnier wrote:
> > I've actually got a working implementation going.  It is this:

> >     (defmacro combine-change-calls (beg end &rest form)
> >       `(if (not inhibit-modification-hooks)
> >            (let* ((-beg- ,beg) (-end- ,end)
> >                   (end-marker (copy-marker -end-)))
> >              (run-hook-with-args 'before-change-functions beg end)
> >              (let ((inhibit-modification-hooks t))
> >                ,@form)
> >              (run-hook-with-args 'after-change-functions
> >                                  beg (marker-position end-marker)
> >                                  (- -end- -beg-)))
> >          ,@form))

> You need to evaluate `beg` and `end` even if inhibit-modification-hooks
> is set, otherwise someone will get bitten.

> I recommend you move the `form` to a lambda so you don't have to
> duplicate it:

>     `(let ((body (lambda () ,@form))
>            (-beg- ,beg)
>            (-end- ,end))
>        ...)

> Another benefit is that by moving `form` outside of the `let*`, you
> won't need to use gensym/make-symbol nor obfuscated names.

> I'd also recommend you check that `beg` hasn't changed position and that
> the distance between end-marker and point-max remained the same.

> >> Maybe combine-change-calls should also combine all those changes on the
> >> undo-list into a big "delete+insert" (of course, it could also try and
> >> keep the undo granularity but mark those undo entries so that they're
> >> undone within their own combine-change-calls).
> > :-)  Either of those would be quite a project, but possibly worth doing.

> Replacing the entries with a pair of delete+insert should be
> pretty easy.  Something like

>     (let ((old-buffer-undo-list buffer-undo-list)
>           (orig-text (buffer-substring beg end)))
>       ...
>       (setq buffer-undo-list
>             `((,(marker-position end-marker) ,beg)
>               (,orig-text . ,beg)
>               . ,old-buffer-undo-list)))

> modulo sanity checks (i.e. don't do it if undo is disabled and don't do
> it if old-buffer-undo-list is not within buffer-undo-list any more).

I'm experimenting with a different strategy: surrounding the mass of
elements in buffer-undo-list with a `(combine-change-begin ,beg ,end)
and a `(combine-change-end ,beg ,end).  This is less violent to the undo
mechanism, for example, still permitting programs to analyse the undo
list.

primitive-undo, when it meets the latter of these, calls
before-change-functions, binds inhibit-modification-hooks to t and calls
itself recursively.  This recursive invocation is terminated by the
combine-change-begin, after-change-functions being called immediately on
return.

The two arms inserted into the pcase form in primitive-undo look like:

          (`(combine-change-end
             ,(and beg (pred integerp))
             ,(and end (pred integerp)))
           (save-excursion
             (run-hook-with-args 'before-change-functions beg end))
           (setq old-len (- end beg))
           (let ((inhibit-modification-hooks t))
             (setq list (primitive-undo 1 list))))
          (`(combine-change-begin
             ,(and beg (pred integerp))
             ,(and end (pred integerp)))
           (if old-len
               ;; Non-nested invocation of `primitive-undo'.
               (save-excursion
                 (run-hook-with-args 'after-change-functions beg end old-len)
                 (setq old-len nil))
             ;; Nested invocation of `primitive-undo'.  Push the element back
             ;; on the list, and push nil to terminate this invocation.
             (push next list)
             (push nil list)))

(where `old-len' is an extra local variable bound to nil in the
surrounding `let' form).

The current version of combine-change-calls, incorporating (at least
most of) your suggestions now looks like:

   (defmacro combine-change-calls (beg end &rest form)
      `(let* ((-beg- ,beg)
              (-end- ,end)
              (body (lambda () ,@form))
              (end-marker (copy-marker -end-)))
         (if inhibit-modification-hooks
             (funcall body)
           (run-hook-with-args 'before-change-functions -beg- -end-)
           (unless (eq buffer-undo-list t)
             (push `(combine-change-begin ,-beg- ,-end-) buffer-undo-list))
           (unwind-protect
               (let ((inhibit-modification-hooks t))
                 (funcall body))
             (unless (eq buffer-undo-list t)
               (push `(combine-change-end ,-beg- ,(marker-position end-marker))
                     buffer-undo-list)))
           (run-hook-with-args 'after-change-functions
                               beg (marker-position end-marker)
                               (- -end- -beg-)))))
 
This makes undo blindingly fast after a large comment-region operation.
It doesn't always leave point in the right place (I understand why -
it's the C function record_point failing to record point because the top
element of buffer-undo-list is no longer nil; it's a
combine-change-begin list).

Do you have any more helpful suggestions for this idea?

Basically, the combine-change-calls idea works.  Given enough
encouragement, I will get my disorganised changes into a proper patch
with documentation, with a view to pushing it to master.

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).