GNU bug report logs - #31888
27.0.50; Segmentation fault in replace-buffer-contents

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 31888 in the body.
You can then email your comments to 31888 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Michał Kondraciuk <k.michal <at> zoho.com>
To: bug-gnu-emacs <at> gnu.org
Subject: 27.0.50; Segmentation fault in replace-buffer-contents
Date: Sun, 17 Jun 2018 15:12:10 +0200

[Message part 1 (text/plain, inline)]

Run this shell command in Emacs source tree (the file contents.c was
generated with clang-format):

emacs -Q src/dispnew.c contents.c --eval '(with-current-buffer
"dispnew.c" (replace-buffer-contents "contents.c"))'

Backtrace (full backtrace in attachment):
Thread 1 "emacs" received signal SIGSEGV, Segmentation fault.
0x00000000005b34cb in find_interval (tree=0x0, 
position=position <at> entry=-12) at ../../src/intervals.c:616
616	      if (relative_position < LEFT_TOTAL_LENGTH (tree))
#0  0x00000000005b34cb in find_interval (tree=0x0, 
position=position <at> entry=-12) at ../../src/intervals.c:616
        relative_position = -13
#1  0x00000000005b4dfd in set_point_both (charpos=-12, bytepos=-12) at 
../../src/intervals.c:1864
        to = <optimized out>
        from = <optimized out>
        toprev = <optimized out>
        fromprev = <optimized out>
        buffer_point = <optimized out>
        old_position = 160
        backwards = true
        original_position = <optimized out>
#2  0x00000000005b5586 in set_point (charpos=<optimized out>) at 
../../src/intervals.c:1754
No locals.
#3  0x000000000055c40d in Freplace_buffer_contents (source=0x184d9a4) at 
../../src/editfns.c:3267
        end_a = <optimized out>
        end_b = <optimized out>
        beg_b = <optimized out>
        a = <optimized out>
        source_buffer = <optimized out>
        min_a = 1
        min_b = 1
        size_a = <optimized out>
        a_empty = <optimized out>
        b_empty = false
        diags = <optimized out>
        buffer = <optimized out>
        sa_avail = <optimized out>
        sa_must_free = <optimized out>
        del_bytes = <optimized out>
        ins_bytes = <optimized out>
        ctx = {buffer_a = 0x1727fa0, buffer_b = 0x177d180, deletions = 
0x184db90 "\254^", insertions = 0x1853ac0 "B\327\v", fdiag = 
0x7fffe336f158, bdiag = 0x7fffe36828e8, too_expensive = 1000000}
        i = <optimized out>
        j = <optimized out>

Lisp backtrace:
"replace-buffer-contents" (0xffffc9f0)
"save-current-buffer" (0xffffcad8)
"with-current-buffer" (0xffffcb98)
"eval" (0xffffcd28)
"command-line-1" (0xffffd340)
"command-line" (0xffffdb48)
"normal-top-level" (0xffffde60)



In GNU Emacs 27.0.50 (build 4, x86_64-pc-linux-gnu, X toolkit, Xaw3d 
scroll bars)
 of 2018-06-17 built on mkc
Repository revision: fa9679ca488a17b2b6b9f31299d69c190aa86642
Windowing system distributor 'The X.Org Foundation', version 11.0.11804000
System Description: Linux Mint 18.2

Recent messages:
For information about GNU Emacs and the GNU system, type C-h C-a.

Configured using:
 'configure --with-x-toolkit=lucid'

Configured features:
XAW3D XPM JPEG TIFF GIF PNG RSVG IMAGEMAGICK SOUND GPM DBUS GSETTINGS
NOTIFY LIBSELINUX GNUTLS LIBXML2 FREETYPE M17N_FLT LIBOTF XFT ZLIB
TOOLKIT_SCROLL_BARS LUCID X11 THREADS LIBSYSTEMD LCMS2

Important settings:
  value of $LC_MONETARY: pl_PL.UTF-8
  value of $LC_NUMERIC: pl_PL.UTF-8
  value of $LANG: en_US.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Lisp Interaction

Minor modes in effect:
  tooltip-mode: t
  global-eldoc-mode: t
  eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  transient-mark-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message rmc puny seq byte-opt gv
bytecomp byte-compile cconv dired dired-loaddefs format-spec rfc822 mml
easymenu mml-sec password-cache epa derived epg epg-config gnus-util
rmail rmail-loaddefs mm-decode mm-bodies mm-encode mail-parse rfc2231
mailabbrev gmm-utils mailheader cl-loaddefs cl-lib sendmail rfc2047
rfc2045 ietf-drums mm-util mail-prsvr mail-utils elec-pair time-date
mule-util tooltip eldoc electric uniquify ediff-hook vc-hooks
lisp-float-type mwheel term/x-win x-win term/common-win x-dnd tool-bar
dnd fontset image regexp-opt fringe tabulated-list replace newcomment
text-mode elisp-mode lisp-mode prog-mode register page menu-bar
rfn-eshadow isearch timer select scroll-bar mouse jit-lock font-lock
syntax facemenu font-core term/tty-colors frame cl-generic cham georgian
utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao korean
japanese eucjp-ms cp51932 hebrew greek romanian slovak czech european
ethiopic indian cyrillic chinese composite charscript charprop
case-table epa-hook jka-cmpr-hook help simple abbrev obarray minibuffer
cl-preloaded nadvice loaddefs button faces cus-face macroexp files
text-properties overlay sha1 md5 base64 format env code-pages mule
custom widget hashtable-print-readable backquote dbusbind inotify lcms2
dynamic-setting system-font-setting font-render-setting x-toolkit x
multi-tty make-network-process emacs)

Memory information:
((conses 16 94745 9710)
 (symbols 48 20133 1)
 (miscs 40 35 86)
 (strings 32 28988 1919)
 (string-bytes 1 755246)
 (vectors 16 14157)
 (vector-slots 8 503184 11636)
 (floats 8 50 67)
 (intervals 56 232 0)
 (buffers 992 11)
 (heap 1024 14329 1068))

[gdb.txt (text/plain, attachment)]

[contents.c (text/x-csrc, attachment)]

Message #8 received at 31888 <at> debbugs.gnu.org (full text, mbox):

From: Milan Stanojević <mstanojevic <at> janestreet.com>
To: 31888 <at> debbugs.gnu.org
Subject: re: bug#31888
Date: Mon, 18 Jun 2018 17:50:13 -0400

happens in 26.1 as well

Also, it takes forever, like 30+ seconds on my machine and then emacs crashes.

Message #11 received at 31888 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Michał Kondraciuk <k.michal <at> zoho.com>
Cc: 31888 <at> debbugs.gnu.org
Subject: Re: bug#31888: 27.0.50; Segmentation fault in replace-buffer-contents
Date: Fri, 22 Jun 2018 16:03:02 +0300

> From: Michał Kondraciuk <k.michal <at> zoho.com>
> Date: Sun, 17 Jun 2018 15:12:10 +0200
> 
> Run this shell command in Emacs source tree (the file contents.c was
> generated with clang-format):
> 
> emacs -Q src/dispnew.c contents.c --eval '(with-current-buffer
> "dispnew.c" (replace-buffer-contents "contents.c"))'
> 
> Backtrace (full backtrace in attachment):
> Thread 1 "emacs" received signal SIGSEGV, Segmentation fault.
> 0x00000000005b34cb in find_interval (tree=0x0, 
> position=position <at> entry=-12) at ../../src/intervals.c:616
> 616	      if (relative_position < LEFT_TOTAL_LENGTH (tree))
> #0  0x00000000005b34cb in find_interval (tree=0x0, 
> position=position <at> entry=-12) at ../../src/intervals.c:616
>          relative_position = -13
> #1  0x00000000005b4dfd in set_point_both (charpos=-12, bytepos=-12) at 
> ../../src/intervals.c:1864
>          to = <optimized out>
>          from = <optimized out>
>          toprev = <optimized out>
>          fromprev = <optimized out>
>          buffer_point = <optimized out>
>          old_position = 160
>          backwards = true
>          original_position = <optimized out>
> #2  0x00000000005b5586 in set_point (charpos=<optimized out>) at 
> ../../src/intervals.c:1754
> No locals.
> #3  0x000000000055c40d in Freplace_buffer_contents (source=0x184d9a4) at 
> ../../src/editfns.c:3267

We were accessing memory we freed, which of course segfaults.
This blunder is now fixed on the emacs-26 branch.

The command is still too slow (takes about 2.5 min for the above use
case in my unoptimized build, about 30 sec of which is spent in
compareseq).  I will try to look into speeding it up.

Thanks.

Message #16 received at 31888-done <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: k.michal <at> zoho.com
Cc: 31888-done <at> debbugs.gnu.org
Subject: Re: bug#31888: 27.0.50; Segmentation fault in replace-buffer-contents
Date: Sat, 23 Jun 2018 14:15:23 +0300

> Date: Fri, 22 Jun 2018 16:03:02 +0300
> From: Eli Zaretskii <eliz <at> gnu.org>
> Cc: 31888 <at> debbugs.gnu.org
> 
> The command is still too slow (takes about 2.5 min for the above use
> case in my unoptimized build, about 30 sec of which is spent in
> compareseq).  I will try to look into speeding it up.

I could find only a minor speedup (about 10%), by slightly changing
buffer_chars_equal per suggestion in another bug report.

In this particular case, replacing the contents of dispnew.c with that
of contents.c takes 2018 deletions and 2781 insertions, each one of
these operations takes about 20 msec in the unoptimized build where I
timed them; the call to compareseq takes another 26 sec (expect the
times to be lower by a factor of 3 in an optimized build).  My
conclusion from various attempts to speed up the code was that most of
this time is taken by making small changes to buffer text, which
involves moving the gap, which in turn requires shuffling of buffer
text to and fro.  I don't see how can we speed that up if we stay with
the current idea of the function, and want to preserve text properties
and overlays as much as possible.

So I ended up inserting a few calls to maybe_quit into the inner loops
of the function, to allow users bail out of lengthy execution, and
warning about the potential slowness in the doc string.

I'm closing this bug; if someone has ideas for how to speed up the
function significantly, please reopen, or file a new bug.

Message #19 received at 31888 <at> debbugs.gnu.org (full text, mbox):

From: João Távora <joaotavora <at> gmail.com>
To: 31888 <at> debbugs.gnu.org
Cc: eliz <at> gnu.org, k.michal <at> zoho.com
Subject: Re: bug#31888: 27.0.50; Segmentation fault in replace-buffer-contents
Date: Mon, 25 Jun 2018 11:49:58 +0100

Eli Zaretskii <eliz <at> gnu.org> writes:

>  I don't see how can we speed that up if we stay with
> the current idea of the function, and want to preserve text properties
> and overlays as much as possible.
>
> ... 
>
> I'm closing this bug; if someone has ideas for how to speed up the
> function significantly, please reopen, or file a new bug.

I'm using this function in my Eglot package, but only because of its
ability to preserve markers, not text properties. And only one marker in
particular, the point marker.  So I wonder if sth like a
replace-buffer-contents-no-properties could be added: according to your
analysis it could be significantly faster.

João

Message #22 received at 31888 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: João Távora <joaotavora <at> gmail.com>
Cc: k.michal <at> zoho.com, 31888 <at> debbugs.gnu.org
Subject: Re: bug#31888: 27.0.50; Segmentation fault in replace-buffer-contents
Date: Mon, 25 Jun 2018 17:54:16 +0300

> From: João Távora <joaotavora <at> gmail.com>
> Cc: eliz <at> gnu.org,  k.michal <at> zoho.com
> Date: Mon, 25 Jun 2018 11:49:58 +0100
> 
> I'm using this function in my Eglot package, but only because of its
> ability to preserve markers, not text properties. And only one marker in
> particular, the point marker.  So I wonder if sth like a
> replace-buffer-contents-no-properties could be added: according to your
> analysis it could be significantly faster.

I doubt that, because keeping markers needs the same technique:
deletions interspersed with insertions, where both deletions and
insertions are as small as possible.

However, I'm working on profiling this command with better
granularity, so maybe I will have additional ideas for speeding it up.

(You should only see the slowness if there are a lot of small
differences scattered all over the buffers.  A few large differences
close together should let the function's optimized algorithm to do a
better job.)

Message #25 received at 31888 <at> debbugs.gnu.org (full text, mbox):

From: João Távora <joaotavora <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: k.michal <at> zoho.com, 31888 <at> debbugs.gnu.org
Subject: Re: bug#31888: 27.0.50; Segmentation fault in replace-buffer-contents
Date: Mon, 25 Jun 2018 16:55:59 +0100

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: João Távora <joaotavora <at> gmail.com>
>> Cc: eliz <at> gnu.org,  k.michal <at> zoho.com
>> Date: Mon, 25 Jun 2018 11:49:58 +0100
>> 
>> I'm using this function in my Eglot package, but only because of its
>> ability to preserve markers, not text properties. And only one marker in
>> particular, the point marker.  So I wonder if sth like a
>> replace-buffer-contents-no-properties could be added: according to your
>> analysis it could be significantly faster.

> I doubt that, because keeping markers needs the same technique:
> deletions interspersed with insertions, where both deletions and
> insertions are as small as possible.

Thanks for explaining.  Without looking at the code, I would expect
performance to vary in proportion to the number of things to preserve
Thus, text properties, especially font-lock's, of which there are many
more than markers, probably weigh more.

> (You should only see the slowness if there are a lot of small
> differences scattered all over the buffers.  A few large differences
> close together should let the function's optimized algorithm to do a
> better job.)

Thanks again.  FWIW, here's why I need it: when asking the LSP server to
reformat the buffer (mostly reindent + whitespace here and there), some
servers answer with multiple small edits resulting in multiple
replace-buffer-contents calls.  For these cases, some speedup can
probably be attained by only calling replace-buffer-contents when the
affected region contains point.

Unfortunately, other servers (Python's, afaik) reply with a complete
slightly changed copy of a buffer, with short whitespace sequences added
or removed more or less uniformely throughout buffer.  These cases would
probably fall into the conditions you describe.  So maybe it could help
if replace-buffer-contents accepted only a subset of buffer markers that
need saving.

João

Message #28 received at 31888 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: joaotavora <at> gmail.com
Cc: Alan Mackenzie <acm <at> muc.de>, k.michal <at> zoho.com,
 Stefan Monnier <monnier <at> iro.umontreal.ca>, 31888 <at> debbugs.gnu.org
Subject: Re: bug#31888: 27.0.50; Segmentation fault in replace-buffer-contents
Date: Fri, 29 Jun 2018 17:07:25 +0300

> Date: Mon, 25 Jun 2018 17:54:16 +0300
> From: Eli Zaretskii <eliz <at> gnu.org>
> Cc: 31888 <at> debbugs.gnu.org, k.michal <at> zoho.com
> 
> However, I'm working on profiling this command with better
> granularity, so maybe I will have additional ideas for speeding it up.

As usual, good profiling tools indicate that the problem was in
entirely unexpected places.

I've now pushed changes that make replace-buffer-contents run between
7 and 10 times faster in the use case that was posted at the beginning
of this bug report: it now finishes in 12 to 22 seconds instead of
more than 2 minutes.  Please see if these changes have any significant
effect in your case.

One aspect of the changes for which I'd like some feedback (and CC
Stefan and Alan) is that the modified code no longer calls the
modification hooks for each small insertion or deletion that the
optimized replacement script calls; instead, we call the modification
hooks just once before the series of changes and once after them.
Doing this speeds up the function by a factor of 2, so if we give up
that twofold speedup, we should do that only for a very good reason.

Message #31 received at 31888 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: Alan Mackenzie <acm <at> muc.de>, k.michal <at> zoho.com, joaotavora <at> gmail.com,
 31888 <at> debbugs.gnu.org
Subject: Re: bug#31888: 27.0.50; Segmentation fault in replace-buffer-contents
Date: Fri, 29 Jun 2018 10:51:25 -0400

> One aspect of the changes for which I'd like some feedback (and CC
> Stefan and Alan) is that the modified code no longer calls the
> modification hooks for each small insertion or deletion that the
> optimized replacement script calls; instead, we call the modification
> hooks just once before the series of changes and once after them.

I think that's fine.  But could we refine the after-change call so it
provides tighter bounds than BEGV..ZV (which is worse than what
insert-file-contents does, for example)?


        Stefan

Message #34 received at 31888 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
Cc: acm <at> muc.de, k.michal <at> zoho.com, joaotavora <at> gmail.com, 31888 <at> debbugs.gnu.org
Subject: Re: bug#31888: 27.0.50; Segmentation fault in replace-buffer-contents
Date: Fri, 29 Jun 2018 18:23:08 +0300

> From: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
> Cc: joaotavora <at> gmail.com, 31888 <at> debbugs.gnu.org, k.michal <at> zoho.com,
>         Alan Mackenzie <acm <at> muc.de>
> Date: Fri, 29 Jun 2018 10:51:25 -0400
> 
> > One aspect of the changes for which I'd like some feedback (and CC
> > Stefan and Alan) is that the modified code no longer calls the
> > modification hooks for each small insertion or deletion that the
> > optimized replacement script calls; instead, we call the modification
> > hooks just once before the series of changes and once after them.
> 
> I think that's fine.  But could we refine the after-change call so it
> provides tighter bounds than BEGV..ZV (which is worse than what
> insert-file-contents does, for example)?

What kind of refinement do you have in mind?  And where and how to get
the info about the tighter bounds?

Message #37 received at 31888 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: acm <at> muc.de, k.michal <at> zoho.com, joaotavora <at> gmail.com, 31888 <at> debbugs.gnu.org
Subject: Re: bug#31888: 27.0.50; Segmentation fault in replace-buffer-contents
Date: Fri, 29 Jun 2018 12:57:20 -0400

>> I think that's fine.  But could we refine the after-change call so it
>> provides tighter bounds than BEGV..ZV (which is worse than what
>> insert-file-contents does, for example)?
> What kind of refinement do you have in mind?  And where and how to get
> the info about the tighter bounds?

Keep track of the first and last char actually modified (or,
equivalently, keep track of the number of chars unmodified at the
beginning and at the end, as this is often easier).


        Stefan

Message #40 received at 31888 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
Cc: acm <at> muc.de, k.michal <at> zoho.com, joaotavora <at> gmail.com, 31888 <at> debbugs.gnu.org
Subject: Re: bug#31888: 27.0.50; Segmentation fault in replace-buffer-contents
Date: Fri, 29 Jun 2018 20:34:38 +0300

> From: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
> Cc: joaotavora <at> gmail.com, 31888 <at> debbugs.gnu.org, k.michal <at> zoho.com, acm <at> muc.de
> Date: Fri, 29 Jun 2018 12:57:20 -0400
> 
> Keep track of the first and last char actually modified (or,
> equivalently, keep track of the number of chars unmodified at the
> beginning and at the end, as this is often easier).

Is that worth the hassle?  The caller asked to replace the entire
buffer by something else, why should they expect after-change hooks to
be run on something other than the entire buffer?

Message #43 received at 31888 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: monnier <at> IRO.UMontreal.CA
Cc: acm <at> muc.de, k.michal <at> zoho.com, joaotavora <at> gmail.com, 31888 <at> debbugs.gnu.org
Subject: Re: bug#31888: 27.0.50; Segmentation fault in replace-buffer-contents
Date: Fri, 29 Jun 2018 20:39:20 +0300

> From: Eli Zaretskii <eliz <at> gnu.org>
> Cc: acm <at> muc.de, 31888 <at> debbugs.gnu.org, joaotavora <at> gmail.com, k.michal <at> zoho.com
> 
> Is that worth the hassle?  The caller asked to replace the entire
> buffer by something else, why should they expect after-change hooks to
> be run on something other than the entire buffer?

s/entire buffer/entire accessible portion of buffer/g

Message #46 received at 31888 <at> debbugs.gnu.org (full text, mbox):

From: João Távora <joaotavora <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: acm <at> muc.de, k.michal <at> zoho.com, monnier <at> IRO.UMontreal.CA,
 31888 <at> debbugs.gnu.org
Subject: Re: bug#31888: 27.0.50; Segmentation fault in replace-buffer-contents
Date: Fri, 29 Jun 2018 20:04:58 +0100

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: Eli Zaretskii <eliz <at> gnu.org>
>> Cc: acm <at> muc.de, 31888 <at> debbugs.gnu.org, joaotavora <at> gmail.com, k.michal <at> zoho.com
>> 
>> Is that worth the hassle?  The caller asked to replace the entire
>> buffer by something else, why should they expect after-change hooks to
>> be run on something other than the entire buffer?
>
> s/entire buffer/entire accessible portion of buffer/g

FWIW I use this on potentially small sections of the buffer, by first
narrowing down to it.  OTOH I have no need for modification hooks.  And
if I did, I'd probably use a simpler region-replacing strategy for every
region besides the one where point is.

João

Message #49 received at 31888 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: João Távora <joaotavora <at> gmail.com>
Cc: acm <at> muc.de, k.michal <at> zoho.com, monnier <at> IRO.UMontreal.CA,
 31888 <at> debbugs.gnu.org
Subject: Re: bug#31888: 27.0.50; Segmentation fault in replace-buffer-contents
Date: Fri, 29 Jun 2018 22:09:36 +0300

> From: João Távora <joaotavora <at> gmail.com>
> Cc: monnier <at> IRO.UMontreal.CA,  acm <at> muc.de,  31888 <at> debbugs.gnu.org,  k.michal <at> zoho.com
> Date: Fri, 29 Jun 2018 20:04:58 +0100
> 
> >> Is that worth the hassle?  The caller asked to replace the entire
> >> buffer by something else, why should they expect after-change hooks to
> >> be run on something other than the entire buffer?
> >
> > s/entire buffer/entire accessible portion of buffer/g
> 
> FWIW I use this on potentially small sections of the buffer, by first
> narrowing down to it.

In that case, the after-change hook will be called on the narrowed
portion of the buffer.

Message #52 received at 31888 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: acm <at> muc.de, k.michal <at> zoho.com, joaotavora <at> gmail.com, 31888 <at> debbugs.gnu.org
Subject: Re: bug#31888: 27.0.50; Segmentation fault in replace-buffer-contents
Date: Fri, 29 Jun 2018 16:40:15 -0400

>> Keep track of the first and last char actually modified (or,
>> equivalently, keep track of the number of chars unmodified at the
>> beginning and at the end, as this is often easier).
>
> Is that worth the hassle?  The caller asked to replace the entire
> buffer by something else, why should they expect after-change hooks to
> be run on something other than the entire buffer?

IIUC replace-buffer-contents is meant to be used in cases where there's
a good probability that the old and new contents are almost identical,
save for a few details here and there.

So, it may very well be the case that out of the 1MB that is covered by
BEGV..ZV only 10bytes in the middle were deleted/inserted/modified, in
which case running a-c-f on those 10bytes will likely lead to
a significantly more efficient recomputation for things like font-lock.

This refinement is not indispensable, but we make this effort in pretty
much every other similar circumstance.  It's usually fairly easy/cheap
to provide those tighter bounds.


        Stefan

Message #55 received at 31888 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
Cc: acm <at> muc.de, k.michal <at> zoho.com, joaotavora <at> gmail.com, 31888 <at> debbugs.gnu.org
Subject: Re: bug#31888: 27.0.50; Segmentation fault in replace-buffer-contents
Date: Sat, 30 Jun 2018 10:44:06 +0300

> From: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
> Cc: joaotavora <at> gmail.com, 31888 <at> debbugs.gnu.org, k.michal <at> zoho.com, acm <at> muc.de
> Date: Fri, 29 Jun 2018 16:40:15 -0400
> 
> IIUC replace-buffer-contents is meant to be used in cases where there's
> a good probability that the old and new contents are almost identical,
> save for a few details here and there.
> 
> So, it may very well be the case that out of the 1MB that is covered by
> BEGV..ZV only 10bytes in the middle were deleted/inserted/modified, in
> which case running a-c-f on those 10bytes will likely lead to
> a significantly more efficient recomputation for things like font-lock.
> 
> This refinement is not indispensable, but we make this effort in pretty
> much every other similar circumstance.  It's usually fairly easy/cheap
> to provide those tighter bounds.

Sigh.  Does the below look reasonable?

diff --git a/src/editfns.c b/src/editfns.c
index 4d3c838..9002211 100644
--- a/src/editfns.c
+++ b/src/editfns.c
@@ -3238,9 +3238,21 @@ differences between the two buffers.  */)
      Instead, we announce a single modification for the entire
      modified region.  But don't do that if the caller inhibited
      modification hooks, because then they don't want that.  */
+  ptrdiff_t from, to;
   if (!inhibit_modification_hooks)
     {
-      prepare_to_modify_buffer (BEGV, ZV, NULL);
+      ptrdiff_t k, l;
+
+      /* Find the first character position to be changed.  */
+      for (k = 0; k < size_a && !bit_is_set (ctx.deletions, k); k++)
+	;
+      from = BEGV + k;
+
+      /* Find the last character position to be changed.  */
+      for (l = size_a; l > 0 && !bit_is_set (ctx.deletions, l - 1); l--)
+	;
+      to = BEGV + l;
+      prepare_to_modify_buffer (from, to, NULL);
       specbind (Qinhibit_modification_hooks, Qt);
       modification_hooks_inhibited = true;
     }
@@ -3293,8 +3305,9 @@ differences between the two buffers.  */)
 
   if (modification_hooks_inhibited)
     {
-      signal_after_change (BEGV, size_a, ZV - BEGV);
-      update_compositions (BEGV, ZV, CHECK_BORDER);
+      ptrdiff_t updated_to = to + ZV - BEGV - size_a;
+      signal_after_change (from, to - from, updated_to - from);
+      update_compositions (from, updated_to, CHECK_INSIDE);
     }
 
   return Qnil;

Message #58 received at 31888 <at> debbugs.gnu.org (full text, mbox):

From: João Távora <joaotavora <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: acm <at> muc.de, k.michal <at> zoho.com, monnier <at> IRO.UMontreal.CA,
 31888 <at> debbugs.gnu.org
Subject: Re: bug#31888: 27.0.50; Segmentation fault in replace-buffer-contents
Date: Sat, 30 Jun 2018 09:33:48 +0100

Eli Zaretskii <eliz <at> gnu.org> writes:

>> >> Is that worth the hassle?  The caller asked to replace the entire
>> >> buffer by something else, why should they expect after-change hooks to
>> >> be run on something other than the entire buffer?
>> > s/entire buffer/entire accessible portion of buffer/g
>> FWIW I use this on potentially small sections of the buffer, by first
>> narrowing down to it.
> In that case, the after-change hook will be called on the narrowed
> portion of the buffer.

Sorry Eli, my previous assessment of the need for need for tighter
bounds was misleading.  I now think I aggree with Stefan's take on this
because:

*  Some LSP servers do in fact serve an entirely reformated file even if
   the actual change is very small.  This is inneficient from these
   servers but there's nothing we can do afaik.

*  I forgot Eglot *is* using the modification hooks to record and
   transmit back changes that it does to files while connected to the
   LSP server (yes, even if the file was reformatted from the server
   side, LSP specifies we must send back the changes we did to it).

   The point here is that if modification hook sees the whole file as
   changed, Eglot is just as inneficient as the server.

Thanks very much for the optimization efforts, I'll try to do some
benchmarks once they are in master (they aren't yet right?)

João

Message #61 received at 31888 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: João Távora <joaotavora <at> gmail.com>
Cc: acm <at> muc.de, k.michal <at> zoho.com, monnier <at> IRO.UMontreal.CA,
 31888 <at> debbugs.gnu.org
Subject: Re: bug#31888: 27.0.50; Segmentation fault in replace-buffer-contents
Date: Sat, 30 Jun 2018 14:03:01 +0300

> From: João Távora <joaotavora <at> gmail.com>
> Cc: monnier <at> IRO.UMontreal.CA,  acm <at> muc.de,  31888 <at> debbugs.gnu.org,  k.michal <at> zoho.com
> Date: Sat, 30 Jun 2018 09:33:48 +0100
> 
>    The point here is that if modification hook sees the whole file as
>    changed, Eglot is just as inneficient as the server.

Not the whole file, the whole narrowed region.  But anyway, I posted a
followup that should do what you want, I think.

> Thanks very much for the optimization efforts, I'll try to do some
> benchmarks once they are in master (they aren't yet right?)

They are on emacs-26, but you could just apply them by hand...

Message #64 received at 31888 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: acm <at> muc.de, k.michal <at> zoho.com, joaotavora <at> gmail.com, 31888 <at> debbugs.gnu.org
Subject: Re: bug#31888: 27.0.50; Segmentation fault in replace-buffer-contents
Date: Sat, 30 Jun 2018 08:54:42 -0400

> Sigh.  Does the below look reasonable?

This looks even better since you also refine the
before-change-functions call.
Thanks,


        Stefan

Message #67 received at 31888 <at> debbugs.gnu.org (full text, mbox):

From: João Távora <joaotavora <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: Alan Mackenzie <acm <at> muc.de>, k.michal <at> zoho.com,
 Stefan Monnier <monnier <at> iro.umontreal.ca>, 31888 <at> debbugs.gnu.org
Subject: Re: bug#31888: 27.0.50; Segmentation fault in replace-buffer-contents
Date: Sat, 30 Jun 2018 14:28:08 +0100

[Message part 1 (text/plain, inline)]

On Sat, Jun 30, 2018, 12:03 Eli Zaretskii <eliz <at> gnu.org> wrote:

> > From: João Távora <joaotavora <at> gmail.com>
> > Cc: monnier <at> IRO.UMontreal.CA,  acm <at> muc.de,  31888 <at> debbugs.gnu.org,
> k.michal <at> zoho.com
> > Date: Sat, 30 Jun 2018 09:33:48 +0100
> >
> >    The point here is that if modification hook sees the whole file as
> >    changed, Eglot is just as inneficient as the server.
>
> Not the whole file, the whole narrowed region.


If the server replies with the whole file, Eglot has no chance but to
widen() before applying, so both things are the same

But anyway, I posted a
> followup that should do what you want, I think.
>

I think so too, so thanks.

João

[Message part 2 (text/html, inline)]

Message #70 received at 31888 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
Cc: acm <at> muc.de, k.michal <at> zoho.com, joaotavora <at> gmail.com, 31888 <at> debbugs.gnu.org
Subject: Re: bug#31888: 27.0.50; Segmentation fault in replace-buffer-contents
Date: Sat, 30 Jun 2018 16:51:54 +0300

> From: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
> Cc: joaotavora <at> gmail.com, 31888 <at> debbugs.gnu.org, k.michal <at> zoho.com, acm <at> muc.de
> Date: Sat, 30 Jun 2018 08:54:42 -0400
> 
> > Sigh.  Does the below look reasonable?
> 
> This looks even better since you also refine the
> before-change-functions call.

Thanks, pushed.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.