[RFC]: replace-region-contents

[RFC]: replace-region-contents

[Tassilo Horn]

Hi all,

on gnu-emacs-help I've asked why my wrapper command around
`json-pretty-print' doesn't restore point although it uses
`save-excursion'.  The reason is that `json-pretty-print' replaces the
region with copy, delete, and insert.  Robert and Eli pointed me to the
new `replace-buffer-contents' which allowed me to rewrite
`json-pretty-print' in a way where point stays where it has been before
pretty-printing:

--8<---------------cut here---------------start------------->8---
(defun json-pretty-print (begin end)
  "Pretty-print selected region."
  (interactive "r")
  (save-excursion
    (save-restriction
      (narrow-to-region begin end)
      (goto-char (point-min))
      (atomic-change-group
        (let ((json-encoding-pretty-print t)
              ;; Distinguish an empty objects from 'null'
              (json-null :json-null)
              ;; Ensure that ordering is maintained
              (json-object-type 'alist)
              (obj (json-read))
              (orig-buffer (current-buffer)))
          (with-temp-buffer
            (insert (json-encode obj))
            (let ((tmp-buffer (current-buffer)))
              (set-buffer orig-buffer)
              (replace-buffer-contents tmp-buffer))))))))
--8<---------------cut here---------------end--------------->8---

I think that replacing a region by transforming its contents in some
arbitray way is some generally useful feature, so I'd propose to extract
that functionality into some new function `replace-region-contents' I'd
like to add to subr-x.el.  Here it is and how it is applied in the
json-pretty-printing scenario.

--8<---------------cut here---------------start------------->8---
;; in subr-x.el (or wherever you please)
(defun replace-region-contents (beg end extract-fn inject-fn)
  "Replace the region between BEG and END using EXTRACT-FN and INJECT-FN.

The current buffer is narrowed to the region between BEG and END,
then EXTRACT-FN is called in order to extract some value.
Thereafter, INJECT-FN is called with that value in a temporary
buffer which it should populate.

Finally, the region in the source buffer is replaced with the
contents of the temporary buffer prepared by INJECT-FN using
`replace-buffer-contents'."
  (save-excursion
    (save-restriction
      (narrow-to-region beg end)
      (goto-char (point-min))
      (atomic-change-group
	(let ((source-buffer (current-buffer))
	      (extracted (funcall extract-fn)))
	  (with-temp-buffer
	    (funcall inject-fn extracted)
	    (let ((tmp-buffer (current-buffer)))
              (set-buffer source-buffer)
              (replace-buffer-contents tmp-buffer))))))))

;; in json.el
(defun json-pretty-print (begin end)
  "Pretty-print selected region."
  (interactive "r")
  (replace-region-contents
   begin end
   (lambda ()
     (let ((json-null :json-null)
	   ;; Ensure that ordering is maintained
	   (json-object-type 'alist))
       (json-read)))
   (lambda (json-obj)
     (let ((json-encoding-pretty-print t)
	   ;; Distinguish an empty objects from 'null'
	   (json-null :json-null))
       (insert (json-encode json-obj))))))
--8<---------------cut here---------------end--------------->8---

Does that seem like a good idea, is there anything to improve, or should
I just fix `json-pretty-print' as in the first snippet?

Bye,
Tassilo

Re: [RFC]: replace-region-contents

[Marcin Borkowski]

On 2019-02-01, at 22:20, Tassilo Horn <tsdh@gnu.org> wrote:

> Hi all,
>
> on gnu-emacs-help I've asked why my wrapper command around
> `json-pretty-print' doesn't restore point although it uses
> `save-excursion'.  The reason is that `json-pretty-print' replaces the
> region with copy, delete, and insert.  Robert and Eli pointed me to the
> new `replace-buffer-contents' which allowed me to rewrite
> `json-pretty-print' in a way where point stays where it has been before
> pretty-printing:
>
> [...]
>
> --8<---------------cut here---------------start------------->8---
> ;; in subr-x.el (or wherever you please)
> (defun replace-region-contents (beg end extract-fn inject-fn)
>   "Replace the region between BEG and END using EXTRACT-FN and INJECT-FN.
>
> The current buffer is narrowed to the region between BEG and END,
> then EXTRACT-FN is called in order to extract some value.
> Thereafter, INJECT-FN is called with that value in a temporary
> buffer which it should populate.
>
> Finally, the region in the source buffer is replaced with the
> contents of the temporary buffer prepared by INJECT-FN using
> `replace-buffer-contents'."

It looks fairly interesting, and I can see quite a few uses for
a function like this.  The docstring doesn't seem to explain a lot to
me, though.  In particular, what are the arguments and return values of
EXTRACT-FN and INJECT-FN?

(I don't have time now to study the code.)

Best,

-- 
Marcin Borkowski
http://mbork.pl

Re: [RFC]: replace-region-contents

[Tassilo Horn]

Marcin Borkowski <mbork@mbork.pl> writes:

Hi Marcin,

>> --8<---------------cut here---------------start------------->8---
>> ;; in subr-x.el (or wherever you please)
>> (defun replace-region-contents (beg end extract-fn inject-fn)
>>   "Replace the region between BEG and END using EXTRACT-FN and INJECT-FN.
>>
>> The current buffer is narrowed to the region between BEG and END,
>> then EXTRACT-FN is called in order to extract some value.
>> Thereafter, INJECT-FN is called with that value in a temporary
>> buffer which it should populate.
>>
>> Finally, the region in the source buffer is replaced with the
>> contents of the temporary buffer prepared by INJECT-FN using
>> `replace-buffer-contents'."
>
> It looks fairly interesting, and I can see quite a few uses for a
> function like this.  The docstring doesn't seem to explain a lot to
> me, though.  In particular, what are the arguments and return values
> of EXTRACT-FN and INJECT-FN?

EXTRACT-FN has no argument.  It's just run in the source buffer.
Whatever it returns is passed as the argument to INJECT-FN which is run
in a temporary, empty buffer and which it should use to prepare the
temporary buffer whose contents will replace the original region in the
source buffer.

Bye,
Tassilo

Re: [RFC]: replace-region-contents

[Stefan Monnier]

> ;; in subr-x.el (or wherever you please)
> (defun replace-region-contents (beg end extract-fn inject-fn)
>   "Replace the region between BEG and END using EXTRACT-FN and INJECT-FN.
>
> The current buffer is narrowed to the region between BEG and END,
> then EXTRACT-FN is called in order to extract some value.
> Thereafter, INJECT-FN is called with that value in a temporary
> buffer which it should populate.

Why two functions instead of just one?

Also, I think the docstring should hint at the fact that this is meant
for cases where the before and after text share significant parts (so
there's a chance of meaningfully preserving markers).  It might do that
simply be referring to `replace-buffer-contents`.

>       (atomic-change-group

Why?  AFAICT the buffer is not modified until we get to calling
replace-buffer-contents which already has (or should have) the
atomicity property.


        Stefan


PS: BTW, maybe replace-buffer-contents should be changed so it
interprets a string argument as the text to use as replacement rather
than as a buffer name.

Re: [RFC]: replace-region-contents

[Tassilo Horn]

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

>> ;; in subr-x.el (or wherever you please)
>> (defun replace-region-contents (beg end extract-fn inject-fn)
>>   "Replace the region between BEG and END using EXTRACT-FN and INJECT-FN.
>>
>> The current buffer is narrowed to the region between BEG and END,
>> then EXTRACT-FN is called in order to extract some value.
>> Thereafter, INJECT-FN is called with that value in a temporary
>> buffer which it should populate.
>
> Why two functions instead of just one?

You mean by copying the region from the source buffer to the temporary
buffer, and then a single function could act just in there?  Maybe a
drawback could be that an EXTRACT-FN might need the original buffer's
configuration, e.g., syntax table, etc.

> Also, I think the docstring should hint at the fact that this is meant
> for cases where the before and after text share significant parts (so
> there's a chance of meaningfully preserving markers).  It might do
> that simply be referring to `replace-buffer-contents`.

It does refer to `replace-buffer-contents'.

>>       (atomic-change-group
>
> Why?  AFAICT the buffer is not modified until we get to calling
> replace-buffer-contents which already has (or should have) the
> atomicity property.

Correct.  I've extracted that from the original `json-pretty-print' and
it's not required anymore.

Bye,
Tassilo

Re: [RFC]: replace-region-contents

[Stefan Monnier]

>> Why two functions instead of just one?
> You mean by copying the region from the source buffer to the temporary
> buffer, and then a single function could act just in there?

No, a function which directly returns the text to insert in the form of
a string (or a buffer, I guess).

> It does refer to `replace-buffer-contents'.

Oh, indeed, I see it now, sorry.


        Stefan

Re: [RFC]: replace-region-contents

[Tassilo Horn]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

>>> Why two functions instead of just one?
>> You mean by copying the region from the source buffer to the temporary
>> buffer, and then a single function could act just in there?
>
> No, a function which directly returns the text to insert in the form
> of a string (or a buffer, I guess).

Like so?

--8<---------------cut here---------------start------------->8---
(defun replace-region-contents (beg end replace-fn)
  (save-excursion
    (save-restriction
      (narrow-to-region beg end)
      (goto-char (point-min))
      (let ((repl (funcall replace-fn)))
	(if (bufferp repl)
	    (replace-buffer-contents repl)
	  (let ((source-buffer (current-buffer)))
	    (with-temp-buffer
	      (insert repl)
	      (let ((tmp-buffer (current-buffer)))
		(set-buffer source-buffer)
		(replace-buffer-contents tmp-buffer)))))))))
--8<---------------cut here---------------end--------------->8---

In the `json-pretty-print' scenario, that would indeed be even easier
(because `json-encode' returns a string anyway):

--8<---------------cut here---------------start------------->8---
(defun json-pretty-print (begin end)
  "Pretty-print selected region."
  (interactive "r")
  (let ((json-encoding-pretty-print t)
        ;; Distinguish an empty objects from 'null'
        (json-null :json-null)
        ;; Ensure that ordering is maintained
        (json-object-type 'alist))
    (replace-region-contents
     begin end
     (lambda () (json-encode (json-read))))))
--8<---------------cut here---------------end--------------->8---

I don't have a preference.  I guess Eli might argue that this version
encourages passing strings around instead of using buffers.  I'd explain
in the doc string that in the case of a string return value, we're going
thru a temporary buffer anyway, so if your REPLACE-FN ends in
(buffer-substring ...), you're clearly doing something wrong...

Bye,
Tassilo

Re: [RFC]: replace-region-contents

[Tassilo Horn]

Tassilo Horn <tsdh@gnu.org> writes:

>> No, a function which directly returns the text to insert in the form
>> of a string (or a buffer, I guess).
>
> Like so?
>
> (defun replace-region-contents (beg end replace-fn)
>   (save-excursion
>     (save-restriction
>       (narrow-to-region beg end)
>       (goto-char (point-min))
>       (let ((repl (funcall replace-fn)))
> 	(if (bufferp repl)
> 	    (replace-buffer-contents repl)
> 	  (let ((source-buffer (current-buffer)))
> 	    (with-temp-buffer
> 	      (insert repl)
> 	      (let ((tmp-buffer (current-buffer)))
> 		(set-buffer source-buffer)
> 		(replace-buffer-contents tmp-buffer)))))))))

I just became aware of the fact that we cannot use
`replace-buffer-contents' in its current state if the replacement is too
large.  It becomes unbearable slow.  Therefore we cannot simply change
`json-pretty-print' to use it, since e.g., restclient.el calls it to
format possibly huge json snippets.

I wonder where and how to fix that.  Maybe `replace-buffer-contents'
could fall back to recognize itself if it doesn't make sense to try to
restore point and marks, e.g., trivial cases like point is on
(point-min) or (point-max) and there are no marks, or the replacement
buffer's content exceeds some maximum...

Bye,
Tassilo

Re: [RFC]: replace-region-contents

[Eli Zaretskii]

> From: Tassilo Horn <tsdh@gnu.org>
> Date: Tue, 05 Feb 2019 14:21:46 +0100
> Cc: emacs-devel@gnu.org
> 
> I just became aware of the fact that we cannot use
> `replace-buffer-contents' in its current state if the replacement is too
> large.  It becomes unbearable slow.

In what version of Emacs?  I think it was slow in 26.1, but wa ssped
up significantly since then.

If you are using emacs-26 or the master branch from Git, and it is
still slow, can you show an example where it's slow, and tell how much
time it took?

> I wonder where and how to fix that.  Maybe `replace-buffer-contents'
> could fall back to recognize itself if it doesn't make sense to try to
> restore point and marks, e.g., trivial cases like point is on
> (point-min) or (point-max) and there are no marks, or the replacement
> buffer's content exceeds some maximum...

AFAIR, the speed of replace-buffer-contents doesn't depend at all on
whether markers are preserved or not, so I don't think this will
matter.

Re: [RFC]: replace-region-contents

[Tassilo Horn]

Eli Zaretskii <eliz@gnu.org> writes:

>> I just became aware of the fact that we cannot use
>> `replace-buffer-contents' in its current state if the replacement is
>> too large.  It becomes unbearable slow.
>
> In what version of Emacs?  I think it was slow in 26.1, but wa ssped
> up significantly since then.

Master as of today.

> If you are using emacs-26 or the master branch from Git, and it is
> still slow, can you show an example where it's slow, and tell how much
> time it took?

I prepared a tar.xz with a synthetic, huge, one-line JSON file generated
by https://next.json-generator.com and a lisp file benchmarking the
current against my proposed version of `json-pretty-print'.

https://drive.google.com/open?id=1kKahq3csBVXWL-nN1-eMhgDctGWBN1AR

Bye,
Tassilo

Re: [RFC]: replace-region-contents

[Eli Zaretskii]

> From: Tassilo Horn <tsdh@gnu.org>
> Cc: monnier@IRO.UMontreal.CA,  emacs-devel@gnu.org
> Date: Tue, 05 Feb 2019 18:19:13 +0100
> 
> > In what version of Emacs?  I think it was slow in 26.1, but wa ssped
> > up significantly since then.
> 
> Master as of today.
> 
> > If you are using emacs-26 or the master branch from Git, and it is
> > still slow, can you show an example where it's slow, and tell how much
> > time it took?
> 
> I prepared a tar.xz with a synthetic, huge, one-line JSON file generated
> by https://next.json-generator.com and a lisp file benchmarking the
> current against my proposed version of `json-pretty-print'.
> 
> https://drive.google.com/open?id=1kKahq3csBVXWL-nN1-eMhgDctGWBN1AR

Ouch!

I guess we'd need to limit use of replace-region-contents to small
enough JSON strings?

Re: [RFC]: replace-region-contents

[Tassilo Horn]

Eli Zaretskii <eliz@gnu.org> writes:

>> I prepared a tar.xz with a synthetic, huge, one-line JSON file
>> generated by https://next.json-generator.com and a lisp file
>> benchmarking the current against my proposed version of
>> `json-pretty-print'.
>> 
>> https://drive.google.com/open?id=1kKahq3csBVXWL-nN1-eMhgDctGWBN1AR
>
> Ouch!
>
> I guess we'd need to limit use of replace-region-contents to small
> enough JSON strings?

I wouldn't want to do that check everywhere I might want to use
replace-buffer-contents.  And the size is not really what matters.
Instead the number of differences is the crucial thing.  When I run my
version on the already formatted JSON, i.e., there are zero differences,
it is done exactly as fast as the original version.

The bottleneck is `compareseq' in lib/diffseq.h.  Maybe that could
signal if more than some maximum number of differences have already been
found, and in that case `replace-buffer-contents' would fallback to a
plain delete and insert?

Bye,
Tassilo

Re: [RFC]: replace-region-contents

[Eli Zaretskii]

> From: Tassilo Horn <tsdh@gnu.org>
> Cc: monnier@IRO.UMontreal.CA,  emacs-devel@gnu.org
> Date: Wed, 06 Feb 2019 18:02:34 +0100
> 
> The bottleneck is `compareseq' in lib/diffseq.h.

Of course.

> Maybe that could signal if more than some maximum number of
> differences have already been found, and in that case
> `replace-buffer-contents' would fallback to a plain delete and
> insert?

There's the too_expensive member that we currently set to a very large
value; maybe we should set it lower? or provide control on it from
Lisp?

Also, there's the EARLY_ABORT macro that can be defined to cause an
early return when the job is too hard.  Maybe you could try defining
it to something appropriate?

Re: [RFC]: replace-region-contents

[Tassilo Horn]

Eli Zaretskii <eliz@gnu.org> writes:

Hi Paul,

the context is `replace-buffer-contents' or rather compareseq in
lib/diffseq.h being too slow when being called where the source buffer
contains a huge, one-line JSON string and the replacing buffer contains
the formatted version.  I.e., the buffer contents are almost equal
modulo gazillion of whitespace/linebreak changes.

>> Maybe that could signal if more than some maximum number of
>> differences have already been found, and in that case
>> `replace-buffer-contents' would fallback to a plain delete and
>> insert?
>
> There's the too_expensive member that we currently set to a very large
> value; maybe we should set it lower? or provide control on it from
> Lisp?

I've just tried playing with that.  We currently set it too one million,
and then the `replace-buffer-contents' call won't finish within 30
minutes on my computer.  As a base-line, the current `json-pretty-print'
without `replace-buffer-contents' takes about 2 seconds.

With a too_expensive value of 1000, it takes about 14 seconds with my
huge, worst-case JSON file.  With a value of 100 it takes about 3
seconds.  With a value of 10, it segfaults.  I guess there's some
minimum boundary.

However, I'm not really sure what the implications are.  The comments
say it produces suboptimal output.  But what does that mean here?  In
the end, the formatted JSON is copied over from the replacing buffer
as-is, and that's the most important thing.

> Also, there's the EARLY_ABORT macro that can be defined to cause an
> early return when the job is too hard.  Maybe you could try defining
> it to something appropriate?

And fallback to plain delete & insert when it returned early?  Not sure
if that's better.  Right now, a (much) lower value of too_expensive
seems like a good option.

Bye,
Tassilo

Re: [RFC]: replace-region-contents

[Tassilo Horn]

Hi all,

if nobody complains, I'd like to install the following patch.

I've set too_expensive to 64 because then my benchmark was almost as
fast as the original version without `replace-buffer-contents', and I
couldn't find any difference in observable behavior in that value anyway
except lower values give much better performance but a too low value
like 10 will segfault.  My main concern was that point was at the same
position before and after pretty-printing my JSON.

I also enabled the heuristic flag, although I've seen no difference
without it.  The comments just suggest there are cases where performance
benefits from that.

--8<---------------cut here---------------start------------->8---
diff --git a/lisp/emacs-lisp/subr-x.el b/lisp/emacs-lisp/subr-x.el
index 7d9f0bba4c..5618f1b478 100644
--- a/lisp/emacs-lisp/subr-x.el
+++ b/lisp/emacs-lisp/subr-x.el
@@ -250,6 +250,33 @@ string-remove-suffix
       (substring string 0 (- (length string) (length suffix)))
     string))
 
+(defun replace-region-contents (beg end replace-fn)
+  "Replace the region between BEG and END using REPLACE-FN.
+
+REPLACE-FN runs on the current buffer narrowed to the region.  It
+should return either a string or a buffer replacing the region.
+
+The replacement is performed using `replace-buffer-contents'.
+
+Note: If the replacement is a string, it'll be placed in a
+temporary buffer so that `replace-buffer-contents' can operate on
+it.  Therefore, if you already have the replacement in a buffer,
+it makes no sense to convert it to a string using
+`buffer-substring' or similar."
+  (save-excursion
+    (save-restriction
+      (narrow-to-region beg end)
+      (goto-char (point-min))
+      (let ((repl (funcall replace-fn)))
+	(if (bufferp repl)
+	    (replace-buffer-contents repl)
+	  (let ((source-buffer (current-buffer)))
+	    (with-temp-buffer
+	      (insert repl)
+	      (let ((tmp-buffer (current-buffer)))
+		(set-buffer source-buffer)
+		(replace-buffer-contents tmp-buffer)))))))))
+
 (provide 'subr-x)
 
 ;;; subr-x.el ends here
diff --git a/lisp/json.el b/lisp/json.el
index 26cd48f41d..8f86104c19 100644
--- a/lisp/json.el
+++ b/lisp/json.el
@@ -52,6 +52,7 @@
 
 ;;; Code:
 
+(require 'subr-x)
 (require 'map)
 
 ;; Parameters
@@ -740,14 +741,14 @@ json-pretty-print-buffer
 (defun json-pretty-print (begin end)
   "Pretty-print selected region."
   (interactive "r")
-  (atomic-change-group
-    (let ((json-encoding-pretty-print t)
-          ;; Distinguish an empty objects from 'null'
-          (json-null :json-null)
-          ;; Ensure that ordering is maintained
-          (json-object-type 'alist)
-          (txt (delete-and-extract-region begin end)))
-      (insert (json-encode (json-read-from-string txt))))))
+  (let ((json-encoding-pretty-print t)
+        ;; Distinguish an empty objects from 'null'
+        (json-null :json-null)
+        ;; Ensure that ordering is maintained
+        (json-object-type 'alist))
+    (replace-region-contents
+     begin end
+     (lambda () (json-encode (json-read))))))
 
 (defun json-pretty-print-buffer-ordered ()
   "Pretty-print current buffer with object keys ordered."
diff --git a/src/editfns.c b/src/editfns.c
index a9ac263daf..7a600bacf1 100644
--- a/src/editfns.c
+++ b/src/editfns.c
@@ -1910,6 +1910,11 @@ determines whether case is significant or ignored.  */)
 
 #undef ELEMENT
 #undef EQUAL
+#define USE_HEURISTIC
+
+#ifdef USE_HEURISTIC
+#define DIFFSEQ_HEURISTIC
+#endif
 
 /* Counter used to rarely_quit in replace-buffer-contents.  */
 static unsigned short rbc_quitcounter;
@@ -2017,8 +2022,11 @@ differences between the two buffers.  */)
     .insertions = SAFE_ALLOCA (ins_bytes),
     .fdiag = buffer + size_b + 1,
     .bdiag = buffer + diags + size_b + 1,
+#ifdef DIFFSEQ_HEURISTIC
+    .heuristic = true,
+#endif
     /* FIXME: Find a good number for .too_expensive.  */
-    .too_expensive = 1000000,
+    .too_expensive = 64,
   };
   memclear (ctx.deletions, del_bytes);
   memclear (ctx.insertions, ins_bytes);
--8<---------------cut here---------------end--------------->8---

Bye,
Tassilo

Re: [RFC]: replace-region-contents

[Stefan Monnier]

> if nobody complains, I'd like to install the following patch.

I think it can go straight to subr.el.


        Stefan

Re: [RFC]: replace-region-contents

[Tassilo Horn]

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

>> if nobody complains, I'd like to install the following patch.
>
> I think it can go straight to subr.el.

Eli suggested subr-x.el, but he wasn't sure and it's been long ago (last
week or so).  So I'll wait a bit for him to speak.

Bye,
Tassilo

Re: [RFC]: replace-region-contents

[Eli Zaretskii]

> From: Tassilo Horn <tsdh@gnu.org>
> Cc: Eli Zaretskii <eliz@gnu.org>,  Paul Eggert <eggert@cs.ucla.edu>,  emacs-devel@gnu.org
> Date: Fri, 08 Feb 2019 18:17:55 +0100
> 
> Stefan Monnier <monnier@iro.umontreal.ca> writes:
> 
> >> if nobody complains, I'd like to install the following patch.
> >
> > I think it can go straight to subr.el.
> 
> Eli suggested subr-x.el, but he wasn't sure and it's been long ago (last
> week or so).  So I'll wait a bit for him to speak.

I don't necessarily object to subr.el, but I'm not sure I understand
whjy this function is different from others that went to subr-x.el.

Re: [RFC]: replace-region-contents

[Stefan Monnier]

> I don't necessarily object to subr.el, but I'm not sure I understand
> whjy this function is different from others that went to subr-x.el.

Either way is fine.


        Stefan

Re: [RFC]: replace-region-contents

[Eli Zaretskii]

> From: Tassilo Horn <tsdh@gnu.org>
> Cc: Paul Eggert <eggert@cs.ucla.edu>,  monnier@IRO.UMontreal.CA,  emacs-devel@gnu.org
> Date: Fri, 08 Feb 2019 17:23:29 +0100
> 
> if nobody complains, I'd like to install the following patch.

No complaints from me, just a few comments.

> I've set too_expensive to 64 because then my benchmark was almost as
> fast as the original version without `replace-buffer-contents', and I
> couldn't find any difference in observable behavior in that value anyway
> except lower values give much better performance but a too low value
> like 10 will segfault.  My main concern was that point was at the same
> position before and after pretty-printing my JSON.

I'd prefer to expose this to Lisp, not hardcode the value.  We could
use the hardcoded value in json.el, but I'd like to leave this to the
application in the other cases.  I'm not sure such a small value will
always be the right tradeoff, so I think we shouldn't decide just yet.

If you agree with that, let's expose this via a new optional argument
to replace-buffer-contents, and let's treat the value of nil as a very
large integer value, i.e. "no limit".

> --- a/src/editfns.c
> +++ b/src/editfns.c
> @@ -1910,6 +1910,11 @@ determines whether case is significant or ignored.  */)
>  
>  #undef ELEMENT
>  #undef EQUAL
> +#define USE_HEURISTIC
> +
> +#ifdef USE_HEURISTIC
> +#define DIFFSEQ_HEURISTIC
> +#endif
>  
>  /* Counter used to rarely_quit in replace-buffer-contents.  */
>  static unsigned short rbc_quitcounter;
> @@ -2017,8 +2022,11 @@ differences between the two buffers.  */)
>      .insertions = SAFE_ALLOCA (ins_bytes),
>      .fdiag = buffer + size_b + 1,
>      .bdiag = buffer + diags + size_b + 1,
> +#ifdef DIFFSEQ_HEURISTIC
> +    .heuristic = true,
> +#endif

Why do we need this triple-level conditional?  If we think the
heuristic is a good idea, let's just enable it unconditionally.  If
someone wants to modify Emacs on the C level, they can edit the source
as easily as they can invoke the compiler with a -U switch.

Finally, I think this needs NEWS entries, both regarding the new
function and the additional argument to replace-buffer-contents.  The
latter is also documented in the ELisp manual, which will need to be
updated.

Thanks.

P.S.  I still hope Paul will chime in and comments on the diffseq bits
we are modifying here.

Re: [RFC]: replace-region-contents

[Tassilo Horn]

Eli Zaretskii <eliz@gnu.org> writes:

>> if nobody complains, I'd like to install the following patch.
>
> No complaints from me, just a few comments.

Too late, but I'll happily do some more changes.

>> I've set too_expensive to 64 because then my benchmark was almost as
>> fast as the original version without `replace-buffer-contents', and I
>> couldn't find any difference in observable behavior in that value
>> anyway except lower values give much better performance but a too low
>> value like 10 will segfault.  My main concern was that point was at
>> the same position before and after pretty-printing my JSON.
>
> I'd prefer to expose this to Lisp, not hardcode the value.  We could
> use the hardcoded value in json.el, but I'd like to leave this to the
> application in the other cases.  I'm not sure such a small value will
> always be the right tradeoff, so I think we shouldn't decide just yet.
>
> If you agree with that, let's expose this via a new optional argument
> to replace-buffer-contents, and let's treat the value of nil as a very
> large integer value, i.e. "no limit".

Yes, fine with me.  I'll do that tomorrow.

>>  #undef ELEMENT
>>  #undef EQUAL
>> +#define USE_HEURISTIC
>> +
>> +#ifdef USE_HEURISTIC
>> +#define DIFFSEQ_HEURISTIC
>> +#endif
>>  
>>  /* Counter used to rarely_quit in replace-buffer-contents.  */
>>  static unsigned short rbc_quitcounter;
>> @@ -2017,8 +2022,11 @@ differences between the two buffers.  */)
>>      .insertions = SAFE_ALLOCA (ins_bytes),
>>      .fdiag = buffer + size_b + 1,
>>      .bdiag = buffer + diags + size_b + 1,
>> +#ifdef DIFFSEQ_HEURISTIC
>> +    .heuristic = true,
>> +#endif
>
> Why do we need this triple-level conditional?  If we think the
> heuristic is a good idea, let's just enable it unconditionally.  If
> someone wants to modify Emacs on the C level, they can edit the source
> as easily as they can invoke the compiler with a -U switch.

Ok.

> Finally, I think this needs NEWS entries, both regarding the new
> function and the additional argument to replace-buffer-contents.  The
> latter is also documented in the ELisp manual, which will need to be
> updated.

I'll write it.

Thanks,
Tassilo

Re: [RFC]: replace-region-contents

[Eli Zaretskii]

> From: Tassilo Horn <tsdh@gnu.org>
> Cc: eggert@cs.ucla.edu,  monnier@IRO.UMontreal.CA,  emacs-devel@gnu.org
> Date: Fri, 08 Feb 2019 23:03:04 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> if nobody complains, I'd like to install the following patch.
> >
> > No complaints from me, just a few comments.
> 
> Too late, but I'll happily do some more changes.

Thank you.

Apologies for not voicing this earlier, I just was under the
impression that we were still discussing alternatives for preventing
too expensive calls in diffseq, and didn't understand you were looking
at what you thought was more or less the final version of the code.

Re: [RFC]: replace-region-contents

[Tassilo Horn]

Eli Zaretskii <eliz@gnu.org> writes:

>> I've set too_expensive to 64 because then my benchmark was almost as
>> fast as the original version without `replace-buffer-contents', and I
>> couldn't find any difference in observable behavior in that value anyway
>> except lower values give much better performance but a too low value
>> like 10 will segfault.  My main concern was that point was at the same
>> position before and after pretty-printing my JSON.
>
> I'd prefer to expose this to Lisp, not hardcode the value.  We could
> use the hardcoded value in json.el, but I'd like to leave this to the
> application in the other cases.  I'm not sure such a small value will
> always be the right tradeoff, so I think we shouldn't decide just yet.
>
> If you agree with that, let's expose this via a new optional argument
> to replace-buffer-contents, and let's treat the value of nil as a very
> large integer value, i.e. "no limit".

Done on branch scratch/replace-region-contents.

>> --- a/src/editfns.c
>> +++ b/src/editfns.c
>> @@ -1910,6 +1910,11 @@ determines whether case is significant or ignored.  */)
>> +#define USE_HEURISTIC
>> +
>> +#ifdef USE_HEURISTIC
>> +#define DIFFSEQ_HEURISTIC
>> +#endif
>> @@ -2017,8 +2022,11 @@ differences between the two buffers.  */)
>>      .insertions = SAFE_ALLOCA (ins_bytes),
>>      .fdiag = buffer + size_b + 1,
>>      .bdiag = buffer + diags + size_b + 1,
>> +#ifdef DIFFSEQ_HEURISTIC
>> +    .heuristic = true,
>> +#endif
>
> Why do we need this triple-level conditional?  If we think the
> heuristic is a good idea, let's just enable it unconditionally.  If
> someone wants to modify Emacs on the C level, they can edit the source
> as easily as they can invoke the compiler with a -U switch.

Removed.

Shall I also move replace-region-contents from subr.el to subr-x.el?

> Finally, I think this needs NEWS entries, both regarding the new
> function and the additional argument to replace-buffer-contents.  The
> latter is also documented in the ELisp manual, which will need to be
> updated.

I'll write that when everybody is ok with the code.

> P.S.  I still hope Paul will chime in and comments on the diffseq bits
> we are modifying here.

Oh, yes, please.

Bye,
Tassilo

Re: [RFC]: replace-region-contents

[Eli Zaretskii]

> From: Tassilo Horn <tsdh@gnu.org>
> Cc: eggert@cs.ucla.edu,  monnier@IRO.UMontreal.CA,  emacs-devel@gnu.org
> Date: Sat, 09 Feb 2019 01:00:48 +0100
> 
> Shall I also move replace-region-contents from subr.el to subr-x.el?

It isn't important enough to argue about it, but our policy AFAIU was
to put new functions in subr-x.el, so there's a slight edge in favor
of that.

Re: [RFC]: replace-region-contents

[Tassilo Horn]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Tassilo Horn <tsdh@gnu.org>
>> Cc: eggert@cs.ucla.edu,  monnier@IRO.UMontreal.CA,  emacs-devel@gnu.org
>> Date: Sat, 09 Feb 2019 01:00:48 +0100
>> 
>> Shall I also move replace-region-contents from subr.el to subr-x.el?
>
> It isn't important enough to argue about it, but our policy AFAIU was
> to put new functions in subr-x.el, so there's a slight edge in favor
> of that.

That's fine with me, so I'll move it.

Bye,
Tassilo

Re: [RFC]: replace-region-contents

[Stefan Monnier]

> --8<---------------cut here---------------start------------->8---
> (defun replace-region-contents (beg end replace-fn)
>   (save-excursion
>     (save-restriction
>       (narrow-to-region beg end)
>       (goto-char (point-min))
>       (let ((repl (funcall replace-fn)))
> 	(if (bufferp repl)
> 	    (replace-buffer-contents repl)
> 	  (let ((source-buffer (current-buffer)))
> 	    (with-temp-buffer
> 	      (insert repl)
> 	      (let ((tmp-buffer (current-buffer)))
> 		(set-buffer source-buffer)
> 		(replace-buffer-contents tmp-buffer)))))))))
> --8<---------------cut here---------------end--------------->8---

LGTM

> I guess Eli might argue that this version encourages passing strings
> around instead of using buffers.

Passing strings around is not bad.  I think many applications where
replace-region-contents is desirable will generate strings more
efficiently than buffers rather than the other way around (after all,
if the rest can be in a buffer, there's a good chance that you can do
the modifications in-place on-the-fly rather than go through
replace-region-contents).

> I'd explain in the doc string that in the case of a string return
> value, we're going thru a temporary buffer anyway, so if your
> REPLACE-FN ends in (buffer-substring ...), you're clearly doing
> something wrong...

Sounds good.


        Stefan

Re: [RFC]: replace-region-contents

[Tassilo Horn]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

>> --8<---------------cut here---------------start------------->8---
>> (defun replace-region-contents (beg end replace-fn)
>>   (save-excursion
>>     (save-restriction
>>       (narrow-to-region beg end)
>>       (goto-char (point-min))
>>       (let ((repl (funcall replace-fn)))
>> 	(if (bufferp repl)
>> 	    (replace-buffer-contents repl)
>> 	  (let ((source-buffer (current-buffer)))
>> 	    (with-temp-buffer
>> 	      (insert repl)
>> 	      (let ((tmp-buffer (current-buffer)))
>> 		(set-buffer source-buffer)
>> 		(replace-buffer-contents tmp-buffer)))))))))
>> --8<---------------cut here---------------end--------------->8---
>
> LGTM

How would I actually use that version with a replace-fn returning a
buffer and not a string?  It looks to me that I need to do the whole
ceremony of creating a temporary buffer, setting buffers, and ensuring
that the temporary buffer is killed even in the case of an abnormal exit
myself.  That's the hassle my original version tried to eliminate in the
first place...

Bye,
Tassilo

Re: [RFC]: replace-region-contents

[Marcin Borkowski]

On 2019-02-06, at 09:07, Tassilo Horn <tsdh@gnu.org> wrote:

> Stefan Monnier <monnier@IRO.UMontreal.CA> writes:
>
>>> --8<---------------cut here---------------start------------->8---
>>> (defun replace-region-contents (beg end replace-fn)
>>>   (save-excursion
>>>     (save-restriction
>>>       (narrow-to-region beg end)
>>>       (goto-char (point-min))
>>>       (let ((repl (funcall replace-fn)))
>>> 	(if (bufferp repl)
>>> 	    (replace-buffer-contents repl)
>>> 	  (let ((source-buffer (current-buffer)))
>>> 	    (with-temp-buffer
>>> 	      (insert repl)
>>> 	      (let ((tmp-buffer (current-buffer)))
>>> 		(set-buffer source-buffer)
>>> 		(replace-buffer-contents tmp-buffer)))))))))
>>> --8<---------------cut here---------------end--------------->8---
>>
>> LGTM
>
> How would I actually use that version with a replace-fn returning a
> buffer and not a string?  It looks to me that I need to do the whole
> ceremony of creating a temporary buffer, setting buffers, and ensuring
> that the temporary buffer is killed even in the case of an abnormal exit
> myself.  That's the hassle my original version tried to eliminate in the
> first place...

I did not follow the whole thread, but why wouldn't `with-temp-buffer'
be a suitable candidate to conduct exactly the ceremony you mentioned?

Also, at least sometimes, buffers are better than strings to perform
e.g. replacements.  (I have a blog post in the works where I actually
measure the performance of one over the other, and due to immutability
of strings there is a lot of GC overhead when you do string replacements
a lot.)

Best,

--
Marcin Borkowski
http://mbork.pl

Re: [RFC]: replace-region-contents

[Tassilo Horn]

Marcin Borkowski <mbork@mbork.pl> writes:

>>>> --8<---------------cut here---------------start------------->8---
>>>> (defun replace-region-contents (beg end replace-fn)
>>>>   (save-excursion
>>>>     (save-restriction
>>>>       (narrow-to-region beg end)
>>>>       (goto-char (point-min))
>>>>       (let ((repl (funcall replace-fn)))
>>>> 	(if (bufferp repl)
>>>> 	    (replace-buffer-contents repl)
>>>> 	  (let ((source-buffer (current-buffer)))
>>>> 	    (with-temp-buffer
>>>> 	      (insert repl)
>>>> 	      (let ((tmp-buffer (current-buffer)))
>>>> 		(set-buffer source-buffer)
>>>> 		(replace-buffer-contents tmp-buffer)))))))))
>>>> --8<---------------cut here---------------end--------------->8---
>>>
>>> LGTM
>>
>> How would I actually use that version with a replace-fn returning a
>> buffer and not a string?  It looks to me that I need to do the whole
>> ceremony of creating a temporary buffer, setting buffers, and ensuring
>> that the temporary buffer is killed even in the case of an abnormal exit
>> myself.  That's the hassle my original version tried to eliminate in the
>> first place...
>
> I did not follow the whole thread, but why wouldn't `with-temp-buffer'
> be a suitable candidate to conduct exactly the ceremony you mentioned?

You cannot use `with-temp-buffer' inside replace-fn and then return that
temp buffer, because it'll be killed as soon as control flow escapes it.
So you need to define your replace-fn in the lexical scope of an already
established `with-temp-buffer' form.  But since you will need to access
the source buffer, you need to manually arrange in which buffer to
operate what.

> Also, at least sometimes, buffers are better than strings to perform
> e.g. replacements.

Yes, sure, although replacements could be performed in the source buffer
itself.

Bye,
Tassilo

Re: [RFC]: replace-region-contents

[Stefan Monnier]

> How would I actually use that version with a replace-fn returning a
> buffer and not a string?  It looks to me that I need to do the whole
> ceremony of creating a temporary buffer, setting buffers, and ensuring
> that the temporary buffer is killed even in the case of an abnormal exit
> myself.

Indeed (yet another reason why we should have anonymous buffers, which
can be GC'd without having to go through kill-buffer), but as mentioned
elsewhere I suspect that this use case will be rare, because when the
output wants to be in a buffer rather than a string you will likely be
able to do the replacement "in-place" without using a second buffer (and
hence without needing the diffing machinery of replace-buffer-contents).


        Stefan

Re: [RFC]: replace-region-contents

[Eli Zaretskii]

> From: Tassilo Horn <tsdh@gnu.org>
> Date: Tue, 05 Feb 2019 06:57:52 +0100
> Cc: emacs-devel@gnu.org
> 
> I don't have a preference.  I guess Eli might argue that this version
> encourages passing strings around instead of using buffers.

Not just strings, _large_ strings.  Small strings are okay, we even
have a special optimization for them.  Large strings are much less
efficient, in both memory management and access, than buffers.  And in
the context of the function we are talking about, the probability of a
string to be large is IMO very high.

I'm not against a string as an option, though.

> I'd explain in the doc string that in the case of a string return
> value, we're going thru a temporary buffer anyway, so if your
> REPLACE-FN ends in (buffer-substring ...), you're clearly doing
> something wrong...

Right, thanks.

Re: [RFC]: replace-region-contents

[Stefan Monnier]

>       (atomic-change-group

Why?  AFAICT only replace-buffer-contents modifies the buffer and it
only does it atomically.

> The current buffer is narrowed to the region between BEG and END,
> then EXTRACT-FN is called in order to extract some value.
> Thereafter, INJECT-FN is called with that value in a temporary
> buffer which it should populate.

Why 2 separate functions?


        Stefan


PS: I wish replace-buffer-contents interpreted a string as the text to
use as replacement rather than as a buffer name (which I find almost
always to be a misfeature: better call `get-buffer` explicitly when we
need to convert a string to a buffer).  I think we should start by
deprecating this "can be a buffer name" usage ASAP.

Re: [RFC]: replace-region-contents

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Sat, 02 Feb 2019 11:17:12 -0500
> Cc: emacs-devel@gnu.org
> 
> PS: I wish replace-buffer-contents interpreted a string as the text to
> use as replacement rather than as a buffer name

You'd need to insert the string into a buffer anyway, before you could
run the code, so might as well do that in Lisp.

(I find text processing based on strings a bad idea in Emacs, btw.
Buffers are much more convenient and efficient.)

Re: [RFC]: replace-region-contents

[Stefan Monnier]

>> PS: I wish replace-buffer-contents interpreted a string as the text to
>> use as replacement rather than as a buffer name
> You'd need to insert the string into a buffer anyway, before you could
> run the code, so might as well do that in Lisp.

I was only talking about the API, not the implementation.  If I had to
implement the feature, I'd likely rename replace-buffer-contents to
internal--replace-buffer-contents and do the string->buffer conversion
in an Elisp wrapper, simply as a convenience to the user.

`replace-buffer-contents` is basically a fancy replacement for
delete-region + insert and `insert` accepts a string as argument, so
it's very natural to also accept a string as argument.

> (I find text processing based on strings a bad idea in Emacs, btw.

Agreed.

In the case of replace-buffer-contents, I think we could have an equally
efficient code that accepts a string instead of a buffer.  But it'd
definitely be inconvenient to write code that works both ways.

> Buffers are much more convenient and efficient.)

I find strings standing for buffers to be ugly and a source of bugs
(because buffer names can change), so even if we don't accept strings as
I suggested above, I'd prefer that we signal an error when we receive
a string rather than silently looking for a buffer that has the
same name.


        Stefan

Re: [RFC]: replace-region-contents

[Eli Zaretskii]

> From: Stefan Monnier <monnier@IRO.UMontreal.CA>
> Cc: tsdh@gnu.org, emacs-devel@gnu.org
> Date: Sun, 03 Feb 2019 12:05:57 -0500
> 
> I find strings standing for buffers to be ugly and a source of bugs

We could drop the feature of supporting buffer names, but alas, that
ship has sailed.  So we are stuck with the current API.  If accepting
a string of text is a frequent use case, we could create a wrapper
Lisp function to do that, though.