Re: [Emacs-diffs] fix/no-undo-boundary-on-secondary-buffer-change e5ff575 2/2: Add a timer to ensure undo-boundaries in buffers.

Re: [Emacs-diffs] fix/no-undo-boundary-on-secondary-buffer-change e5ff575 2/2: Add a timer to ensure undo-boundaries in buffers.

[Stefan Monnier]

> +(defun undo-ensure-boundary ()
> +  "Add an `undo-boundary' if `buffer-undo-list' is long.
> +
> +Return t if an undo boundary has been added.
> +
> +Normally, Emacs will only garbage collect data from
> +`buffer-undo-list' when this list is longer than `undo-limit'. It
> +then collects data from after the first `undo-boundary' after
> +`undo-limit'. If there are no boundaries in the list,
> +`buffer-undo-list' will grow until it reaches
> +`undo-outer-limit' (by default a much larger value than
> +`undo-limit'), when it will discard the data anyway. In this
> +case, however, it is treated as an exceptional case, and the a
> +warning is signalled.
> +
> +This function add an `undo-boundary' to `buffer-undo-list' if
> +there is no other `undo-boundary', and `buffer-undo-list' is
> +longer than `undo-limit'. It provides a useful default mechanism
> +for adding an `undo-boundary' which retains data where possible,
> +without signalling warnings to the user."

I think this makes sense, but we should do it in the GC code, then.


        Stefan

Re: [Emacs-diffs] fix/no-undo-boundary-on-secondary-buffer-change e5ff575 2/2: Add a timer to ensure undo-boundaries in buffers.

[Phillip Lord]

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

>> +(defun undo-ensure-boundary ()
>> +  "Add an `undo-boundary' if `buffer-undo-list' is long.
>> +
>> +Return t if an undo boundary has been added.
>> +
>> +Normally, Emacs will only garbage collect data from
>> +`buffer-undo-list' when this list is longer than `undo-limit'. It
>> +then collects data from after the first `undo-boundary' after
>> +`undo-limit'. If there are no boundaries in the list,
>> +`buffer-undo-list' will grow until it reaches
>> +`undo-outer-limit' (by default a much larger value than
>> +`undo-limit'), when it will discard the data anyway. In this
>> +case, however, it is treated as an exceptional case, and the a
>> +warning is signalled.
>> +
>> +This function add an `undo-boundary' to `buffer-undo-list' if
>> +there is no other `undo-boundary', and `buffer-undo-list' is
>> +longer than `undo-limit'. It provides a useful default mechanism
>> +for adding an `undo-boundary' which retains data where possible,
>> +without signalling warnings to the user."
>
> I think this makes sense, but we should do it in the GC code, then.


It's possible, but it then negates the utility of the
`under-outer-limit' functionality. The point is that after
undo-outer-limit GC ditches the undo data anyway in the absence of any
boundaries, but does so noisily. But, if we run undo-ensure-boundary
before compaction, the boundary will mean the undo-list is always
GC'able with noise.

Phil

Re: [Emacs-diffs] fix/no-undo-boundary-on-secondary-buffer-change e5ff575 2/2: Add a timer to ensure undo-boundaries in buffers.

[Stefan Monnier]

> It's possible, but it then negates the utility of the
> `under-outer-limit' functionality.  The point is that after
> undo-outer-limit GC ditches the undo data anyway in the absence of any
> boundaries, but does so noisily.  But, if we run undo-ensure-boundary
> before compaction, the boundary will mean the undo-list is always
> GC'able with noise.

Hmm... making under-outer-limit (virtually) useless would be good,
actually (and your patch also goes in that direction), but indeed doing
it in the GC doesn't work quite the way I hoped.

Basically, the problem with too-long undo steps is that we can't safely
cut them into smaller pieces once they're created (it's not clear what
is a safe cut-point).
[ I mean "safe" in a soft sense: it's not like we risk a crash.  ]

So pushing a boundary is safer, if we can assume that the current state
of the undo-log corresponds to a safe cut-point.

And for some reason I thought that indeed the current state
of the undo-log during GC would usually correspond to a safe cut-point,
but actually this is not really true.

So, yes, I now think your solution is better.

So let's go with your approach, but we need to fix an efficiency problem
with it: with the code you have now, we always run a timer every 10
seconds which always loops through all buffers.
For power-saving reasons, doing that 24/7 on a mostly-idle machine can
be a bad idea, so we should try and make sure we don't wake up Emacs
when there's nothing to do.

Here's a plan:
- change the C code so that it keeps track of a set of "buffers that
  might need a boundary".
- when we push an undo element, we add the corresponding buffer to that
  set.  If the set was empty, call an Elisp function (that can start
  the timer).
- BTW shouldn't `undo-size' be called `undo-step-size' since it should
  only concern itself with the size of the current step.

The "set" could be represented as a list (and the timer can limit its
scan to the given set of buffers), or as a boolean.

BTW, we could also use the same "set" to change the "post command push
undo boundary" so it pushes boundaries on all modified buffers (so we
can through away the "push boundary upon buffer change" which causes
problems for your code).


        Stefan

Re: [Emacs-diffs] fix/no-undo-boundary-on-secondary-buffer-change e5ff575 2/2: Add a timer to ensure undo-boundaries in buffers.

[Phillip Lord]

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

>> It's possible, but it then negates the utility of the
>> `under-outer-limit' functionality.  The point is that after
>> undo-outer-limit GC ditches the undo data anyway in the absence of any
>> boundaries, but does so noisily.  But, if we run undo-ensure-boundary
>> before compaction, the boundary will mean the undo-list is always
>> GC'able with noise.
>
> Hmm... making under-outer-limit (virtually) useless would be good,

Yes, I think that having a bit of emacs which is there just to say to
the user "hey, this might be bug I want to tell you about". It might be
a sensible thing to leave in as a developer message (like "Garbage
Collecting....").



> actually (and your patch also goes in that direction), but indeed doing
> it in the GC doesn't work quite the way I hoped.
>
> Basically, the problem with too-long undo steps is that we can't safely
> cut them into smaller pieces once they're created (it's not clear what
> is a safe cut-point).
> [ I mean "safe" in a soft sense: it's not like we risk a crash.  ]
>
> So pushing a boundary is safer, if we can assume that the current state
> of the undo-log corresponds to a safe cut-point.

I don't think we can assume that is "safe" but at least (while emacs is
synchronous) is will be running *between* process interactions, if I
understand correctly. I guess a process interaction happens the C
library Emacs uses for each process decides, which is probably buffered
in many cases. Safe? Don't know. Can't think of anything better though,
abstracted from knowledge of the process.

The disadvantage of this is that when it happens it makes all of the
undo-log open for GC.


> And for some reason I thought that indeed the current state
> of the undo-log during GC would usually correspond to a safe cut-point,
> but actually this is not really true.

I guess that a GC can happen at any point at all, including in the
middle of a process interaction?.

> So, yes, I now think your solution is better.
>
> So let's go with your approach, but we need to fix an efficiency problem
> with it: with the code you have now, we always run a timer every 10
> seconds which always loops through all buffers.
> For power-saving reasons, doing that 24/7 on a mostly-idle machine can
> be a bad idea, so we should try and make sure we don't wake up Emacs
> when there's nothing to do.
>
> Here's a plan:
> - change the C code so that it keeps track of a set of "buffers that
>   might need a boundary".
> - when we push an undo element, we add the corresponding buffer to that
>   set.  If the set was empty, call an Elisp function (that can start
>   the timer).

Iff I do this, I would like to keep into elisp as much as possible.
Partly because the only reason I am doing this is too little of the undo
machinary can only be modified in C. Mostly, though, because I never
actually learned C and am doing it rather blind at the moment. I'm not
adverse to learning, of course, but that Emacs is big system to learn on.

I guess adding undo-pre-extend and undo-post-extend hooks should do the
trick? Run with the relevant buffer current. The post-extend one would
let me know if the last element is NOT a boundary. Then I can add the
buffer to the list and kick off the timer unless it's already running.
Then anyone could switch this behaviour off. Then undo-outer-limit would
become the final backstop.

Two issues. simple.el does not depend on timer, so I am not clear where
to add this. And, I am aware that `buffer-undo-list' is just a list.
Anyone can change it anywhere in lisp, which would make the hooks
difficult to enforce.


> - BTW shouldn't `undo-size' be called `undo-step-size' since it should
>   only concern itself with the size of the current step.

Yes. Probably. Or, better, to have both. I had to implement undo-size
because AFAICT, it's not possible to do from lisp.


>
> The "set" could be represented as a list (and the timer can limit its
> scan to the given set of buffers), or as a boolean.
>
> BTW, we could also use the same "set" to change the "post command push
> undo boundary" so it pushes boundaries on all modified buffers (so we
> can through away the "push boundary upon buffer change" which causes
> problems for your code).

Just to clarify, you are suggesting a post-command add undo-boundary,
rather than post-change (i.e. potentially many times per command)? I
would need to think on this.


Phil

Re: [Emacs-diffs] fix/no-undo-boundary-on-secondary-buffer-change e5ff575 2/2: Add a timer to ensure undo-boundaries in buffers.

[Stefan Monnier]

> Iff I do this, I would like to keep into elisp as much as possible.

Yes, that's generally what we try to do.

> I guess adding undo-pre-extend and undo-post-extend hooks should do the
> trick?

Not sure.  There's also the desire to keep costs down.  Adding undo
elements can happen *very often* in some cases (for some commands which
perform many changes), so the case of adding an undo record in a buffer
that already has had other undo records added since the last undo
boundary should be fast and I'd rather not run undo-pre-extend and
undo-post-extend hooks in that case.

So I think we should have a hook run whenever we push the first change
after a boundary.  That would let you (from Elisp) add the buffer to
some set of "modified" buffers, start timers, etc...

> Then anyone could switch this behaviour off. Then undo-outer-limit would
> become the final backstop.

Sounds good.

> Two issues. simple.el does not depend on timer, so I am not clear where
> to add this.

I think you can add it to simple.el for a first attempt (timer.el is
preloaded anyway).  Bootstrap is always slightly delicate, and it's
difficult to guess in advance what problems will show up and how to best
fix them.  So just assume the gods will be with you for now, and if they
don't show up, we'll see what needs to be done.

> And, I am aware that `buffer-undo-list' is just a list.  Anyone can
> change it anywhere in lisp, which would make the hooks difficult
> to enforce.

It's a problem in theory, indeed, but in practice it should be mostly
a non-issue.  IOW, I wouldn't worry about it.

> Just to clarify, you are suggesting a post-command add undo-boundary,
> rather than post-change (i.e. potentially many times per command)? I
> would need to think on this.

I'm not suggesting it: that's what the code does currently (look for
Fundo_boundary in src/keyboard.c).


        Stefan

RFM: How to make a buffer-local var in C layer

[Phillip Lord]

Sorry for the total newbie noise, but I am trying to add a buffer-local
variable to the undo machinary. In emacs-lisp I would do...

(defvar-local undo-buffer-undoably-changed nil
  "Non-nil means that that the buffer has had a recent undo-able change.

Recent means since the value of this variable was last set explicitly to nil,
usually as part of the undo machinary.")

My C is very poor (I'm learning it as I go). But I *think* I need to do
this in the C layer, as I am going to use and set this directly from
within undo.c. Or am I wrong about this? Is it easier to just use the
lisp above and access it's value in C (not sure how do do that either).

If I need to do this in C, I have worked out how to add new variables to
Emacs in C, but it's the buffer-local bit that is causing problems. Do,
I need to do follow all the steps in buffer.c, which look something like

- call it "buffer-undoably-changed"
- add to the structure in buffer.h
- add a bset-* method
- do DEFVAR_PER_BUFFER in syms_of_buffer_c

The background to this is in this thread here:

https://lists.gnu.org/archive/html/emacs-devel/2015-09/msg00013.html

Sorry for dumb questions; this bit of Emacs is totally new to me.

Phil

Re: RFM: How to make a buffer-local var in C layer

[Stefan Monnier]

> (defvar-local undo-buffer-undoably-changed nil
>   "Non-nil means that that the buffer has had a recent undo-able change.

This macroexpands to defvar + make-variable-buffer-local.

In C defvar turns into DEFVAR_LISP, and make-variable-buffer-local turns
into Fmake_variable_buffer_local.

fontification-functions is defined in the C code exactly that way.


        Stefan

Re: RFM: How to make a buffer-local var in C layer

[Phillip Lord]

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

>> (defvar-local undo-buffer-undoably-changed nil
>>   "Non-nil means that that the buffer has had a recent undo-able change.
>
> This macroexpands to defvar + make-variable-buffer-local.
>
> In C defvar turns into DEFVAR_LISP, and make-variable-buffer-local turns
> into Fmake_variable_buffer_local.
>
> fontification-functions is defined in the C code exactly that way.


Ah, perfect! Have that part working now. Next questions!

What is the difference between Vfontification_functions and
Qfontification_functions?

How do I set the buffer local value? Something like:

Fsetq(Qfontification_functions,Qt)

Phil

Re: RFM: How to make a buffer-local var in C layer

[Stefan Monnier]

> What is the difference between Vfontification_functions and
> Qfontification_functions?

Both are C variables of type Lisp_Object.
The "V" one contains the value of the variable
`fontification-functions', while the Q one contains the symbol
`fontification-functions'.

> How do I set the buffer local value? Something like:
> Fsetq(Qfontification_functions,Qt)

You could (tho calling Fset rather than Fsetq), but you can do it much
more simply/cheaply with:

   Vfontification_functions = Qt;


-- Stefan

Re: RFM: How to make a buffer-local var in C layer

[Stefan Monnier]

> Fsetq(Qfontification_functions,Qt)

BTW, one notable difference (other than performance) between

    Fset (Qfontification_functions, Qt);
and
    Vfontification_functions = Qt;

is that the former will obey make-variable-buffer-local while the
latter won't.  So if you want the var to be buffer-local, either use the
former, or make sure to call Fmake_local_variable explicitly at
some point.


        Stefan

Re: RFM: How to make a buffer-local var in C layer

[Phillip Lord]

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

> Both are C variables of type Lisp_Object.
> The "V" one contains the value of the variable
> `fontification-functions', while the Q one contains the symbol
> `fontification-functions'.

Ah, yes, okay.

>> Fsetq(Qfontification_functions,Qt)
>
> BTW, one notable difference (other than performance) between
>
>     Fset (Qfontification_functions, Qt);
> and
>     Vfontification_functions = Qt;
>
> is that the former will obey make-variable-buffer-local while the
> latter won't.  So if you want the var to be buffer-local, either use the
> former, or make sure to call Fmake_local_variable explicitly at
> some point.


Okay. Performance is irrelevant here, to be honest, so I've gone for
Fset.

I've pushed those changes up to my branch now and they appear to be
working. I think that I have all the C support that I need, and I'd
welcome any comments that you have (don't worry about all the commits
and change messages -- I'm planning on squashing before I'd think about
moving to master).

I'll add the emacs-lisp support next, which should be relatively
simple(.el).

Phil