bug#16526: 24.3.50; scroll-conservatively & c-mode regression

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[martin rudalics]

With current trunk and emacs -Q evaluating the following form takes more
than two minutes here with builds on Windows and GNU/Linux:

(progn
   (setq scroll-conservatively 101)
   (find-file (concat source-directory "src/xdisp.c"))
   (end-of-buffer)
   (sit-for 3)
   (beginning-of-buffer))

It used to take about 10 seconds before revision 116070.

martin

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[martin rudalics]

What happens is that apparently back_comment 530 times scans the buffer
from the beginning of the buffer to the first comment before the current
position where the list of current positions goes like this:

(780 14143 15852 18026 20032 20480 21464 21846 22845 23484 25453 26968
...
942907 943099 944334 948653 948830 948653 948830 948653 948830 948653
948830 780 12)

The bug makes working with c-mode virtually impossible here.  So until a
correct solution to the problem is found I intend to install the patch
below in order to get back a working environment.

martin


=== modified file 'src/syntax.c'
--- src/syntax.c	2014-01-01 07:43:34 +0000
+++ src/syntax.c	2014-01-24 15:24:00 +0000
@@ -530,7 +530,8 @@
  {
    ptrdiff_t opoint = PT, opoint_byte = PT_BYTE;

-  if (!open_paren_in_column_0_is_defun_start)
+  if (!open_paren_in_column_0_is_defun_start
+      && !open_paren_in_column_0_is_hard_defun_start)
      {
        find_start_value = BEGV;
        find_start_value_byte = BEGV_BYTE;
@@ -538,6 +539,7 @@
        find_start_modiff = MODIFF;
        find_start_begv = BEGV;
        find_start_pos = pos;
+
        return BEGV;
      }

@@ -808,7 +810,8 @@

  	case Sopen:
  	  /* Assume a defun-start point is outside of strings.  */
-	  if (open_paren_in_column_0_is_defun_start
+	  if ((open_paren_in_column_0_is_defun_start
+	       || open_paren_in_column_0_is_hard_defun_start)
  	      && (from == stop
  		  || (temp_byte = dec_bytepos (from_byte),
  		      FETCH_CHAR (temp_byte) == '\n')))
@@ -3608,6 +3611,10 @@
  	       doc: /* Non-nil means an open paren in column 0 denotes the start of a defun.  */);
    open_paren_in_column_0_is_defun_start = 1;

+  DEFVAR_BOOL ("open-paren-in-column-0-is-hard-defun-start",
+	       open_paren_in_column_0_is_hard_defun_start,
+	       doc: /* Non-nil means an open paren in column 0 does denote the start of a defun.  */);
+  open_paren_in_column_0_is_hard_defun_start = 0;

    DEFVAR_LISP ("find-word-boundary-function-table",
  	       Vfind_word_boundary_function_table,

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[Alan Mackenzie]

Hello, Martin.

martin rudalics <rudalics@gmx.at> wrote:
> With current trunk and emacs -Q evaluating the following form takes more
> than two minutes here with builds on Windows and GNU/Linux:

> (progn
>   (setq scroll-conservatively 101)
>   (find-file (concat source-directory "src/xdisp.c"))
>   (end-of-buffer)
>   (sit-for 3)
>   (beginning-of-buffer))

> It used to take about 10 seconds before revision 116070.

It's taking me about 7 seconds, on an Emacs version whose latest applied
revision is 116070.

Quick update to current (latest revision 116146), and rebuild .......

It's still taking me about 7 seconds.  As far as I can see, you haven't
installed your temporary patch to syntax.c.  So, I haven't been able to
reproduce the problem yet.

Is there something more subtle I'm missing?

[P.S. isn't it good that we can use revision numbers to identify
approximate ages of repository states. ;-]

> martin

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[Juanma Barranquero]

On Fri, Jan 24, 2014 at 9:48 PM, Alan Mackenzie <acm@muc.de> wrote:

> It's still taking me about 7 seconds.  As far as I can see, you haven't
> installed your temporary patch to syntax.c.  So, I haven't been able to
> reproduce the problem yet.
>
> Is there something more subtle I'm missing?

On Windows, with current trunk, I see the same slowdown that Martin reports.

    J

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[martin rudalics]

 >> With current trunk and emacs -Q evaluating the following form takes more
 >> than two minutes here with builds on Windows and GNU/Linux:
 >
 >> (progn
 >>   (setq scroll-conservatively 101)
 >>   (find-file (concat source-directory "src/xdisp.c"))
 >>   (end-of-buffer)
 >>   (sit-for 3)
 >>   (beginning-of-buffer))
 >
 >> It used to take about 10 seconds before revision 116070.
 >
 > It's taking me about 7 seconds, on an Emacs version whose latest applied
 > revision is 116070.
 >
 > Quick update to current (latest revision 116146), and rebuild .......
 >
 > It's still taking me about 7 seconds.

Updated to revision 116153, bootstrapped (at least one hour on my
machine), still taking more than two minutes here.

 > As far as I can see, you haven't
 > installed your temporary patch to syntax.c.

I'm afraid I have to do that now.

 > So, I haven't been able to
 > reproduce the problem yet.
 >
 > Is there something more subtle I'm missing?

Is there something more subtle I'm missing?

 > [P.S. isn't it good that we can use revision numbers to identify
 > approximate ages of repository states. ;-]

It didn't help so far in this case ...

martin

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[Eli Zaretskii]

> Date: Sat, 25 Jan 2014 10:23:18 +0100
> From: martin rudalics <rudalics@gmx.at>
> Cc: Alan Mackenzie <acm@muc.de>
> 
>  >> With current trunk and emacs -Q evaluating the following form takes more
>  >> than two minutes here with builds on Windows and GNU/Linux:
>  >
>  >> (progn
>  >>   (setq scroll-conservatively 101)
>  >>   (find-file (concat source-directory "src/xdisp.c"))
>  >>   (end-of-buffer)
>  >>   (sit-for 3)
>  >>   (beginning-of-buffer))
>  >
>  >> It used to take about 10 seconds before revision 116070.
>  >
>  > It's taking me about 7 seconds, on an Emacs version whose latest applied
>  > revision is 116070.
>  >
>  > Quick update to current (latest revision 116146), and rebuild .......
>  >
>  > It's still taking me about 7 seconds.
> 
> Updated to revision 116153, bootstrapped (at least one hour on my
> machine), still taking more than two minutes here.

I see this too, both on Windows and on GNU/Linux.  Here's the profile:

  - redisplay_internal (C function)                                2560  95%
   - jit-lock-function                                             2560  95%
    - jit-lock-fontify-now                                         2560  95%
     - funcall                                                     2560  95%
      - #<compiled 0x4093379>                                      2560  95%
       - run-hook-with-args                                        2560  95%
	- font-lock-fontify-region                                 2560  95%
	 - c-font-lock-fontify-region                              2560  95%
	  - font-lock-default-fontify-region                       2559  95%
	   - font-lock-fontify-keywords-region                     2547  94%
	    - c-font-lock-complex-decl-prepare                     2543  94%
	     - c-parse-state                                       2543  94%
	      - c-parse-state-1                                    2543  94%
	       - c-append-lower-brace-pair-to-state-cache          2542  94%
		  byte-code                                        2542  94%
	       - c-append-to-state-cache                              1   0%
		  byte-code                                           1   0%
	    + #<compiled 0x407e625>                                   3   0%
	    + c-font-lock-enum-tail                                   1   0%
	   + font-lock-fontify-syntactically-region                  12   0%
	  + mapc                                                      1   0%
  + command-execute                                                 116   4%
  + ...                                                               6   0%

This takes about 40 seconds on a fast (Core i7) machine.

>  > As far as I can see, you haven't
>  > installed your temporary patch to syntax.c.
> 
> I'm afraid I have to do that now.
> 
>  > So, I haven't been able to
>  > reproduce the problem yet.
>  >
>  > Is there something more subtle I'm missing?
> 
> Is there something more subtle I'm missing?

Probably.  I actually don't understand how come scroll-conservatively
affects non-scrolling commands.  Can you elaborate on that?

You also mentioned back_comment doing something unreasonable.  Can you
expand on that, too?  This part specifically I don't understand:

> What happens is that apparently back_comment 530 times scans the buffer
> from the beginning of the buffer to the first comment before the current
> position where the list of current positions goes like this:

Since the problem happens as result of beginning-of-buffer, why would
back_comment need to "scan the buffer from the beginning of the buffer
to the first comment before the current position"?  And what is the
current position in this case?

Btw, if I disable font-lock ("M-x global-font-lock-mode RET") before
repeating the recipe, the move to bob is instantaneous, and turning on
font-lock after that doesn't seem to have any adverse effects on
responsiveness.

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[martin rudalics]

 > Probably.  I actually don't understand how come scroll-conservatively
 > affects non-scrolling commands.  Can you elaborate on that?

How should I know?  I suppose redisplay_window eventually winds up
calling the fontification function and sooner or later the c-code calls
back_comment.

 > You also mentioned back_comment doing something unreasonable.  Can you
 > expand on that, too?  This part specifically I don't understand:
 >
 >> What happens is that apparently back_comment 530 times scans the buffer
 >> from the beginning of the buffer to the first comment before the current
 >> position where the list of current positions goes like this:
 >
 > Since the problem happens as result of beginning-of-buffer, why would
 > back_comment need to "scan the buffer from the beginning of the buffer
 > to the first comment before the current position"?  And what is the
 > current position in this case?

I earlier posted the first and last positions here:

(780 14143 15852 18026 20032 20480 21464 21846 22845 23484 25453 26968
...
942907 943099 944334 948653 948830 948653 948830 948653 948830 948653
948830 780 12)

You can try by yourself.  I gathered the positions in the following
excerpt

find_defun_start (ptrdiff_t pos, ptrdiff_t pos_byte)
{
   ptrdiff_t opoint = PT, opoint_byte = PT_BYTE;

   if (!open_paren_in_column_0_is_defun_start)
     {
       find_start_value = BEGV;
       find_start_value_byte = BEGV_BYTE;
       find_start_buffer = current_buffer;
       find_start_modiff = MODIFF;
       find_start_begv = BEGV;
       find_start_pos = pos;
       <---------------------------- right here
       return BEGV;
     }

 > Btw, if I disable font-lock ("M-x global-font-lock-mode RET") before
 > repeating the recipe, the move to bob is instantaneous, and turning on
 > font-lock after that doesn't seem to have any adverse effects on
 > responsiveness.

Sure.  The problem happens obviously via jit-lock.  But IMHO disabling
`open-paren-in-column-0-is-defun-start' IS asking for trouble as long as
back_comment doesn't rely on `syntax-ppss' to find a safe position.

martin

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[Eli Zaretskii]

> Date: Sat, 25 Jan 2014 15:58:57 +0100
> From: martin rudalics <rudalics@gmx.at>
> CC: 16526@debbugs.gnu.org, acm@muc.de
> 
>  > Probably.  I actually don't understand how come scroll-conservatively
>  > affects non-scrolling commands.  Can you elaborate on that?
> 
> How should I know?  I suppose redisplay_window eventually winds up
> calling the fontification function and sooner or later the c-code calls
> back_comment.

Yes, that's what happens.  And it cannot be avoided, AFAICS, when
scroll-conservatively is on.

>  > You also mentioned back_comment doing something unreasonable.  Can you
>  > expand on that, too?  This part specifically I don't understand:
>  >
>  >> What happens is that apparently back_comment 530 times scans the buffer
>  >> from the beginning of the buffer to the first comment before the current
>  >> position where the list of current positions goes like this:
>  >
>  > Since the problem happens as result of beginning-of-buffer, why would
>  > back_comment need to "scan the buffer from the beginning of the buffer
>  > to the first comment before the current position"?  And what is the
>  > current position in this case?
> 
> I earlier posted the first and last positions here:
> 
> (780 14143 15852 18026 20032 20480 21464 21846 22845 23484 25453 26968
> ...
> 942907 943099 944334 948653 948830 948653 948830 948653 948830 948653
> 948830 780 12)

What I see is that find_defun_start is called many times, with its
first argument moving from _end_ of the buffer backwards.  This
happens when Emacs needs to redisplay the last portion of the buffer,
immediately after the call to end-of-buffer.

Alan, I suspect that you tried to reproduce this without setting
scroll-conservatively to a value above 100.  Because otherwise this is
100% reproducible.

If you still cannot reproduce this, please tell us what information to
collect for you.

>  > Btw, if I disable font-lock ("M-x global-font-lock-mode RET") before
>  > repeating the recipe, the move to bob is instantaneous, and turning on
>  > font-lock after that doesn't seem to have any adverse effects on
>  > responsiveness.
> 
> Sure.  The problem happens obviously via jit-lock.

JIT Lock is triggered also when font-lock is turned on after the move
to end of the buffer.  But the difference seems to come from the fact
that under scroll-conservatively, we examine the buffer a little bit
above/below the window, when we decide where to put window-start.

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[martin rudalics]

 >> How should I know?  I suppose redisplay_window eventually winds up
 >> calling the fontification function and sooner or later the c-code calls
 >> back_comment.
 >
 > Yes, that's what happens.  And it cannot be avoided, AFAICS, when
 > scroll-conservatively is on.

Well ... so you know why it calls back_comment around the END of the
buffer?

 > What I see is that find_defun_start is called many times,

... 530 times as I mentioned earlier ...

 > with its
 > first argument moving from _end_ of the buffer backwards.

Not monotonously.  Sometimes it's called from the same position (for
example 948653 is at least three times on my list) again.

 > This
 > happens when Emacs needs to redisplay the last portion of the buffer,
 > immediately after the call to end-of-buffer.

Hmm ... but the problem is when going to BOB.

 > JIT Lock is triggered also when font-lock is turned on after the move
 > to end of the buffer.  But the difference seems to come from the fact
 > that under scroll-conservatively, we examine the buffer a little bit
 > above/below the window, when we decide where to put window-start.

And somehow a "current position" is still near the end of the buffer at
that time.

martin

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[Eli Zaretskii]

> Date: Sat, 25 Jan 2014 17:48:02 +0100
> From: martin rudalics <rudalics@gmx.at>
> CC: 16526@debbugs.gnu.org, acm@muc.de
> 
>  >> How should I know?  I suppose redisplay_window eventually winds up
>  >> calling the fontification function and sooner or later the c-code calls
>  >> back_comment.
>  >
>  > Yes, that's what happens.  And it cannot be avoided, AFAICS, when
>  > scroll-conservatively is on.
> 
> Well ... so you know why it calls back_comment around the END of the
> buffer?

It starts at the end of the buffer, but then moves all the way back
to the beginning.

And yes, I know why: it's because scroll-conservatively causes
redisplay to examine buffer text around point, when it decides where
to place window-start.  This is triggered by redisplay after the move
to the end of the buffer.

>  > What I see is that find_defun_start is called many times,
> 
> ... 530 times as I mentioned earlier ...
> 
>  > with its
>  > first argument moving from _end_ of the buffer backwards.
> 
> Not monotonously.  Sometimes it's called from the same position (for
> example 948653 is at least three times on my list) again.

True.  But I'm not sure this is relevant to the slow redisplay.

>  > This
>  > happens when Emacs needs to redisplay the last portion of the buffer,
>  > immediately after the call to end-of-buffer.
> 
> Hmm ... but the problem is when going to BOB.

No, going to BOB is instantaneous.  The problem happens in redisplay
after going to EOB.

>  > JIT Lock is triggered also when font-lock is turned on after the move
>  > to end of the buffer.  But the difference seems to come from the fact
>  > that under scroll-conservatively, we examine the buffer a little bit
>  > above/below the window, when we decide where to put window-start.
> 
> And somehow a "current position" is still near the end of the buffer at
> that time.

Yes, because Emacs is at EOB.

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[martin rudalics]

 >> Well ... so you know why it calls back_comment around the END of the
 >> buffer?
 >
 > It starts at the end of the buffer, but then moves all the way back
 > to the beginning.

IIUC `beginning-of-buffer' does set_point_both.  Does that move all the
way back to the beginning?

 > And yes, I know why: it's because scroll-conservatively causes
 > redisplay to examine buffer text around point,

Where is `point' at that time?

 > when it decides where
 > to place window-start.  This is triggered by redisplay after the move
 > to the end of the buffer.

But the `end-of-buffer' call terminates cleanly after a few seconds -
that's what the `sit-for' proves in my code.

 >>  > What I see is that find_defun_start is called many times,
 >>
 >> ... 530 times as I mentioned earlier ...
 >>
 >>  > with its
 >>  > first argument moving from _end_ of the buffer backwards.
 >>
 >> Not monotonously.  Sometimes it's called from the same position (for
 >> example 948653 is at least three times on my list) again.
 >
 > True.  But I'm not sure this is relevant to the slow redisplay.

It hints at some really bad code embedded in really bad code.

 >>  > This
 >>  > happens when Emacs needs to redisplay the last portion of the buffer,
 >>  > immediately after the call to end-of-buffer.
 >>
 >> Hmm ... but the problem is when going to BOB.
 >
 > No, going to BOB is instantaneous.  The problem happens in redisplay
 > after going to EOB.

EOB happens before my `sit-for'.

 >>  > JIT Lock is triggered also when font-lock is turned on after the move
 >>  > to end of the buffer.  But the difference seems to come from the fact
 >>  > that under scroll-conservatively, we examine the buffer a little bit
 >>  > above/below the window, when we decide where to put window-start.
 >>
 >> And somehow a "current position" is still near the end of the buffer at
 >> that time.
 >
 > Yes, because Emacs is at EOB.

Why?

martin

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[Eli Zaretskii]

> Date: Sat, 25 Jan 2014 19:25:32 +0100
> From: martin rudalics <rudalics@gmx.at>
> CC: 16526@debbugs.gnu.org, acm@muc.de
> 
>  >> Well ... so you know why it calls back_comment around the END of the
>  >> buffer?
>  >
>  > It starts at the end of the buffer, but then moves all the way back
>  > to the beginning.
> 
> IIUC `beginning-of-buffer' does set_point_both.  Does that move all the
> way back to the beginning?

Of course not, it jumps there in one go.

>  > And yes, I know why: it's because scroll-conservatively causes
>  > redisplay to examine buffer text around point,
> 
> Where is `point' at that time?

At EOB.

>  > when it decides where
>  > to place window-start.  This is triggered by redisplay after the move
>  > to the end of the buffer.
> 
> But the `end-of-buffer' call terminates cleanly after a few seconds -
> that's what the `sit-for' proves in my code.

Right, and that's when redisplay kicks in, which invokes JIT Lock.

>  >> Not monotonously.  Sometimes it's called from the same position (for
>  >> example 948653 is at least three times on my list) again.
>  >
>  > True.  But I'm not sure this is relevant to the slow redisplay.
> 
> It hints at some really bad code embedded in really bad code.

Not necessarily.  Sometimes, this cannot be avoided.  More thorough
analysis is needed to determine whether it's really bad code.

>  >>  > This
>  >>  > happens when Emacs needs to redisplay the last portion of the buffer,
>  >>  > immediately after the call to end-of-buffer.
>  >>
>  >> Hmm ... but the problem is when going to BOB.
>  >
>  > No, going to BOB is instantaneous.  The problem happens in redisplay
>  > after going to EOB.
> 
> EOB happens before my `sit-for'.

See above.  Redisplay is separate from command execution.  So yes, we
are already at EOB, and then redisplay happens.

>  >>  > JIT Lock is triggered also when font-lock is turned on after the move
>  >>  > to end of the buffer.  But the difference seems to come from the fact
>  >>  > that under scroll-conservatively, we examine the buffer a little bit
>  >>  > above/below the window, when we decide where to put window-start.
>  >>
>  >> And somehow a "current position" is still near the end of the buffer at
>  >> that time.
>  >
>  > Yes, because Emacs is at EOB.
> 
> Why?

Because we moved there with end-of-buffer.

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[Eli Zaretskii]

> Date: Sat, 25 Jan 2014 20:31:10 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: acm@muc.de, 16526@debbugs.gnu.org
> 
> >  > when it decides where
> >  > to place window-start.  This is triggered by redisplay after the move
> >  > to the end of the buffer.
> > 
> > But the `end-of-buffer' call terminates cleanly after a few seconds -
> > that's what the `sit-for' proves in my code.
> 
> Right, and that's when redisplay kicks in, which invokes JIT Lock.

More accurately, end-of-buffer calls recenter, which needs to compute
where to put window-start, which requires Emacs to examine the text
before point.  As part of this examination, we invoke functions that
simulate display, and those trigger JIT Lock, whose fontification
function starts the madness.

bug#16526: 24.3.50; scroll-conservatively & c-mode regression. The purpose of revision 116070.

[Alan Mackenzie]

Hello, Martin and Eli.

On Sat, Jan 25, 2014 at 03:58:57PM +0100, martin rudalics wrote:
>  > Probably.  I actually don't understand how come scroll-conservatively
>  > affects non-scrolling commands.  Can you elaborate on that?

> How should I know?  I suppose redisplay_window eventually winds up
> calling the fontification function and sooner or later the c-code calls
> back_comment.

Yes.

>  > You also mentioned back_comment doing something unreasonable.  Can you
>  > expand on that, too?  This part specifically I don't understand:

>  >> What happens is that apparently back_comment 530 times scans the buffer
>  >> from the beginning of the buffer to the first comment before the current
>  >> position where the list of current positions goes like this:

>  > Since the problem happens as result of beginning-of-buffer, why would
>  > back_comment need to "scan the buffer from the beginning of the buffer
>  > to the first comment before the current position"?  And what is the
>  > current position in this case?

> I earlier posted the first and last positions here:

> (780 14143 15852 18026 20032 20480 21464 21846 22845 23484 25453 26968
> ...
> 942907 943099 944334 948653 948830 948653 948830 948653 948830 948653
> 948830 780 12)

> You can try by yourself.  I gathered the positions in the following
> excerpt

> find_defun_start (ptrdiff_t pos, ptrdiff_t pos_byte)
> {
>    ptrdiff_t opoint = PT, opoint_byte = PT_BYTE;

>    if (!open_paren_in_column_0_is_defun_start)
>      {
>        find_start_value = BEGV;
>        find_start_value_byte = BEGV_BYTE;
>        find_start_buffer = current_buffer;
>        find_start_modiff = MODIFF;
>        find_start_begv = BEGV;
>        find_start_pos = pos;
>        <---------------------------- right here
>        return BEGV;
>      }

>  > Btw, if I disable font-lock ("M-x global-font-lock-mode RET") before
>  > repeating the recipe, the move to bob is instantaneous, and turning on
>  > font-lock after that doesn't seem to have any adverse effects on
>  > responsiveness.

> Sure.  The problem happens obviously via jit-lock.  But IMHO disabling
> `open-paren-in-column-0-is-defun-start' IS asking for trouble as long as
> back_comment doesn't rely on `syntax-ppss' to find a safe position.

The reason I disabled open-paren-etc. was a bug report from Michael Welsh
Duggan, where commented out code was causing the CC Mode "state cache" to
get corrupted.  The critical bit of his source looks like this (note the
commenting out at L3):

 1. #define PARTIAL_MD5_SIZE 4096
 2.
 3. /*
 4.
 5. TODO: Partial sums (for working with very large files).
 6.
 7. typedef struct _signature
 8. {
 9.   md5_state_t state;
10.   md5_byte_t  digest[16];
11. } signature_t;
12.
13. typedef struct _signatures
14. {
15.   int         num_signatures;
16.   signature_t *signatures;
17. } signatures_t;
18.
19. */
20.
21. typedef struct _file {

With pos at BOL21, (scan-lists pos -1 -1) was returning BOL17.  It was
ignoring the commenting out.  Fscan_lists calls scan_lists which calls
back_comment.  Because open-paren-etc. was t, back_comment was tripping
up on the brace at BOL14 and thus not recognising the comment at all.

The "solution" to the bug was to bind open-paren-etc. to nil around the
critical code.  This is almost certainly what is causing the slow down.

Though for some reason, I don't see this slow down myself.  This is quite
worrying.  I am using an up-to-date (as of yesterday evening) bzr Emacs
and following your (Martin's) recipe by yanking it into a buffer and
doing C-x C-e on it.

If I've understood correctly, I think the root cause of the slowness is
the lack of optimisation of find_defun_start when open-paren-etc. is nil
- it brutally returns BOB, no matter what.  Perhaps some sort of cache of
safe positions (doesn't one exist already?) could accelerate
find_defun_start to acceptable speed in this use case.

> martin

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#16526: 24.3.50; scroll-conservatively & c-mode regression. The purpose of revision 116070.

[Stefan Monnier]

> - it brutally returns BOB, no matter what.  Perhaps some sort of cache of
> safe positions (doesn't one exist already?) could accelerate
> find_defun_start to acceptable speed in this use case.

syntax-ppss maintains such a cache.  But it's in Elisp.


        Stefan

bug#16526: 24.3.50; scroll-conservatively & c-mode regression. The purpose of revision 116070.

[Eli Zaretskii]

> Date: Sat, 25 Jan 2014 20:27:21 +0000
> Cc: 16526@debbugs.gnu.org
> From: Alan Mackenzie <acm@muc.de>
> 
> Though for some reason, I don't see this slow down myself.  This is quite
> worrying.  I am using an up-to-date (as of yesterday evening) bzr Emacs
> and following your (Martin's) recipe by yanking it into a buffer and
> doing C-x C-e on it.

How do you configure your build?  Can you show the exact configure and
make commands you use?

bug#16526: 24.3.50; scroll-conservatively & c-mode regression. The purpose of revision 116070.

[Alan Mackenzie]

Hi, Eli.

On Sun, Jan 26, 2014 at 05:41:12AM +0200, Eli Zaretskii wrote:
> > Date: Sat, 25 Jan 2014 20:27:21 +0000
> > Cc: 16526@debbugs.gnu.org
> > From: Alan Mackenzie <acm@muc.de>

> > Though for some reason, I don't see this slow down myself.  This is quite
> > worrying.  I am using an up-to-date (as of yesterday evening) bzr Emacs
> > and following your (Martin's) recipe by yanking it into a buffer and
> > doing C-x C-e on it.

> How do you configure your build?  Can you show the exact configure and
> make commands you use?

./configure --with-gif=no --with-tiff=no --with-gpm
time make -j5 bootstrap

.  Normally, I run Emacs on a Linux tty, but I tried this build under X,
too.  This didn't make a difference.  Somehow, I must have made some
sort of version control mistake.  There's no way that something so
massive at the syntax.c level can be cheated by a crafty configure
option.  ;-(

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[martin rudalics]

 >> IIUC `beginning-of-buffer' does set_point_both.  Does that move all the
 >> way back to the beginning?
 >
 > Of course not, it jumps there in one go.
 >
 >>  > And yes, I know why: it's because scroll-conservatively causes
 >>  > redisplay to examine buffer text around point,
 >>
 >> Where is `point' at that time?
 >
 > At EOB.

But if `beginning-of-buffer' jumps to BOB in one go how comes that
`point' at the time something "causes redisplay to examine buffer text
around point" is still at EOB?

 >> But the `end-of-buffer' call terminates cleanly after a few seconds -
 >> that's what the `sit-for' proves in my code.
 >
 > Right, and that's when redisplay kicks in, which invokes JIT Lock.

If I shorten my scenario to

(progn
   (setq scroll-conservatively 101)
   (find-file (concat source-directory "src/xdisp.c"))
   (end-of-buffer)
   (sit-for 3))

it terminates here in a few seconds.  When/where exactly does redisplay
kick in in the original scenario?

 >>  > No, going to BOB is instantaneous.  The problem happens in redisplay
 >>  > after going to EOB.
 >>
 >> EOB happens before my `sit-for'.
 >
 > See above.  Redisplay is separate from command execution.  So yes, we
 > are already at EOB, and then redisplay happens.

And I see nothing bad happen until that redisplay finishes.

 >>  >> And somehow a "current position" is still near the end of the buffer at
 >>  >> that time.
 >>  >
 >>  > Yes, because Emacs is at EOB.
 >>
 >> Why?
 >
 > Because we moved there with end-of-buffer.

And then `beginning-of-buffer' moves `point' to `point-min' and the
trouble starts.  We probably are miscommunicating but please check once
more your claim that the problem is with redisplay after
`end-of-buffer'.  If I omit `beginning-of-buffer' from my code I can see
no problems.

martin

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[martin rudalics]

 > More accurately, end-of-buffer calls recenter, which needs to compute
 > where to put window-start, which requires Emacs to examine the text
 > before point.  As part of this examination, we invoke functions that
 > simulate display, and those trigger JIT Lock, whose fontification
 > function starts the madness.

This doesn't match my observations.  With my shortened scenario

(progn
   (setq scroll-conservatively 101)
   (find-file (concat source-directory "src/xdisp.c"))
   (end-of-buffer)
   (sit-for 3))

there is no such problem.  Please enlighten.  I must be missing
something.

martin

bug#16526: 24.3.50; scroll-conservatively & c-mode regression. The purpose of revision 116070.

[Eli Zaretskii]

> Date: Sun, 26 Jan 2014 09:54:47 +0000
> Cc: rudalics@gmx.at, 16526@debbugs.gnu.org
> From: Alan Mackenzie <acm@muc.de>
> 
> > How do you configure your build?  Can you show the exact configure and
> > make commands you use?
> 
> ./configure --with-gif=no --with-tiff=no --with-gpm
> time make -j5 bootstrap

Try this instead, and be amazed:

  CFLAGS='-O0 -g3' ./configure --with-gif=no --with-tiff=no --with-gpm --enable-checking='yes,glyphs'
  make -j5

IOW, force an unoptimized build, and add some diagnostics code to the
display engine.

My testing indicates that -O0 more than doubles the run time in this
case, and --enable-checking doubles it some more -- for a total factor
of 5 to 6.  I get about 7 sec in an optimized version, and about 40
sec in the one configured as above.

(In general, I believe that every Emacs developer who does something
that can affect redisplay should always run an unoptimized build --
this will make any inefficiencies stand out very clearly.)

See my other message with some results of digging into this problem;
hopefully, you will be able to pick up where I left off.

> There's no way that something so massive at the syntax.c level can
> be cheated by a crafty configure option.  ;-(

Well, evidently it can.

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[Eli Zaretskii]

> Date: Sun, 26 Jan 2014 12:20:01 +0100
> From: martin rudalics <rudalics@gmx.at>
> CC: acm@muc.de, 16526@debbugs.gnu.org
> 
>  > More accurately, end-of-buffer calls recenter, which needs to compute
>  > where to put window-start, which requires Emacs to examine the text
>  > before point.  As part of this examination, we invoke functions that
>  > simulate display, and those trigger JIT Lock, whose fontification
>  > function starts the madness.
> 
> This doesn't match my observations.  With my shortened scenario
> 
> (progn
>    (setq scroll-conservatively 101)
>    (find-file (concat source-directory "src/xdisp.c"))
>    (end-of-buffer)
>    (sit-for 3))
> 
> there is no such problem.

You are right, sorry.  (I wasn't wrong, either: recenter does call the
same find_defun_start around EOB, which is what I saw.  But those
calls are very few and aren't responsible for the slowdown.  I also
wasn't wrong about point being at EOB, see below.  But I described
what happens incorrectly.)

Here's what I see in the debugger:

After beginning-of-buffer jumps to point-min, redisplay kicks in.
Since scroll-conservatively is set to a large value, redisplay first
tries to see whether it can bring point into view by scrolling the
window as little as possible.  It calls try_scrolling, which at some
point (around line 15000) tries to see whether the new location of
point is close enough to the current window start.  It does so by
calling move_it_to, which simulates the display.  While doing so,
move_it_to hits a portion of text with font-lock properties, and calls
JIT Lock to fontify them.

And here's where things go awry: For some reason, the CC Mode
fontification code decides it needs to scan the buffer backwards,
starting from EOB.  So it goes temporarily to EOB (this is why I saw
point being there), and scans all the way back, I think in this loop
from c-append-lower-brace-pair-to-state-cache, which is called with
its first argument FROM set to EOB:

	  ;; In the next pair of nested loops, the inner one moves back past a
	  ;; pair of (mis-)matching parens or brackets; the outer one moves
	  ;; back over a sequence of unmatched close brace/paren/bracket each
	  ;; time round.
	  (while
	      (progn
		(c-safe
		  (while
		      (and (setq ce (scan-lists bra -1 -1)) ; back past )/]/}; might signal
			   (setq bra (scan-lists ce -1 1)) ; back past (/[/{; might signal
			   (or (> bra here) ;(> ce here)
			       (and
				(< ce here)
				(or (not (eq (char-after bra) ?\{))
				    (and (goto-char bra)
					 (c-beginning-of-macro)
					 (< (point) macro-start-or-from))))))))
		(and ce (< ce bra)))
	    (setq bra ce))	; If we just backed over an unbalanced closing
					; brace, ignore it.

This loop takes a lot of time, of course, and is a waste of time,
since eventually try_scrolling comes to the correct conclusion that
scrolling is impossible, and instead recenters at BOB.

Why does CC Mode decide to go from EOB backwards, I don't know;
presumably, this is decided by c-parse-state-get-strategy as part of
c-parse-state-1.

For the record, below is the full C and Lisp backtrace I obtained when
c-append-lower-brace-pair-to-state-cache jumps to the EOB, which in
Lisp happens here:

  (save-excursion
    (save-restriction
      (let* (new-cons
	     (cache-pos (c-state-cache-top-lparen)) ; might be nil.
	     (macro-start-or-from
	      (progn (goto-char from)  <<<<<<<<<<<<<<<<<<<<<<<<<<<
		     (c-beginning-of-macro)
		     (point)))
	     (bra			; Position of "{".
	      ;; Don't start scanning in the middle of a CPP construct unless
	      ;; it contains HERE - these constructs, in Emacs, are "commented
	      ;; out" with category properties.
	      (if (eq (c-get-char-property macro-start-or-from 'category)
			'c-cpp-delimiter)
		    macro-start-or-from
		  from))
	     ce)			; Position of "}"

I hope this information will allow Alan to find the culprit and solve
the problem.

=============================================================================
Hardware watchpoint 4: -location current_buffer->pt

Old value = 1617
New value = 947778
0x011df59f in temp_set_point_both (
    buffer=0x380be00 <__register_frame_info+58768896>, charpos=947778,
    bytepos=947778) at intervals.c:1789
1789      SET_BUF_PT_BOTH (buffer, charpos, bytepos);
(gdb) bt
#0  0x011df59f in temp_set_point_both (
    buffer=0x380be00 <__register_frame_info+58768896>, charpos=947778,
    bytepos=947778) at intervals.c:1789
#1  0x011dfd99 in set_point_both (charpos=947778, bytepos=947778)
    at intervals.c:2045
#2  0x011df60a in set_point (charpos=947778) at intervals.c:1807
#3  0x011700fd in Fgoto_char (position=...) at editfns.c:239
#4  0x011820e2 in eval_sub (form=...) at eval.c:2176
#5  0x0117dc49 in Fprogn (body=...) at eval.c:459
#6  0x01181e50 in eval_sub (form=...) at eval.c:2124
#7  0x0117edea in FletX (args=...) at eval.c:872
#8  0x01181e50 in eval_sub (form=...) at eval.c:2124
#9  0x0117dc49 in Fprogn (body=...) at eval.c:459
#10 0x01176cc2 in Fsave_restriction (body=...) at editfns.c:3415
#11 0x01181e50 in eval_sub (form=...) at eval.c:2124
#12 0x0117dc49 in Fprogn (body=...) at eval.c:459
#13 0x0117161e in Fsave_excursion (args=...) at editfns.c:941
#14 0x01181e50 in eval_sub (form=...) at eval.c:2124
#15 0x0117dc49 in Fprogn (body=...) at eval.c:459
#16 0x01183f33 in funcall_lambda (fun=..., nargs=2, arg_vector=0x886db0)
    at eval.c:3033
#17 0x01183911 in apply_lambda (fun=..., args=...) at eval.c:2915
#18 0x0118244d in eval_sub (form=...) at eval.c:2251
#19 0x0117d9d4 in Fif (args=...) at eval.c:410
#20 0x01181e50 in eval_sub (form=...) at eval.c:2124
#21 0x0117dc49 in Fprogn (body=...) at eval.c:459
#22 0x0117db99 in Fcond (args=...) at eval.c:437
#23 0x01181e50 in eval_sub (form=...) at eval.c:2124
#24 0x0117dc49 in Fprogn (body=...) at eval.c:459
#25 0x0117ef6b in FletX (args=...) at eval.c:897
#26 0x01181e50 in eval_sub (form=...) at eval.c:2124
#27 0x0117dc49 in Fprogn (body=...) at eval.c:459
#28 0x01183f33 in funcall_lambda (fun=..., nargs=0, arg_vector=0x887310)
    at eval.c:3033
#29 0x01183911 in apply_lambda (fun=..., args=...) at eval.c:2915
#30 0x0118244d in eval_sub (form=...) at eval.c:2251
#31 0x0117dc49 in Fprogn (body=...) at eval.c:459
#32 0x01181e50 in eval_sub (form=...) at eval.c:2124
#33 0x0117fb6a in Funwind_protect (args=...) at eval.c:1206
#34 0x01181e50 in eval_sub (form=...) at eval.c:2124
#35 0x0117dc49 in Fprogn (body=...) at eval.c:459
#36 0x0117ef6b in FletX (args=...) at eval.c:897
#37 0x01181e50 in eval_sub (form=...) at eval.c:2124
#38 0x0117fb6a in Funwind_protect (args=...) at eval.c:1206
#39 0x01181e50 in eval_sub (form=...) at eval.c:2124
#40 0x0117dc49 in Fprogn (body=...) at eval.c:459
#41 0x0117da92 in Fif (args=...) at eval.c:411
#42 0x01181e50 in eval_sub (form=...) at eval.c:2124
#43 0x0117dc49 in Fprogn (body=...) at eval.c:459
#44 0x01181e50 in eval_sub (form=...) at eval.c:2124
#45 0x0117fb6a in Funwind_protect (args=...) at eval.c:1206
#46 0x01181e50 in eval_sub (form=...) at eval.c:2124
#47 0x0117dd09 in Fprog1 (args=...) at eval.c:491
#48 0x01181e50 in eval_sub (form=...) at eval.c:2124
#49 0x0117dc49 in Fprogn (body=...) at eval.c:459
#50 0x0117f401 in Flet (args=...) at eval.c:967
#51 0x01181e50 in eval_sub (form=...) at eval.c:2124
#52 0x0117dc49 in Fprogn (body=...) at eval.c:459
#53 0x01183f33 in funcall_lambda (fun=..., nargs=0, arg_vector=0x887f28)
    at eval.c:3033
#54 0x0118371d in Ffuncall (nargs=1, args=0x887f24) at eval.c:2867
#55 0x011c4bc3 in exec_byte_code (bytestr=..., vector=..., maxdepth=...,
    args_template=..., nargs=0, args=0x0) at bytecode.c:919
#56 0x01183fc8 in funcall_lambda (fun=..., nargs=1,
    arg_vector=0x3936595 <__register_frame_info+59991445>) at eval.c:3040
#57 0x01183644 in Ffuncall (nargs=2, args=0x888264) at eval.c:2855
#58 0x011c4bc3 in exec_byte_code (bytestr=..., vector=..., maxdepth=...,
    args_template=..., nargs=0, args=0x0) at bytecode.c:919
#59 0x01183fc8 in funcall_lambda (fun=..., nargs=3,
    arg_vector=0x1317efd <pure+652573>) at eval.c:3040
#60 0x01183644 in Ffuncall (nargs=4, args=0x8885b4) at eval.c:2855
#61 0x011c4bc3 in exec_byte_code (bytestr=..., vector=..., maxdepth=...,
    args_template=..., nargs=0, args=0x0) at bytecode.c:919
#62 0x01183fc8 in funcall_lambda (fun=..., nargs=3,
    arg_vector=0x1317555 <pure+650101>) at eval.c:3040
#63 0x01183644 in Ffuncall (nargs=4, args=0x8888f4) at eval.c:2855
#64 0x011c4bc3 in exec_byte_code (bytestr=..., vector=..., maxdepth=...,
    args_template=..., nargs=0, args=0x0) at bytecode.c:919
#65 0x01183fc8 in funcall_lambda (fun=..., nargs=3, arg_vector=0x51d1bfd)
    at eval.c:3040
#66 0x01183644 in Ffuncall (nargs=4, args=0x888c34) at eval.c:2855
#67 0x011c4bc3 in exec_byte_code (bytestr=..., vector=..., maxdepth=...,
    args_template=..., nargs=0, args=0x0) at bytecode.c:919
#68 0x01183fc8 in funcall_lambda (fun=..., nargs=2,
    arg_vector=0x1317155 <pure+649077>) at eval.c:3040
#69 0x01183644 in Ffuncall (nargs=3, args=0x889098) at eval.c:2855
#70 0x011829aa in funcall_nil (nargs=3, args=0x889098) at eval.c:2357
#71 0x01182dbf in run_hook_with_args (nargs=3, args=0x889098,
    funcall=0x1182992 <funcall_nil>) at eval.c:2542
#72 0x01182a21 in Frun_hook_with_args (nargs=3, args=0x889098) at eval.c:2403
#73 0x0118330a in Ffuncall (nargs=4, args=0x889094) at eval.c:2787
#74 0x011c4bc3 in exec_byte_code (bytestr=..., vector=..., maxdepth=...,
    args_template=..., nargs=0, args=0x8893e4) at bytecode.c:919
#75 0x01183bf2 in funcall_lambda (fun=..., nargs=0, arg_vector=0x8893e4)
    at eval.c:2974
#76 0x01183644 in Ffuncall (nargs=1, args=0x8893e0) at eval.c:2855
#77 0x01181fe4 in eval_sub (form=...) at eval.c:2148
#78 0x0118025e in internal_lisp_condition_case (var=..., bodyform=...,
    handlers=...) at eval.c:1314
#79 0x011c5af4 in exec_byte_code (bytestr=..., vector=..., maxdepth=...,
    args_template=..., nargs=2, args=0x8898c4) at bytecode.c:1169
#80 0x01183bf2 in funcall_lambda (fun=..., nargs=2, arg_vector=0x8898bc)
    at eval.c:2974
#81 0x01183644 in Ffuncall (nargs=3, args=0x8898b8) at eval.c:2855
#82 0x011c4bc3 in exec_byte_code (bytestr=..., vector=..., maxdepth=...,
    args_template=..., nargs=1, args=0x889c58) at bytecode.c:919
#83 0x01183bf2 in funcall_lambda (fun=..., nargs=1, arg_vector=0x889c54)
    at eval.c:2974
#84 0x01183644 in Ffuncall (nargs=2, args=0x889c50) at eval.c:2855
#85 0x011806bb in internal_condition_case_n (bfun=0x11830e4 <Ffuncall>,
    nargs=2, args=0x889c50, handlers=..., hfun=0x1026dfa <safe_eval_handler>)
    at eval.c:1427
#86 0x01026f19 in safe_call (nargs=2, func=...) at xdisp.c:2563
#87 0x01026f56 in safe_call1 (fn=..., arg=...) at xdisp.c:2579
#88 0x0102a1d5 in handle_fontified_prop (it=0x88d2fc) at xdisp.c:3756
#89 0x010293a3 in handle_stop (it=0x88d2fc) at xdisp.c:3320
#90 0x010359e6 in next_element_from_buffer (it=0x88d2fc) at xdisp.c:8077
#91 0x010325b4 in get_next_display_element (it=0x88d2fc) at xdisp.c:6732
#92 0x010364d4 in move_it_in_display_line_to (it=0x88d2fc, to_charpos=949978,
    to_x=0, op=(MOVE_TO_X | MOVE_TO_POS)) at xdisp.c:8412
#93 0x01038443 in move_it_to (it=0x88d2fc, to_charpos=949978, to_x=0,
    to_y=1600, to_vpos=-1, op=11) at xdisp.c:8936
#94 0x0104798f in try_scrolling (window=..., just_this_one_p=0,
    arg_scroll_conservatively=101, scroll_step=0, temp_scroll_step=0,
    last_line_misfit=0) at xdisp.c:15016
#95 0x0104bb7b in redisplay_window (window=..., just_this_one_p=false)
    at xdisp.c:16119
#96 0x01044bf1 in redisplay_window_0 (window=...) at xdisp.c:14056
#97 0x01180479 in internal_condition_case_1 (
    bfun=0x1044bbb <redisplay_window_0>, arg=..., handlers=...,
    hfun=0x1044b97 <redisplay_window_error>) at eval.c:1369
#98 0x01044b7c in redisplay_windows (window=...) at xdisp.c:14036
#99 0x01043b49 in redisplay_internal () at xdisp.c:13635
#100 0x0103ca92 in resize_echo_area_exactly () at xdisp.c:10559
#101 0x010f622b in command_loop_1 () at keyboard.c:1571
#102 0x01180366 in internal_condition_case (bfun=0x10f5b5b <command_loop_1>,
    handlers=..., hfun=0x10f53c7 <cmd_error>) at eval.c:1345
#103 0x010f5811 in command_loop_2 (ignore=...) at keyboard.c:1170
#104 0x0117f913 in internal_catch (tag=..., func=0x10f57ed <command_loop_2>,
    arg=...) at eval.c:1109
#105 0x010f57c9 in command_loop () at keyboard.c:1149
#106 0x010f4f63 in recursive_edit_1 () at keyboard.c:777
#107 0x010f5120 in Frecursive_edit () at keyboard.c:841
#108 0x010f32ff in main (argc=2, argv=0xe51ff8) at emacs.c:1643

Lisp Backtrace:
"goto-char" (0x886820)
"progn" (0x88696c)
"let*" (0x886a8c)
"save-restriction" (0x886bac)
"save-excursion" (0x886ccc)
"c-append-lower-brace-pair-to-state-cache" (0x886db0)
"if" (0x886fac)
"cond" (0x8870dc)
"let*" (0x88722c)
"c-parse-state-1" (0x887310)
"progn" (0x8874fc)
"unwind-protect" (0x8875ec)
"let*" (0x88773c)
"unwind-protect" (0x88782c)
"if" (0x88794c)
"progn" (0x887a3c)
"unwind-protect" (0x887b2c)
"prog1" (0x887c2c)
"let" (0x887dbc)
"c-parse-state" (0x887f28)
"c-font-lock-complex-decl-prepare" (0x888268)
"font-lock-fontify-keywords-region" (0x8885b8)
"font-lock-default-fontify-region" (0x8888f8)
"c-font-lock-fontify-region" (0x888c38)
"font-lock-fontify-region" (0x88909c)
"run-hook-with-args" (0x889098)
0x37cc9b0 PVEC_COMPILED
"funcall" (0x8893e0)
"jit-lock-fontify-now" (0x8898bc)
"jit-lock-function" (0x889c54)
"redisplay_internal (C function)" (0x153de1c)

bug#16526: 24.3.50; scroll-conservatively & c-mode regression. The purpose of revision 116070.

[Eli Zaretskii]

> Date: Sat, 25 Jan 2014 20:27:21 +0000
> Cc: 16526@debbugs.gnu.org
> From: Alan Mackenzie <acm@muc.de>
> 
> If I've understood correctly, I think the root cause of the slowness is
> the lack of optimisation of find_defun_start when open-paren-etc. is nil
> - it brutally returns BOB, no matter what.

FWIW, I don't think this is the culprit, at least not directly.  Why
does CC Mode fontification code decide to scan the buffer backwards
from EOB, when point is at BOB, is a much more relevant question, so
it seems.

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[Alan Mackenzie]

Hi, Eli.

On Sun, Jan 26, 2014 at 07:20:27PM +0200, Eli Zaretskii wrote:

> You are right, sorry.  (I wasn't wrong, either: recenter does call the
> same find_defun_start around EOB, which is what I saw.  But those
> calls are very few and aren't responsible for the slowdown.  I also
> wasn't wrong about point being at EOB, see below.  But I described
> what happens incorrectly.)

> Here's what I see in the debugger:

> After beginning-of-buffer jumps to point-min, redisplay kicks in.
> Since scroll-conservatively is set to a large value, redisplay first
> tries to see whether it can bring point into view by scrolling the
> window as little as possible.  It calls try_scrolling, which at some
> point (around line 15000) tries to see whether the new location of
> point is close enough to the current window start.  It does so by
> calling move_it_to, which simulates the display.  While doing so,
> move_it_to hits a portion of text with font-lock properties, and calls
> JIT Lock to fontify them.

> And here's where things go awry: For some reason, the CC Mode
> fontification code decides it needs to scan the buffer backwards,
> starting from EOB.

The @dfn{state cache}, a list of certain brace/paren/bracket positions
around some position, is set for a position near EOB.  With the switch to
a different position, CC Mode tweaks the state cache rather than
calculating it anew starting at BOB.  When the new position is nearer
BOB, the code searches backwards for the appropriate braces.  However, it
shouldn't be scanning the entire buffer backwards.  There is clearly a
bug here.

> So it goes temporarily to EOB (this is why I saw point being there),
> and scans all the way back, I think in this loop from
> c-append-lower-brace-pair-to-state-cache, which is called with its
> first argument FROM set to EOB:

> 	  ;; In the next pair of nested loops, the inner one moves back past a
> 	  ;; pair of (mis-)matching parens or brackets; the outer one moves
> 	  ;; back over a sequence of unmatched close brace/paren/bracket each
> 	  ;; time round.
> 	  (while
> 	      (progn
> 		(c-safe
> 		  (while
> 		      (and (setq ce (scan-lists bra -1 -1)) ; back past )/]/}; might signal
> 			   (setq bra (scan-lists ce -1 1)) ; back past (/[/{; might signal
> 			   (or (> bra here) ;(> ce here)
> 			       (and
> 				(< ce here)
> 				(or (not (eq (char-after bra) ?\{))
> 				    (and (goto-char bra)
> 					 (c-beginning-of-macro)
> 					 (< (point) macro-start-or-from))))))))
> 		(and ce (< ce bra)))
> 	    (setq bra ce))	; If we just backed over an unbalanced closing
> 					; brace, ignore it.

The backward scan-lists calls will be causing continual forward searches
from BOB in syntax.c, every time the backward scan hits a comment ender.

> This loop takes a lot of time, of course, and is a waste of time,
> since eventually try_scrolling comes to the correct conclusion that
> scrolling is impossible, and instead recenters at BOB.

> Why does CC Mode decide to go from EOB backwards, I don't know;
> presumably, this is decided by c-parse-state-get-strategy as part of
> c-parse-state-1.

Yes.  There is a bug here.  I have a strong suspicion where.

[ .... ]

> I hope this information will allow Alan to find the culprit and solve
> the problem.

Yes indeed, thanks.  But I'm not going to be able to resolve it in a
scale of hours.  It's going to be days.  Sorry!

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[martin rudalics]

 > After beginning-of-buffer jumps to point-min, redisplay kicks in.
 > Since scroll-conservatively is set to a large value, redisplay first
 > tries to see whether it can bring point into view by scrolling the
 > window as little as possible.  It calls try_scrolling, which at some
 > point (around line 15000) tries to see whether the new location of
 > point is close enough to the current window start.  It does so by
 > calling move_it_to, which simulates the display.  While doing so,
 > move_it_to hits a portion of text with font-lock properties, and calls
 > JIT Lock to fontify them.

Well... try_scrolling could detect that `point' is some 15000 lines away
from the current window start so trying to scroll the window as little
as possible might not be worth the effort.

 > And here's where things go awry: For some reason, the CC Mode
 > fontification code decides it needs to scan the buffer backwards,
 > starting from EOB.  So it goes temporarily to EOB (this is why I saw
 > point being there), and scans all the way back, I think in this loop
 > from c-append-lower-brace-pair-to-state-cache, which is called with
 > its first argument FROM set to EOB:

But it's redisplay which temporarily puts `point' at EOB and triggers
the fontification subsystem to "work" at that position?

 > This loop takes a lot of time, of course, and is a waste of time,
 > since eventually try_scrolling comes to the correct conclusion that
 > scrolling is impossible, and instead recenters at BOB.

Are you sure that try_scrolling doesn't call this loop over and over
again?

 > Why does CC Mode decide to go from EOB backwards, I don't know;
 > presumably, this is decided by c-parse-state-get-strategy as part of
 > c-parse-state-1.

This seems obvious.  To decide whether code shall be fontified this way
or another it has to decide whether the code is part of a comment and
find that comment's start.  As long as it is not aware of the fact that
`point' is already at BOB, obviously.

martin

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[martin rudalics]

 > The @dfn{state cache}, a list of certain brace/paren/bracket positions
 > around some position, is set for a position near EOB.  With the switch to
 > a different position, CC Mode tweaks the state cache rather than
 > calculating it anew starting at BOB.  When the new position is nearer
 > BOB, the code searches backwards for the appropriate braces.  However, it
 > shouldn't be scanning the entire buffer backwards.  There is clearly a
 > bug here.

In my scenario `end-of-buffer' should already have produced the complete
state cache.  How else would you have been able to fontify code near EOB
correctly?

 > The backward scan-lists calls will be causing continual forward searches
 > from BOB in syntax.c, every time the backward scan hits a comment ender.

IIUC now any call to `forward-comment' with a negative argument will
automatically go to BOB unless it's already there.  How else could it
determine that it's not called from a position within a comment?  So if
you decide to bind `open-paren-in-column-0-is-defun-start' to nil, then
why don't you build the state cache anew from BOB?

In any case, please provide an option say
`c-open-paren-in-column-0-is-defun-start' which people can set to avoid
those scans.  The doc-string should mention that things like Michael's
commented out code are handled correctly iff this option is nil and that
long delays while working with c-mode can be sometimes avoided by
setting this option to t.  The default value could be obviously nil.

martin

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[Stefan Monnier]

> IIUC now any call to `forward-comment' with a negative argument will
> automatically go to BOB unless it's already there.

Actually, that should not always be the case, no.

> How else could it determine that it's not called from a position
> within a comment?

In general, that is true, but back_comment tries to be more clever, e.g,
when it scans backward over code like

   /* toto */
   blabla
   /* titi */

it should not need to scan further than the end of line 1, since the
"*/" there will tell it that the "/*" on line 3 can't be within
another comment and is hence the one-true comment starter that matches
the "*/" on line 3.

Of course, reality is more complex because there are also strings and
//...\n comments, but at least back_comment tries to avoid scanning from
BOB when it can.  I think it even tries hard enough to consciously
prefer performance over correctness, so that it will incorrectly handle

   // hello /* there
   bogus code */

and will jump to "/*" thinking it jumped over a comment.

> In any case, please provide an option say
> `c-open-paren-in-column-0-is-defun-start' which people can set to avoid
> those scans.  The doc-string should mention that things like Michael's
> commented out code are handled correctly iff this option is nil and that
> long delays while working with c-mode can be sometimes avoided by
> setting this option to t.  The default value could be obviously nil.

Another option is to put a syntax-table property on those rare
open-paren-in-column-0-inside-comment-or-string, so that
open-paren-in-column-0-is-defun-start still works properly.


        Stefan


PS: And yes, back_comment should somehow make use of syntax-ppss-cache to
avoid scanning from BOB.

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[Eli Zaretskii]

> Date: Mon, 27 Jan 2014 09:20:57 +0100
> From: martin rudalics <rudalics@gmx.at>
> CC: acm@muc.de, 16526@debbugs.gnu.org
> 
> Well... try_scrolling could detect that `point' is some 15000 lines away
> from the current window start so trying to scroll the window as little
> as possible might not be worth the effort.

How would it detect that, except using the same method it uses now?
Note that we are not talking physical lines in the buffer here, we are
talking screen lines.  Actually, not even that: we are talking
_canonical_ screen lines, i.e. practically pixels.  Now, those 15000
lines could well be covered by an invisible property, or by a display
property that displays something other than buffer text.  Or they
could use a face that calls for a very small font, so that all the
15000 lines take very little screen estate.  In all of these cases, a
user who sets scroll-conservatively to 101 wants the screen scrolled
rather than recentered.  So that's what try_scrolling tries to do.
It's why that function exists.

However, try_scrolling's search is limited, so it normally fails very
quickly.  Except that in this case, the code invoked by JIT Lock hogs
the CPU for a very long time.

>  > And here's where things go awry: For some reason, the CC Mode
>  > fontification code decides it needs to scan the buffer backwards,
>  > starting from EOB.  So it goes temporarily to EOB (this is why I saw
>  > point being there), and scans all the way back, I think in this loop
>  > from c-append-lower-brace-pair-to-state-cache, which is called with
>  > its first argument FROM set to EOB:
> 
> But it's redisplay which temporarily puts `point' at EOB and triggers
> the fontification subsystem to "work" at that position?

No, redisplay doesn't change point, not ever.  It's cc-engine's
routines that do it in this case, and I've shown where in the code
that happens, see the backtrace I posted.

>  > This loop takes a lot of time, of course, and is a waste of time,
>  > since eventually try_scrolling comes to the correct conclusion that
>  > scrolling is impossible, and instead recenters at BOB.
> 
> Are you sure that try_scrolling doesn't call this loop over and over
> again?

Sorry, I don't understand the question.  try_scrolling doesn't include
any loops where all this happens, it just calls move_it_to, a single
call.  All the rest happens inside that call.  Normally, that call
should just descend a few screen lines starting at point (which is at
BOB), until it hits the limit I mentioned above, and then
try_scrolling should return a failure indication.

I did verify that there's a single call to move_it_to, and that all
the time you wait is spent inside that call.

>  > Why does CC Mode decide to go from EOB backwards, I don't know;
>  > presumably, this is decided by c-parse-state-get-strategy as part of
>  > c-parse-state-1.
> 
> This seems obvious.  To decide whether code shall be fontified this way
> or another it has to decide whether the code is part of a comment and
> find that comment's start.  As long as it is not aware of the fact that
> `point' is already at BOB, obviously.

Why shouldn't CC Mode be aware that point is at BOB?  It has just to
look at its value.

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[martin rudalics]

 > In general, that is true, but back_comment tries to be more clever, e.g,
 > when it scans backward over code like
 >
 >    /* toto */
 >    blabla
 >    /* titi */
 >
 > it should not need to scan further than the end of line 1, since the
 > "*/" there will tell it that the "/*" on line 3 can't be within
 > another comment and is hence the one-true comment starter that matches
 > the "*/" on line 3.

I recall now.  I tend to forget that C comments cannot be nested.

 > Another option is to put a syntax-table property on those rare
 > open-paren-in-column-0-inside-comment-or-string, so that
 > open-paren-in-column-0-is-defun-start still works properly.

Wouldn't we then have to parse the entire buffer for strings and
comments in order to detect these parens?  More or less this would
duplicate the effort of `syntax-ppss'.

martin

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[martin rudalics]

 > How would it detect that, except using the same method it uses now?
 > Note that we are not talking physical lines in the buffer here, we are
 > talking screen lines.  Actually, not even that: we are talking
 > _canonical_ screen lines, i.e. practically pixels.  Now, those 15000
 > lines could well be covered by an invisible property, or by a display
 > property that displays something other than buffer text.  Or they
 > could use a face that calls for a very small font, so that all the
 > 15000 lines take very little screen estate.  In all of these cases, a
 > user who sets scroll-conservatively to 101 wants the screen scrolled
 > rather than recentered.  So that's what try_scrolling tries to do.
 > It's why that function exists.

At least now I understand why setting `scroll-conservatively' to 101 can
be expensive.

 >> But it's redisplay which temporarily puts `point' at EOB and triggers
 >> the fontification subsystem to "work" at that position?
 >
 > No, redisplay doesn't change point, not ever.  It's cc-engine's
 > routines that do it in this case, and I've shown where in the code
 > that happens, see the backtrace I posted.

I see.  Fontification routines just get the boundaries they have to
operate on.  The backtrace is difficult to interpret - I never know
which of Alan's macros save excursion.  But since it returns a buffer
position it likely that it reuses that position later on.

 >> Are you sure that try_scrolling doesn't call this loop over and over
 >> again?
 >
 > Sorry, I don't understand the question.  try_scrolling doesn't include
 > any loops where all this happens, it just calls move_it_to, a single
 > call.  All the rest happens inside that call.  Normally, that call
 > should just descend a few screen lines starting at point (which is at
 > BOB), until it hits the limit I mentioned above, and then
 > try_scrolling should return a failure indication.
 >
 > I did verify that there's a single call to move_it_to, and that all
 > the time you wait is spent inside that call.

I just wanted to make sure that there's no loop in try_scrolling.

 > Why shouldn't CC Mode be aware that point is at BOB?  It has just to
 > look at its value.

It shouldn't matter anyway since, as you said above, the display engine
never sets point.

Thanks for the clarifications.  My questions were naive because I never
looked into how scrolling works and moreover do not understand c-mode
macros well.

martin

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[Stefan Monnier]

>> Another option is to put a syntax-table property on those rare
>> open-paren-in-column-0-inside-comment-or-string, so that
>> open-paren-in-column-0-is-defun-start still works properly.
> Wouldn't we then have to parse the entire buffer for strings and
> comments in order to detect these parens?  More or less this would
> duplicate the effort of `syntax-ppss'.

IIUC CC-mode does do such scanning to place syntax-table properties on
various elements (it should do it from syntax-propertize-function, but
for various reasons it does it at various other places instead, in
piecemeal fashion).  So it would just be adding one more element to
find and mark.


        Stefan

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[Alan Mackenzie]

Hello, Eli, hello, Martin.

On Sun, Jan 26, 2014 at 08:43:10PM +0000, Alan Mackenzie wrote:
> On Sun, Jan 26, 2014 at 07:20:27PM +0200, Eli Zaretskii wrote:

> > You are right, sorry.  (I wasn't wrong, either: recenter does call the
> > same find_defun_start around EOB, which is what I saw.  But those
> > calls are very few and aren't responsible for the slowdown.  I also
> > wasn't wrong about point being at EOB, see below.  But I described
> > what happens incorrectly.)

> > Here's what I see in the debugger:

> > After beginning-of-buffer jumps to point-min, redisplay kicks in.
> > Since scroll-conservatively is set to a large value, redisplay first
> > tries to see whether it can bring point into view by scrolling the
> > window as little as possible.  It calls try_scrolling, which at some
> > point (around line 15000) tries to see whether the new location of
> > point is close enough to the current window start.  It does so by
> > calling move_it_to, which simulates the display.  While doing so,
> > move_it_to hits a portion of text with font-lock properties, and calls
> > JIT Lock to fontify them.

> > And here's where things go awry: For some reason, the CC Mode
> > fontification code decides it needs to scan the buffer backwards,
> > starting from EOB.

> The @dfn{state cache}, a list of certain brace/paren/bracket positions
> around some position, is set for a position near EOB.  With the switch to
> a different position, CC Mode tweaks the state cache rather than
> calculating it anew starting at BOB.  When the new position is nearer
> BOB, the code searches backwards for the appropriate braces.  However, it
> shouldn't be scanning the entire buffer backwards.  There is clearly a
> bug here.

> > So it goes temporarily to EOB (this is why I saw point being there),
> > and scans all the way back, I think in this loop from
> > c-append-lower-brace-pair-to-state-cache, which is called with its
> > first argument FROM set to EOB:

> > 	  ;; In the next pair of nested loops, the inner one moves back past a
> > 	  ;; pair of (mis-)matching parens or brackets; the outer one moves
> > 	  ;; back over a sequence of unmatched close brace/paren/bracket each
> > 	  ;; time round.
> > 	  (while
> > 	      (progn
> > 		(c-safe
> > 		  (while
> > 		      (and (setq ce (scan-lists bra -1 -1)) ; back past )/]/}; might signal
> > 			   (setq bra (scan-lists ce -1 1)) ; back past (/[/{; might signal
> > 			   (or (> bra here) ;(> ce here)
> > 			       (and
> > 				(< ce here)
> > 				(or (not (eq (char-after bra) ?\{))
> > 				    (and (goto-char bra)
> > 					 (c-beginning-of-macro)
> > 					 (< (point) macro-start-or-from))))))))
> > 		(and ce (< ce bra)))
> > 	    (setq bra ce))	; If we just backed over an unbalanced closing
> > 					; brace, ignore it.

> The backward scan-lists calls will be causing continual forward searches
> from BOB in syntax.c, every time the backward scan hits a comment ender.

> > This loop takes a lot of time, of course, and is a waste of time,
> > since eventually try_scrolling comes to the correct conclusion that
> > scrolling is impossible, and instead recenters at BOB.

> > Why does CC Mode decide to go from EOB backwards, I don't know;
> > presumably, this is decided by c-parse-state-get-strategy as part of
> > c-parse-state-1.

> Yes.  There is a bug here.  I have a strong suspicion where.

> [ .... ]

> > I hope this information will allow Alan to find the culprit and solve
> > the problem.

> Yes indeed, thanks.  But I'm not going to be able to resolve it in a
> scale of hours.  It's going to be days.  Sorry!

OK, here is a rough patch (smooth version to follow if it's any good),
which attempts to solve the problem by not calling
c-append-lower-brace-pair-to-state-cache in the pertinent circumstances.
Please try it out and let me know if it solves the problem (I still can't
reproduce the massive slowdown myself).



diff -r d6064f65b0a1 cc-engine.el
--- a/cc-engine.el	Sun Jan 19 12:09:59 2014 +0000
+++ b/cc-engine.el	Wed Jan 29 21:17:50 2014 +0000
@@ -2556,6 +2556,8 @@
   ;; o - ('forward START-POINT) - scan forward from START-POINT,
   ;;	 which is not less than the highest position in `c-state-cache' below here.
   ;; o - ('backward nil) - scan backwards (from HERE).
+  ;; o - ('back-and-forward START-POINT) - like 'forward, but when HERE is earlier
+  ;;     than GOOD-POS.
   ;; o - ('IN-LIT nil) - point is inside the literal containing point-min.
   (let ((cache-pos (c-get-cache-scan-pos here))	; highest position below HERE in cache (or 1)
 	strategy	    ; 'forward, 'backward, or 'IN-LIT.
@@ -2570,9 +2572,10 @@
      ((< (- good-pos here) (- here cache-pos)) ; FIXME!!! ; apply some sort of weighting.
       (setq strategy 'backward))
      (t
-      (setq strategy 'forward
+      (setq strategy 'back-and-forward
 	    start-point cache-pos)))
-    (list strategy (and (eq strategy 'forward) start-point))))
+    (list strategy ;(and (eq strategy 'forward)
+	  start-point)));)
 
 
 ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
@@ -2857,7 +2860,7 @@
   ;; adjust it to get outside a string/comment.	 (Sorry about this!  The code
   ;; needs to be FAST).
   ;;
-  ;; Return a list (GOOD-POS SCAN-BACK-POS PPS-STATE), where
+  ;; Return a list (GOOD-POS SCAN-BACK-POS PPS-STATE), where  FIXME!!!
   ;; o - GOOD-POS is a position where the new value `c-state-cache' is known
   ;;   to be good (we aim for this to be as high as possible);
   ;; o - SCAN-BACK-POS, if not nil, indicates there may be a brace pair
@@ -2892,6 +2895,7 @@
 	     pos
 	     upper-lim	   ; ,beyond which `c-state-cache' entries are removed
 	     scan-back-pos
+	     cons-separated
 	     pair-beg pps-point-state target-depth)
 
 	;; Remove entries beyond HERE.  Also remove any entries inside
@@ -2913,7 +2917,8 @@
 		   (consp (car c-state-cache))
 		   (> (cdar c-state-cache) upper-lim))
 	  (setcar c-state-cache (caar c-state-cache))
-	  (setq scan-back-pos (car c-state-cache)))
+	  (setq scan-back-pos (car c-state-cache)
+		cons-separated t))
 
 	;; The next loop jumps forward out of a nested level of parens each
 	;; time round; the corresponding elements in `c-state-cache' are
@@ -2986,7 +2991,8 @@
 	  (setq c-state-cache (cons (cons pair-beg pos)
 				    c-state-cache)))
 
-	(list pos scan-back-pos pps-state)))))
+	;(when (< pos scan-back-pos) (setq scan-back-pos nil)) ; 2014-01-26  FIXME!!!
+	(list pos scan-back-pos pps-state cons-separated)))))
 
 (defun c-remove-stale-state-cache-backwards (here)
   ;; Strip stale elements of `c-state-cache' by moving backwards through the
@@ -3271,6 +3277,7 @@
 		     ; are no open parens/braces between it and HERE.
 	 bopl-state
 	 res
+	 cons-separated
 	 scan-backward-pos scan-forward-p) ; used for 'backward.
     ;; If POINT-MIN has changed, adjust the cache
     (unless (= (point-min) c-state-point-min)
@@ -3283,13 +3290,15 @@
 
     ;; SCAN!
     (cond
-     ((eq strategy 'forward)
+     ((memq strategy '(forward back-and-forward)) ;(eq strategy 'forward)
       (setq res (c-remove-stale-state-cache start-point here here-bopl))
       (setq cache-pos (car res)
 	    scan-backward-pos (cadr res)
-	    bopl-state (car (cddr res))) ; will be nil if (< here-bopl
+	    bopl-state (car (cddr res)) ; will be nil if (< here-bopl
 					; start-point)
-      (if scan-backward-pos
+	    cons-separated (car (cdddr res)))
+      (if (and scan-backward-pos
+	       (or cons-separated (eq strategy 'forward))) ;scan-backward-pos
 	  (c-append-lower-brace-pair-to-state-cache scan-backward-pos here))
       (setq good-pos
 	    (c-append-to-state-cache cache-pos here))


-- 
Alan Mackenzie (Nuremberg, Germany).

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[Alan Mackenzie]

Hi, Martin.

On Mon, Jan 27, 2014 at 09:21:24AM +0100, martin rudalics wrote:
>  > The @dfn{state cache}, a list of certain brace/paren/bracket positions
>  > around some position, is set for a position near EOB.  With the switch to
>  > a different position, CC Mode tweaks the state cache rather than
>  > calculating it anew starting at BOB.  When the new position is nearer
>  > BOB, the code searches backwards for the appropriate braces.  However, it
>  > shouldn't be scanning the entire buffer backwards.  There is clearly a
>  > bug here.

> In my scenario `end-of-buffer' should already have produced the complete
> state cache.  How else would you have been able to fontify code near EOB
> correctly?

The state cache contains not only a list of enclosing
braces/brackets/parentheses but also any brace pair immediately
preceding one of these things.  So a typical state cache looks like:

    (4532 (4284 . 4526) 4278 (4131 . 4248))

(with a detailed explanation in cc-engine.el in the comment for
`c-parse-state-1').

When c-parse-state is called at a position somewhat distant from the
previous call, it modifies the state cache by crawling through the
buffer rather than recalculating it from scratch.

>  > The backward scan-lists calls will be causing continual forward searches
>  > from BOB in syntax.c, every time the backward scan hits a comment ender.

> IIUC now any call to `forward-comment' with a negative argument will
> automatically go to BOB unless it's already there.  How else could it
> determine that it's not called from a position within a comment?

I don't think this happens.  back_comment (in syntax.c) assumes it
starts outside a comment.

> So if you decide to bind `open-paren-in-column-0-is-defun-start' to
> nil, then why don't you build the state cache anew from BOB?
 
The state cache has different values for different buffer positions.

> In any case, please provide an option say
> `c-open-paren-in-column-0-is-defun-start' which people can set to
> avoid those scans.  The doc-string should mention that things like
> Michael's commented out code are handled correctly iff this option is
> nil and that long delays while working with c-mode can be sometimes
> avoided by setting this option to t.  The default value could be
> obviously nil.
 
I'm hoping that the (rough) patch in one of my other emails from this
evening will have solved the bug, so that playing around with
open-paren-in-column-0-etc. won't be needed.

As a matter of interest, with my patch applied, running your recipe on
xdisp.c took me 4.4 seconds (including the (sit-for 3)).

> martin

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[martin rudalics]

 > I don't think this happens.  back_comment (in syntax.c) assumes it
 > starts outside a comment.

Does it?

 >> In any case, please provide an option say
 >> `c-open-paren-in-column-0-is-defun-start' which people can set to
 >> avoid those scans.  The doc-string should mention that things like
 >> Michael's commented out code are handled correctly iff this option is
 >> nil and that long delays while working with c-mode can be sometimes
 >> avoided by setting this option to t.  The default value could be
 >> obviously nil.
 >
 > I'm hoping that the (rough) patch in one of my other emails from this
 > evening will have solved the bug, so that playing around with
 > open-paren-in-column-0-etc. won't be needed.
 >
 > As a matter of interest, with my patch applied, running your recipe on
 > xdisp.c took me 4.4 seconds (including the (sit-for 3)).

Less than 8 seconds here which is quite remarkable.  Unfortunately, I
seem to have another problem now which makes `beginning-of-buffer' in
xdisp.c extremely slow with my setup (~ 30 secs).  Have to bisect my
.emacs again ...

Thanks so far, martin

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[martin rudalics]

Weird.  With your patch on Windows I put into my .emacs these two lines

(find-file (concat source-directory "src/xdisp.c"))
(setq initial-frame-alist '((fullscreen . maximized)))

and then switch to buffer xdisp.c.  If I _now_ type <C-end> it takes at
least 20 seconds.  Can anyone reproduce that?

With a non-maximized frame there's no such problem.  And on Debian Gtk
<C-end> takes less than 8 seconds here.

martin

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[Eli Zaretskii]

> Date: Thu, 30 Jan 2014 14:47:02 +0100
> From: martin rudalics <rudalics@gmx.at>
> Cc: 16526@debbugs.gnu.org
> 
> Weird.  With your patch on Windows I put into my .emacs these two lines
> 
> (find-file (concat source-directory "src/xdisp.c"))
> (setq initial-frame-alist '((fullscreen . maximized)))
> 
> and then switch to buffer xdisp.c.  If I _now_ type <C-end> it takes at
> least 20 seconds.  Can anyone reproduce that?

It takes about 11 sec here.  But reverting the patch doesn't change
this timing, I still get 11 sec.

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[Eli Zaretskii]

> Date: Wed, 29 Jan 2014 21:52:40 +0000
> Cc: 16526@debbugs.gnu.org
> From: Alan Mackenzie <acm@muc.de>
> 
> OK, here is a rough patch (smooth version to follow if it's any good),
> which attempts to solve the problem by not calling
> c-append-lower-brace-pair-to-state-cache in the pertinent circumstances.
> Please try it out and let me know if it solves the problem

It takes about 3 sec here, so I think it does solve the problem.

> (I still can't reproduce the massive slowdown myself).

Did you try to configure like I've shown and rebuild?

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[martin rudalics]

 > It takes about 11 sec here.  But reverting the patch doesn't change
 > this timing, I still get 11 sec.

But it does take longer with a maximized window?  If so, do you have an
explanation?

martin

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[Alan Mackenzie]

Hi, Eli.

On Thu, Jan 30, 2014 at 07:16:27PM +0200, Eli Zaretskii wrote:
> > Date: Wed, 29 Jan 2014 21:52:40 +0000
> > Cc: 16526@debbugs.gnu.org
> > From: Alan Mackenzie <acm@muc.de>

> > OK, here is a rough patch (smooth version to follow if it's any good),
> > which attempts to solve the problem by not calling
> > c-append-lower-brace-pair-to-state-cache in the pertinent circumstances.
> > Please try it out and let me know if it solves the problem

> It takes about 3 sec here, so I think it does solve the problem.

> > (I still can't reproduce the massive slowdown myself).

> Did you try to configure like I've shown and rebuild?

Sorry, no I hadn't.  I wasn't paying enough attention when I read that
post of yours.

I've just tried it now, and with the unoptimised and instrumented
version just created, using the unpatched cc-engine.elc, Martin's recipe
takes me ~70 seconds.  With my patch, it takes ~13 seconds.

I think you commented somewhere that it would be a good idea to test on
such a build "any change which might affect redisplay".  That might be
so, but how should one know which changes might do this?  Binding
open-paren-in-column-0-etc. to nil wouldn't have raised any alarm bells
in this regard, since beginning-of-defun doesn't seem much connected
with redisplay.

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[Eli Zaretskii]

> Date: Fri, 31 Jan 2014 20:00:01 +0000
> Cc: rudalics@gmx.at, 16526@debbugs.gnu.org
> From: Alan Mackenzie <acm@muc.de>
> 
> I think you commented somewhere that it would be a good idea to test on
> such a build "any change which might affect redisplay".  That might be
> so, but how should one know which changes might do this?

In your case, it's simple: any changes that affect font-lock,
automatically affect redisplay, via JIT Lock.  So I suggest always to
do your testing of such changes in an unoptimized build.  That is,
when you finish development and start testing, do it in an unoptimized
build.

Thanks.

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[Eli Zaretskii]

> Date: Thu, 30 Jan 2014 19:58:17 +0100
> From: martin rudalics <rudalics@gmx.at>
> CC: acm@muc.de, 16526@debbugs.gnu.org
> 
>  > It takes about 11 sec here.  But reverting the patch doesn't change
>  > this timing, I still get 11 sec.
> 
> But it does take longer with a maximized window?

Yes, it does.

> If so, do you have an explanation?

Those 11 seconds are spent inside 'recenter', which is called by
end-of-buffer:

  ;; If we went to a place in the middle of the buffer,
  ;; adjust it to the beginning of a line.
  (cond ((and arg (not (consp arg))) (forward-line 1))
	((and (eq (current-buffer) (window-buffer))
              (> (point) (window-end nil t)))
	 ;; If the end of the buffer is not already on the screen,
	 ;; then scroll specially to put it near, but not at, the bottom.
	 (overlay-recenter (point))
	 (recenter -3))))  <<<<<<<<<<<<<<<<<<<<<<<<<<

Stepping through 'recenter', I see the following:

  . It treats GUI frames specially (and indeed, in "emacs -nw", I
    don't see this slowdown).  The special handling is that it
    attempts to find the window-start point that corresponds to the -3
    argument, by interpreting that argument as the number of pixels
    equivalent to 3 canonical-height lines.

  . To this end, 'recenter' calls move_it_vertically_backward with the
    -3 argument converted to pixels.  move_it_vertically_backward then
    tries to find that place, by simulating display.

  . As part of the display simulation, jit-lock-function is called
    (because we hit a buffer position which has a non-nil 'fontified'
    property) with a single argument: buffer position 948757.  This
    single call takes almost all of the 11 seconds.

When the frame is not maximized, the window height is smaller, so
'recenter' moves back a smaller amount of pixels, and calls
jit-lock-function at a different buffer position.  That call takes
only about 2 sec (still quite a lot, IMO).

Perhaps Alan could explain why the CC Mode fontification takes such a
long time for buffer position 948757.

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[martin rudalics]

 > When the frame is not maximized, the window height is smaller, so
 > 'recenter' moves back a smaller amount of pixels, and calls
 > jit-lock-function at a different buffer position.  That call takes
 > only about 2 sec (still quite a lot, IMO).

Thanks for the explanation.  It would never have occurred to me that
different window starts could cause such a difference in execution time.

martin

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[Alan Mackenzie]

Hello, Eli.

On Sat, Feb 01, 2014 at 12:41:51PM +0200, Eli Zaretskii wrote:
> > Date: Thu, 30 Jan 2014 19:58:17 +0100
> > From: martin rudalics <rudalics@gmx.at>
> > CC: acm@muc.de, 16526@debbugs.gnu.org

> >  > It takes about 11 sec here.  But reverting the patch doesn't change
> >  > this timing, I still get 11 sec.

> > But it does take longer with a maximized window?

> Yes, it does.

> > If so, do you have an explanation?

> Those 11 seconds are spent inside 'recenter', which is called by
> end-of-buffer:

>   ;; If we went to a place in the middle of the buffer,
>   ;; adjust it to the beginning of a line.
>   (cond ((and arg (not (consp arg))) (forward-line 1))
> 	((and (eq (current-buffer) (window-buffer))
>               (> (point) (window-end nil t)))
> 	 ;; If the end of the buffer is not already on the screen,
> 	 ;; then scroll specially to put it near, but not at, the bottom.
> 	 (overlay-recenter (point))
> 	 (recenter -3))))  <<<<<<<<<<<<<<<<<<<<<<<<<<

> Stepping through 'recenter', I see the following:

>   . It treats GUI frames specially (and indeed, in "emacs -nw", I
>     don't see this slowdown).  The special handling is that it
>     attempts to find the window-start point that corresponds to the -3
>     argument, by interpreting that argument as the number of pixels
>     equivalent to 3 canonical-height lines.

>   . To this end, 'recenter' calls move_it_vertically_backward with the
>     -3 argument converted to pixels.  move_it_vertically_backward then
>     tries to find that place, by simulating display.

>   . As part of the display simulation, jit-lock-function is called
>     (because we hit a buffer position which has a non-nil 'fontified'
>     property) with a single argument: buffer position 948757.  This
>     single call takes almost all of the 11 seconds.

> When the frame is not maximized, the window height is smaller, so
> 'recenter' moves back a smaller amount of pixels, and calls
> jit-lock-function at a different buffer position.  That call takes
> only about 2 sec (still quite a lot, IMO).

> Perhaps Alan could explain why the CC Mode fontification takes such a
> long time for buffer position 948757.

No, sorry I can't explain it.  Except that CC Mode will be scanning
forward through the entire buffer once (hopefully only once) to get the
"parse state" near EOB.

On a normally optimised 24.3 build, M-x goto-char 948757 is taking me a
little over a second.  This is in a GUI frame in X Windows, maximised by
clicking on the maximise button at the top right of the frame.

On my non-optimised 24.3.50 build with tracing built in, the same action,
M-x goto-char 948757 takes 9 seconds.  In a non-maximised frame it takes
8 seconds.  In this build, M-> takes ~2 seconds regardless of whether the
frame is maximised or not.  I can't explain this difference.

I've committed my patch.  I hope it clears up the bug (which I haven't
yet closed) more or less satisfactorally.

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[Eli Zaretskii]

> Date: Sun, 2 Feb 2014 17:40:50 +0000
> Cc: martin rudalics <rudalics@gmx.at>, 16526@debbugs.gnu.org
> From: Alan Mackenzie <acm@muc.de>
> 
> > Perhaps Alan could explain why the CC Mode fontification takes such a
> > long time for buffer position 948757.
> 
> No, sorry I can't explain it.  Except that CC Mode will be scanning
> forward through the entire buffer once (hopefully only once) to get the
> "parse state" near EOB.

Why can't it start from a position outside any function instead?

> On a normally optimised 24.3 build, M-x goto-char 948757 is taking me a
> little over a second.  This is in a GUI frame in X Windows, maximised by
> clicking on the maximise button at the top right of the frame.
> 
> On my non-optimised 24.3.50 build with tracing built in, the same action,
> M-x goto-char 948757 takes 9 seconds.  In a non-maximised frame it takes
> 8 seconds.  In this build, M-> takes ~2 seconds regardless of whether the
> frame is maximised or not.  I can't explain this difference.

These times are consistent with what I see.

> I've committed my patch.  I hope it clears up the bug (which I haven't
> yet closed) more or less satisfactorally.

Thanks.

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[Alan Mackenzie]

Hi, Eli.

On Sun, Feb 02, 2014 at 08:07:25PM +0200, Eli Zaretskii wrote:
> > Date: Sun, 2 Feb 2014 17:40:50 +0000
> > Cc: martin rudalics <rudalics@gmx.at>, 16526@debbugs.gnu.org
> > From: Alan Mackenzie <acm@muc.de>

> > > Perhaps Alan could explain why the CC Mode fontification takes such a
> > > long time for buffer position 948757.

> > No, sorry I can't explain it.  Except that CC Mode will be scanning
> > forward through the entire buffer once (hopefully only once) to get the
> > "parse state" near EOB.

> Why can't it start from a position outside any function instead?

It used to do this, but this was leading to errors, particularly in C++
where, for example, people frequently write "namespace {" rather than
putting the "{" on the next line in column 0.

Even on xdisp.c, over 30,000 and almost 1Mb, the scan doesn't take that
long - a little over a second on my 4 yo machine.

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[Eli Zaretskii]

> Date: Sun, 2 Feb 2014 19:20:07 +0000
> Cc: rudalics@gmx.at, 16526@debbugs.gnu.org
> From: Alan Mackenzie <acm@muc.de>
> 
> Even on xdisp.c, over 30,000 and almost 1Mb, the scan doesn't take that
> long - a little over a second on my 4 yo machine.

Do you need help in identifying what takes 11 sec in the unoptimized
build to traverse a few lines?

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[Alan Mackenzie]

Hello, Eli.

On Sun, Feb 02, 2014 at 10:53:18PM +0200, Eli Zaretskii wrote:
> > Date: Sun, 2 Feb 2014 19:20:07 +0000
> > Cc: rudalics@gmx.at, 16526@debbugs.gnu.org
> > From: Alan Mackenzie <acm@muc.de>

> > Even on xdisp.c, over 30,000 and almost 1Mb, the scan doesn't take that
> > long - a little over a second on my 4 yo machine.

> Do you need help in identifying what takes 11 sec in the unoptimized
> build to traverse a few lines?

Thanks for the offer.  I really need to get down and spend a few hours
investigating.  I've elp'd CC Mode, and tried M-> (2 seconds) followed
by <PgUp> (13 seconds), and it looks like
c-append-lower-brace-pair-to-state-cache is taking up the time.  It
seems it is being called too often.  Maybe.

Maybe I'll pin it down at the weekend.

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[Eli Zaretskii]

> Date: Wed, 5 Feb 2014 23:00:01 +0000
> Cc: rudalics@gmx.at, 16526@debbugs.gnu.org
> From: Alan Mackenzie <acm@muc.de>
> 
> > Do you need help in identifying what takes 11 sec in the unoptimized
> > build to traverse a few lines?
> 
> Thanks for the offer.  I really need to get down and spend a few hours
> investigating.  I've elp'd CC Mode, and tried M-> (2 seconds) followed
> by <PgUp> (13 seconds), and it looks like
> c-append-lower-brace-pair-to-state-cache is taking up the time.  It
> seems it is being called too often.  Maybe.
> 
> Maybe I'll pin it down at the weekend.

Thanks.

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[Eli Zaretskii]

> Date: Thu, 06 Feb 2014 08:01:32 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: rudalics@gmx.at, 16526@debbugs.gnu.org
> 
> > Date: Wed, 5 Feb 2014 23:00:01 +0000
> > Cc: rudalics@gmx.at, 16526@debbugs.gnu.org
> > From: Alan Mackenzie <acm@muc.de>
> > 
> > > Do you need help in identifying what takes 11 sec in the unoptimized
> > > build to traverse a few lines?
> > 
> > Thanks for the offer.  I really need to get down and spend a few hours
> > investigating.  I've elp'd CC Mode, and tried M-> (2 seconds) followed
> > by <PgUp> (13 seconds), and it looks like
> > c-append-lower-brace-pair-to-state-cache is taking up the time.  It
> > seems it is being called too often.  Maybe.
> > 
> > Maybe I'll pin it down at the weekend.
> 
> Thanks.

I hope Alan did something with this issue, or will in some near
future.

But either way, the original bug was fixed, so I'm closing this.

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[Alan Mackenzie]

Hi, Eli.

On Sun, Jun 22, 2014 at 07:49:29PM +0300, Eli Zaretskii wrote:
> > Date: Thu, 06 Feb 2014 08:01:32 +0200
> > From: Eli Zaretskii <eliz@gnu.org>
> > Cc: rudalics@gmx.at, 16526@debbugs.gnu.org

> > > Date: Wed, 5 Feb 2014 23:00:01 +0000
> > > Cc: rudalics@gmx.at, 16526@debbugs.gnu.org
> > > From: Alan Mackenzie <acm@muc.de>

> > > > Do you need help in identifying what takes 11 sec in the unoptimized
> > > > build to traverse a few lines?

> > > Thanks for the offer.  I really need to get down and spend a few hours
> > > investigating.  I've elp'd CC Mode, and tried M-> (2 seconds) followed
> > > by <PgUp> (13 seconds), and it looks like
> > > c-append-lower-brace-pair-to-state-cache is taking up the time.  It
> > > seems it is being called too often.  Maybe.

> > > Maybe I'll pin it down at the weekend.

> > Thanks.

> I hope Alan did something with this issue, or will in some near
> future.

I'm afraid I got bogged down, back in February.  I've still got all my
notes and attempted changes, so I'll see if I can't get something fixed
for this.

> But either way, the original bug was fixed, so I'm closing this.

Thanks.

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[Eli Zaretskii]

> Date: Sun, 22 Jun 2014 19:32:58 +0000
> Cc: 16526@debbugs.gnu.org
> From: Alan Mackenzie <acm@muc.de>
> 
> I'm afraid I got bogged down, back in February.  I've still got all my
> notes and attempted changes, so I'll see if I can't get something fixed
> for this.

Thank you.

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[Alan Mackenzie]

Hi, Eli.

On Sun, Jun 22, 2014 at 07:49:29PM +0300, Eli Zaretskii wrote:
> > Date: Thu, 06 Feb 2014 08:01:32 +0200
> > From: Eli Zaretskii <eliz@gnu.org>
> > Cc: rudalics@gmx.at, 16526@debbugs.gnu.org

> > > Date: Wed, 5 Feb 2014 23:00:01 +0000
> > > Cc: rudalics@gmx.at, 16526@debbugs.gnu.org
> > > From: Alan Mackenzie <acm@muc.de>

> > > > Do you need help in identifying what takes 11 sec in the unoptimized
> > > > build to traverse a few lines?

Just to recap what was taking 11 seconds.  With xdisp.c revision 116189
newly loaded into an emacs -Q, do (goto-char 948757).  This emacs was
built with CFLAGS='-O0 -g3' and --enable-checking='yes,glyphs'

Yes, I would like some help, please.

[ From this point on, all my timings are with reference to my normal
optimised build of Emacs 24.3 (though the version doesn't make any
difference). ]

I elp-instrument-package "c-" and elp-instrument-function "scan-lists".
After the (goto-char 948757), my elp-results looks like this (with blank
lines added for clarity):

c-font-lock-fontify-region                       6           1.12553871    0.187589785
scan-lists                                       2359        0.9151535670  0.0003879413
c-parse-state                                    200         0.8451750940  0.0042258754
c-parse-state-1                                  200         0.7929608409  0.0039648042
c-font-lock-enclosing-decls                      6           0.7164836050  0.1194139341

c-append-lower-brace-pair-to-state-cache         4           0.7143309609  0.1785827402
c-font-lock-declarations                         6           0.255113226   0.0425188710
c-find-decl-spots                                6           0.255040271   0.0425067118
c-backward-over-enum-header                      62          0.1797902729  0.0028998431
c-font-lock-complex-decl-prepare                 6           0.0648537759  0.0108089626

Clearly c-append-lower-brace-pair-to-state-cache is problematic.

I then timed each of the four calls of c-append-lower-...-cache.  Three
were rapid, one took 0.6977412700653076 seconds.  I then zeroed in on the
main loop (the only loop, really) and when I found it was hardly looping
at all (a mere four calls of scan-lists altogether).  I then timed _each_
individual call of scan-lists, and found that the call

    (scan-lists 947171 -1 1)

took 0.6962041854858398 seconds.  (947171 is just inside the closing
brace of syms_of_xdisp, and the result of the scan is 919215, the
function's opening brace.)

How come this scan-lists is taking 0.7 seconds?  When I call this from
M-: (let ((thyme (float-time))) (scan-lists 947171 -1 1) (- (float-time) thyme))
, I get 0.013844966888427734 seconds, 2% of the above figure.

It is true that in CC-Mode, that bit of the code is deeply nested inside
some macros which manipulate text properties.  But when I include these
in the manual call of scan-lists, they barely make any difference.

How can this one single scan-lists called from CC Mode be so slow, yet
the identical call from the minibuffer be 50 times as fast?  My
understanding is that scan-lists is a primitive operation, unaffected by
virtually any context (apart from specific things like
parse-sexp-ignore-comments and syntax-table text properties which are
meant to affect it).

I'm at a loss for ideas right now.  Suggestions would be most welcome.

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[Stefan Monnier]

> How can this one single scan-lists called from CC Mode be so slow, yet
> the identical call from the minibuffer be 50 times as fast?

Do the two return the same position?  The time taken by scan-lists is
proportional to the "distance", so a 50x slowdown would not be
surprising if it goes 50x further.

> My understanding is that scan-lists is a primitive operation,
> unaffected by virtually any context (apart from specific things like
> parse-sexp-ignore-comments and syntax-table text properties which are
> meant to affect it).

Another influence could be parse-sexp-lookup-properties, tho it
definitely shouldn't account for more than a few percents.


        Stefan

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[Alan Mackenzie]

Hi, Stefan.

Thanks for the reply.

On Wed, Jun 25, 2014 at 10:51:24PM -0400, Stefan Monnier wrote:
> > How can this one single scan-lists called from CC Mode be so slow, yet
> > the identical call from the minibuffer be 50 times as fast?

> Do the two return the same position?

They do, yes.

> The time taken by scan-lists is proportional to the "distance", so a
> 50x slowdown would not be surprising if it goes 50x further.

This is surely not the problem.  50x further would have taken point well
outside the bounds of the file to a negative offset.  Besides, I printed
out the end position (in both cases) with `message'.  That position was
of the opening brace of syms_of_xdisp.

> > My understanding is that scan-lists is a primitive operation,
> > unaffected by virtually any context (apart from specific things like
> > parse-sexp-ignore-comments and syntax-table text properties which are
> > meant to affect it).

> Another influence could be parse-sexp-lookup-properties, tho it
> definitely shouldn't account for more than a few percents.

parse-sexp-lookup-properties was set in both cases.  (CC Mode sets it
buffer locally in its initialisation.)

Could CC Mode's use of category text properties be causing this delay,
somehow?

Could scan-lists be exhausting some sort of resource in this particular
invocation which needs ~0.7s to replenish?  Or something like that?

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[Stefan Monnier]

> Could CC Mode's use of category text properties be causing this delay,
> somehow?

I can't see how/why.

> Could scan-lists be exhausting some sort of resource in this particular
> invocation which needs ~0.7s to replenish?  Or something like that?

Or a GC call or something like that?  Can't think of any reason why that
could happen, no.

`profiler-start' is not very fine-grained, so it probably won't give you
much insight as to what's going on inside `scan-lists', but at least it
should be easy to try.


        Stefan

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[Stefan Monnier]

>> Could scan-lists be exhausting some sort of resource in this particular
>> invocation which needs ~0.7s to replenish?  Or something like that?
> Or a GC call or something like that?  Can't think of any reason why that
> could happen, no.

One way to figure it out is to change your code such that scan-lists is
always called twice in a row.  If the time spend is the same both time,
then it's definitely not some sort of resources exhaustion that just
happens to slow down this one particular call.


        Stefan

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[Alan Mackenzie]

Hi, Stefan.

On Fri, Jun 27, 2014 at 06:54:55PM -0400, Stefan Monnier wrote:
> >> Could scan-lists be exhausting some sort of resource in this particular
> >> invocation which needs ~0.7s to replenish?  Or something like that?
> > Or a GC call or something like that?  Can't think of any reason why that
> > could happen, no.

> One way to figure it out is to change your code such that scan-lists is
> always called twice in a row.  If the time spend is the same both time,
> then it's definitely not some sort of resources exhaustion that just
> happens to slow down this one particular call.

Yes, the second call takes as long as the first call.

One thing that did make a big difference was binding
parse-sexp-ignore-comments to nil around a call to scan-lists.  My two
calls to (scan-lists 947171 -1 1) give these results:

parse-sexp-ignore-comments |  end position             time
---------------------------------------------------------------------
           t               |     919215         0.6976802349090576
          nil              |     899080         0.0021085739135742188

So, although with parse-..-comments nil, the distance traversed was
greater, the time taken was a factor ~300 less.  This suggests that
comments have something to do with the problem.

parse_sexp_ignore_comments is only tested twice in scan_lists.  The first
time, it is only setting up syntax table variables and suchlike after
having half-detected a comment closer.  The second time, back_comment
gets called.  It seems likely the problem is in back_comment.

AH!!!!!  GOT IT!!!!  What causes the slowdown is binding
open-paren-in-column-0-is-defun-start to nil around the scan-lists.  In
CC Mode's "engine" parts, that variable is bound to nil.  There are good
reasons for this (which I can't precisely recall just at the moment).

In CC Mode, "'" has string syntax.  So any C comment with an odd number
of apostrophes looks, to back_comment, like it has an "unbalanced string",
and the code ends up at label lossage: in back_comment.  Here
find_defun_start is called, and this returns BOB when open-paren-..-0 is
nil.  From here (BOB) the code scans forward to check this putative
"unbalanced string".  It's a long way from BOB to ~900k, hence the
sluggishness.

Where do we go from here?

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[martin rudalics]

 > AH!!!!!  GOT IT!!!!  What causes the slowdown is binding
 > open-paren-in-column-0-is-defun-start to nil around the scan-lists.  In
 > CC Mode's "engine" parts, that variable is bound to nil.  There are good
 > reasons for this (which I can't precisely recall just at the moment).
 >
 > In CC Mode, "'" has string syntax.  So any C comment with an odd number
 > of apostrophes looks, to back_comment, like it has an "unbalanced string",
 > and the code ends up at label lossage: in back_comment.  Here
 > find_defun_start is called, and this returns BOB when open-paren-..-0 is
 > nil.  From here (BOB) the code scans forward to check this putative
 > "unbalanced string".  It's a long way from BOB to ~900k, hence the
 > sluggishness.
 >
 > Where do we go from here?

In every version of Emacs I'm actually doing some work I have disabled
CC mode's `open-paren-in-column-0-is-defun-start' bindings.  Otherwise,
working on a file like xdisp.c would be virtually impossible for me.  As
for the release I would simply do what I proposed earlier - make these
overriding bindings customizable.  So someone who wants to comment out
code with a paren in the first column can have the "correct" behavior
while people like me can continue work with "incorrect" but yet tame
behavior.  And obviously I could continue to work without having bzr
status tell me that I've changed CC mode files ;-)

martin

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[Stefan Monnier]

> Where do we go from here?

Not sure.  Would `syntax-ppss' return correct values from the code in
question?  If yes, then changing back_comment to call back to Elisp and
use syntax-ppss in the lossage case should eliminate this
algorithmic problem.


        Stefan

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[Alan Mackenzie]

Hi, Martin.

On Sat, Jun 28, 2014 at 04:00:40PM +0200, martin rudalics wrote:
>  > AH!!!!!  GOT IT!!!!  What causes the slowdown is binding
>  > open-paren-in-column-0-is-defun-start to nil around the scan-lists.  In
>  > CC Mode's "engine" parts, that variable is bound to nil.  There are good
>  > reasons for this (which I can't precisely recall just at the moment).

>  > In CC Mode, "'" has string syntax.  So any C comment with an odd number
>  > of apostrophes looks, to back_comment, like it has an "unbalanced string",
>  > and the code ends up at label lossage: in back_comment.  Here
>  > find_defun_start is called, and this returns BOB when open-paren-..-0 is
>  > nil.  From here (BOB) the code scans forward to check this putative
>  > "unbalanced string".  It's a long way from BOB to ~900k, hence the
>  > sluggishness.

>  > Where do we go from here?

> In every version of Emacs I'm actually doing some work I have disabled
> CC mode's `open-paren-in-column-0-is-defun-start' bindings.

This is harmless (and thus helpful) in code conforming to GNU standards.
As you note below, it causes confusion and heartache in other
circumstances.

> Otherwise, working on a file like xdisp.c would be virtually impossible
> for me.  As for the release I would simply do what I proposed earlier -
> make these overriding bindings customizable.

I'd far rather the code worked properly (including fast enough).  Making
a choice between two ugly workarounds customisable would be
short-changing users.

> So someone who wants to comment out code with a paren in the first
> column can have the "correct" behavior while people like me can
> continue work with "incorrect" but yet tame behavior.  And obviously I
> could continue to work without having bzr status tell me that I've
> changed CC mode files ;-)

This particular scan-lists which took 0.7s had to traverse 65 comments,
of which 29 had an odd number of apostrophes.  Scanning xdisp.c from BOB
to syms_of_xdisp on my machine takes 0.024s.  28 * 0.024s = 0.67s.  So if
there were simple cacheing in syntax.c, such that the scan from BOB was
done at most once for each scan-lists, the sluggishness would largely
vanish in this case.

If you look at find_defun_start in syntax.c, you'll be struck by just
what an ugly kludge the code for null open_paren_.._start is.
Substantial improvement must surely be possible.

> martin

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[Eli Zaretskii]

> Date: Sat, 28 Jun 2014 14:55:09 +0000
> From: Alan Mackenzie <acm@muc.de>
> Cc: 16526-done@debbugs.gnu.org
> 
> This particular scan-lists which took 0.7s had to traverse 65 comments,
> of which 29 had an odd number of apostrophes.  Scanning xdisp.c from BOB
> to syms_of_xdisp on my machine takes 0.024s.  28 * 0.024s = 0.67s.  So if
> there were simple cacheing in syntax.c, such that the scan from BOB was
> done at most once for each scan-lists, the sluggishness would largely
> vanish in this case.

Sorry for asking the obvious: why cannot the code know that an
apostrophe inside a comment is just a simple character of no special
significance?  Why does it need to treat apostrophes _inside_comments_
specially?

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[Alan Mackenzie]

Hi, Eli.

On Sat, Jun 28, 2014 at 06:12:53PM +0300, Eli Zaretskii wrote:
> > Date: Sat, 28 Jun 2014 14:55:09 +0000
> > From: Alan Mackenzie <acm@muc.de>
> > Cc: 16526-done@debbugs.gnu.org

> > This particular scan-lists which took 0.7s had to traverse 65 comments,
> > of which 29 had an odd number of apostrophes.  Scanning xdisp.c from BOB
> > to syms_of_xdisp on my machine takes 0.024s.  28 * 0.024s = 0.67s.  So if
> > there were simple cacheing in syntax.c, such that the scan from BOB was
> > done at most once for each scan-lists, the sluggishness would largely
> > vanish in this case.

> Sorry for asking the obvious: why cannot the code know that an
> apostrophe inside a comment is just a simple character of no special
> significance?  Why does it need to treat apostrophes _inside_comments_
> specially?

I think it's because whilst scanning backwards, it's not known that the
comment ender actually ends a comment.  For example:

    /*    */   "       /*     "   */
                              ^
			      |

.  Here we actually have a comment followed by a string followed by an
unmated comment closer.  If, at the marked position, we were to assume
that the " was "nothing special", we'd mis-parse the above as a comment
containing a quote mark, then get horribly confused trying to find the
mating double quote mark for the first ".  Or something like that.

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[Alan Mackenzie]

Hi, Stefan.

On Sat, Jun 28, 2014 at 10:34:58AM -0400, Stefan Monnier wrote:
> > Where do we go from here?

> Not sure.  Would `syntax-ppss' return correct values from the code in
> question?  If yes, then changing back_comment to call back to Elisp and
> use syntax-ppss in the lossage case should eliminate this
> algorithmic problem.

No, syntax-ppss wouldn't do here without considerable modification.
scan-lists must be utterly robust.  syntax-ppss can get confused if the
user changes the syntax-table, or adds systax-table text properties, or
binds before-change-functions to nil and makes a change.

How about simply (tm) find-defun-start actually determining a defun
start (when open-paren-..-defun is nil) the first time it is called for
an invocation of scan-lists, then remembering this position for
subsequent use?  This would work well for the particular scan-lists
invocation we've been talking about.

As an enhancement, which would work well for scan-lists at top level
with a large negative COUNT, we could keep a list of top level positions
during the initial scan, say every ~5k characters.  (Or even store the
actual parse states every 5k characters exactly.)

Or something like that.

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[martin rudalics]

 > Making a choice between two ugly workarounds customisable would be
 > short-changing users.

My `open-paren-in-column-0-is-defun-start' is t and by ignoring that
setting in CC mode you are already short-changing me.

martin

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[Alan Mackenzie]

Hi, Martin.

On Sun, Jun 29, 2014 at 10:44:42AM +0200, martin rudalics wrote:
>  > Making a choice between two ugly workarounds customisable would be
>  > short-changing users.

> My `open-paren-in-column-0-is-defun-start' is t and by ignoring that
> setting in CC mode you are already short-changing me.

That's a bit philosophical.  open-..-start is advertised as a high level
option, and CC Mode only binds it to nil at a low level, the "engine"
level.  Binding user options to set values at a low level is quite a
common practice in Elisp programming.

The actual bug here is that find_defun_start in syntax.c doesn't do any
cacheing, hence is scanning from BOB far too often.  How about we see
how things work once this is fixed.

> martin

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[martin rudalics]

 > That's a bit philosophical.  open-..-start is advertised as a high level
 > option, and CC Mode only binds it to nil at a low level, the "engine"
 > level.  Binding user options to set values at a low level is quite a
 > common practice in Elisp programming.

It is a quite bad common practice in Elisp programming.

 > The actual bug here is that find_defun_start in syntax.c doesn't do any
 > cacheing, hence is scanning from BOB far too often.  How about we see
 > how things work once this is fixed.

We're discussing this problem for some ten years now and I'm afraid we
won't find a solution for the imminent release.

martin

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[Alan Mackenzie]

Hi, Martin.

On Sun, Jun 29, 2014 at 12:06:46PM +0200, martin rudalics wrote:
>  > That's a bit philosophical.  open-..-start is advertised as a high level
>  > option, and CC Mode only binds it to nil at a low level, the "engine"
>  > level.  Binding user options to set values at a low level is quite a
>  > common practice in Elisp programming.

> It is a quite bad common practice in Elisp programming.

:-)  We're down at the philosophy level again.

>  > The actual bug here is that find_defun_start in syntax.c doesn't do any
>  > cacheing, hence is scanning from BOB far too often.  How about we see
>  > how things work once this is fixed.

> We're discussing this problem for some ten years now and I'm afraid we
> won't find a solution for the imminent release.

We've been discussing it for some while, yes, but a concrete diagnosis
of the problem first happened yesterday, as far as I'm aware.  Do you
feel like fixing it?  I think you know the C code better than I do, and
you're highly motivated.  Once fixed, Stefan might be inclined to
include the fix in 24.4.

As I suggested yesterday, what is needed is for find_defun_start to
actually determine the beginning-of-defun before POS/POS_BYTE (using
scan_sexps_forward) rather than just giving up and returning BOB.
This position must then be cached for future find_defun_start calls.
The cacheing mechanism is already in place for non-nil open-...-start.

The above simple cacheing might not be adequate in theory, but I'm
guessing that in practice it will be a substantial win.

> martin

-- 
Alan Mackenzie (Nuremberg, Germany)

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[martin rudalics]

 > As I suggested yesterday, what is needed is for find_defun_start to
 > actually determine the beginning-of-defun before POS/POS_BYTE (using
 > scan_sexps_forward) rather than just giving up and returning BOB.
 > This position must then be cached for future find_defun_start calls.
 > The cacheing mechanism is already in place for non-nil open-...-start.

I don't even understand why you can't use the 9th element returned by
`syntax-ppss' to get the beginning of the outermost defun enclosing
`point'.

martin

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[Alan Mackenzie]

On Sun, Jun 29, 2014 at 04:18:42PM +0200, martin rudalics wrote:
>  > As I suggested yesterday, what is needed is for find_defun_start to
>  > actually determine the beginning-of-defun before POS/POS_BYTE (using
>  > scan_sexps_forward) rather than just giving up and returning BOB.
>  > This position must then be cached for future find_defun_start calls.
>  > The cacheing mechanism is already in place for non-nil open-...-start.

> I don't even understand why you can't use the 9th element returned by
> `syntax-ppss' to get the beginning of the outermost defun enclosing
> `point'.

scan-lists, a primitive, must be utterly robust.  syntax-ppss is too
fragile to be used here without a lot of hardening.

syntax-ppss's cache is rendered invalid at any operation which changes
the syntax table (e.g. `modify-syntax-entry'), or changes to a different
syntax table (e.g. `with-syntax-table'), or when a syntax-table text
property/overlay is applied to the buffer, or even when a symbol's
property list is changed when that symbol is the value of a category
text property.  CC Mode does all these things, and does all of them
apart from the first during run time, not merely at mode initialisation.
Also, the syntax-ppss cache's functioning is dependent upon
before-change-functions, which can be bound to nil by any program at any
time.

If one were to harden syntax-ppss against all these things, one would
probably end up calculating the cache from scratch every time, in
effect.

scan-lists is a primitive, and must function correctly regardless of any
trickery which might be played on it.

> martin

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[martin rudalics]

 > scan-lists, a primitive, must be utterly robust.  syntax-ppss is too
 > fragile to be used here without a lot of hardening.
 >
 > syntax-ppss's cache is rendered invalid at any operation which changes
 > the syntax table (e.g. `modify-syntax-entry'), or changes to a different
 > syntax table (e.g. `with-syntax-table'), or when a syntax-table text
 > property/overlay is applied to the buffer, or even when a symbol's
 > property list is changed when that symbol is the value of a category
 > text property.  CC Mode does all these things, and does all of them
 > apart from the first during run time, not merely at mode initialisation.

How can any of these affect the cached start position of a defun before
the position where the buffer contents were changed?

 > Also, the syntax-ppss cache's functioning is dependent upon
 > before-change-functions, which can be bound to nil by any program at any
 > time.

There's an appropriate advice what to do in such case.

 > If one were to harden syntax-ppss against all these things, one would
 > probably end up calculating the cache from scratch every time, in
 > effect.

Can you give an example?

 > scan-lists is a primitive, and must function correctly regardless of any
 > trickery which might be played on it.

You can easily play trickery on `scan-lists' if you start it within a
comment or string.  So `syntax-ppss' is at least as primitive as
`scan-lists' (especially when the latter is used with negative COUNT).

Anyway, IIUC this implies that CC mode can't work with `syntax-ppss' at
all.  If that is true, then `open-paren-in-column-0-is-defun-start' was
indeed the last resort that made editing larger files possible.

martin

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[Alan Mackenzie]

Hi, Martin.

On Sun, Jun 29, 2014 at 06:01:58PM +0200, martin rudalics wrote:
>  > scan-lists, a primitive, must be utterly robust.  syntax-ppss is too
>  > fragile to be used here without a lot of hardening.

>  > syntax-ppss's cache is rendered invalid at any operation which changes
>  > the syntax table (e.g. `modify-syntax-entry'), or changes to a different
>  > syntax table (e.g. `with-syntax-table'), or when a syntax-table text
>  > property/overlay is applied to the buffer, or even when a symbol's
>  > property list is changed when that symbol is the value of a category
>  > text property.  CC Mode does all these things, and does all of them
>  > apart from the first during run time, not merely at mode initialisation.

> How can any of these affect the cached start position of a defun before
> the position where the buffer contents were changed?

In this context, the "start of defun" means an outermost paren of some
sort, nothing more, nothing less.  Any of the changes above could disturb
this:  For example, adding an open-paren syntax-table property to a
character which thereby becomes the BOD.  Or temporarily switching to the
Pascal syntax table, where "{" and "}" are comment brackets.  I'm not
saying that something like this will happen on a regular basis, but it's
definitely possible.

>  > Also, the syntax-ppss cache's functioning is dependent upon
>  > before-change-functions, which can be bound to nil by any program at any
>  > time.

> There's an appropriate advice what to do in such case.

Primitive functions are deaf to advice.  You're surely not suggesting
that a primitive like scan-lists should become vulnerable to high-level
programmers failing to follow advice?  If this were to be done,
scan-lists would only work most of the time, not all of the time.  This
would be catastrophic for Emacs.

>  > If one were to harden syntax-ppss against all these things, one would
>  > probably end up calculating the cache from scratch every time, in
>  > effect.

> Can you give an example?

An example of what?  What I meant was, each time you used syntax-ppss
from scan-lists, you'd somehow need to be sure that a syntax-table entry
hadn't been changed, etc.  I can't really see how this would be possible.
You'd somehow need to handle constructs like

    (with-syntax-table c++-template-syntax-table
      ....
      (scan-lists (point) -1 1)
      ....)

.

>  > scan-lists is a primitive, and must function correctly regardless of any
>  > trickery which might be played on it.

> You can easily play trickery on `scan-lists' if you start it within a
> comment or string.

No.  scan-lists works according to its spec even if you start it inside a
comment/string.  It's effect is well defined.  For example if you write
parens inside a comment, C-M-n and friends do the right thing there.

> So `syntax-ppss' is at least as primitive as `scan-lists' (especially
> when the latter is used with negative COUNT).

Sorry, but that's utter rubbish.  syntax-ppss is a high-level convenience
function, which if used carefully by the lisp programmer gives correct
results.

By contrast, scan-lists does precisely what it says, no ifs and no buts.
Even with a negative COUNT.

> Anyway, IIUC this implies that CC mode can't work with `syntax-ppss' at
> all.  If that is true, then `open-paren-in-column-0-is-defun-start' was
> indeed the last resort that made editing larger files possible.

CC Mode doesn't use syntax-ppss, but I can't see how that's implied by
anything we've been discussing so far.  It does maintain its own caches
which fulfil the same purpose.  For example, there's a syntactic cache
which treats preprocessor constructs as comments, something syntax-ppss
can't do.

open-..-start being t is a kludge which works for certain types of files
and situations and not others.  It was causing hard to fathom errors in
CC Mode, particularly C and C++.

Do I take it you're not keen on enhancing find_defun_start in the manner
I suggested?

> martin

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[Stefan Monnier]

> No, syntax-ppss wouldn't do here without considerable modification.

Why?

> scan-lists must be utterly robust.  syntax-ppss can get confused if the
> user changes the syntax-table,

Indeed, the user shouldn't change the syntax-table ;-)

> or adds systax-table text properties,

The user should do that via syntax-propertize.

> or binds before-change-functions to nil and makes a change.

User gets what she deserves.  Binding inhibit-modification-hooks while
making changes is dangerous, but binding before-change-functions to nil
is simply evil and asking for trouble.

> How about simply (tm) find-defun-start actually determining a defun
> start (when open-paren-..-defun is nil) the first time it is called for
> an invocation of scan-lists, then remembering this position for
> subsequent use?

That's indeed what syntax-ppss would do.  But if the syntax-table may
change (or added as text-properties, or if before-change-functions is
let-bound to nil while making modifications), any such mechanism is
bound to fail because in those cases it shouldn't "remember this
position for subsequent use".

> As an enhancement, which would work well for scan-lists at top level
> with a large negative COUNT, we could keep a list of top level positions
> during the initial scan, say every ~5k characters.  (Or even store the
> actual parse states every 5k characters exactly.)

You're describing syntax-ppss.


        Stefan

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[Stefan Monnier]

> scan-lists, a primitive, must be utterly robust.  syntax-ppss is too
> fragile to be used here without a lot of hardening.

FWIW, CC-mode is one of the last remaining major modes (at least the
only one I know of that's bundled with Emacs) that doesn't play nice
with syntax-ppss.  Incidentally, AFAIK it's also the only code which
uses font-lock-extend-after-change-region-function (and yes, I think the
two are (indirectly) related).

> If one were to harden syntax-ppss against all these things, one would
> probably end up calculating the cache from scratch every time, in effect.

Exactly.  IOW a cache can only be robust if the major mode plays by the
rules, which cc-mode doesn't do.


        Stefan

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[martin rudalics]

 >> How can any of these affect the cached start position of a defun before
 >> the position where the buffer contents were changed?
 >
 > In this context, the "start of defun" means an outermost paren of some
 > sort, nothing more, nothing less.  Any of the changes above could disturb
 > this:  For example, adding an open-paren syntax-table property to a
 > character which thereby becomes the BOD.  Or temporarily switching to the
 > Pascal syntax table, where "{" and "}" are comment brackets.  I'm not
 > saying that something like this will happen on a regular basis, but it's
 > definitely possible.

Then you can forget caching.

 >>   > Also, the syntax-ppss cache's functioning is dependent upon
 >>   > before-change-functions, which can be bound to nil by any program at any
 >>   > time.
 >
 >> There's an appropriate advice what to do in such case.
 >
 > Primitive functions are deaf to advice.  You're surely not suggesting
 > that a primitive like scan-lists should become vulnerable to high-level
 > programmers failing to follow advice?  If this were to be done,
 > scan-lists would only work most of the time, not all of the time.  This
 > would be catastrophic for Emacs.

I meant that if someone is going to disable `before-change-functions'
that someone has to take care of flushing the cache.

 > An example of what?  What I meant was, each time you used syntax-ppss
 > from scan-lists, you'd somehow need to be sure that a syntax-table entry
 > hadn't been changed, etc.  I can't really see how this would be possible.
 > You'd somehow need to handle constructs like
 >
 >      (with-syntax-table c++-template-syntax-table
 >        ....
 >        (scan-lists (point) -1 1)
 >        ....)
 >
 > .

And how would a defun start cache handle such constructs?

 > No.  scan-lists works according to its spec even if you start it inside a
 > comment/string.  It's effect is well defined.  For example if you writen
 > parens inside a comment, C-M-n and friends do the right thing there.

Here C-M-n is bound to `forward-list' which according to its doc-string
"assumes point is not in a string or comment".  `scan-lists' has
precisely the same problems as `syntax-ppss' wrt syntax changes.

 >> So `syntax-ppss' is at least as primitive as `scan-lists' (especially
 >> when the latter is used with negative COUNT).
 >
 > Sorry, but that's utter rubbish.  syntax-ppss is a high-level convenience
 > function, which if used carefully by the lisp programmer gives correct
 > results.

We're not talking about `scan-lists' alone.  We're talking about
`scan-lists' (or more precisely find_defun_start) with a cache of
previously calculated positions.

 > By contrast, scan-lists does precisely what it says, no ifs and no buts.
 > Even with a negative COUNT.

Any "problems" of `syntax-ppss' in this regard are the problems of
scan_lists plus those of maintaining a cache.  No ifs and no buts.

 >> Anyway, IIUC this implies that CC mode can't work with `syntax-ppss' at
 >> all.  If that is true, then `open-paren-in-column-0-is-defun-start' was
 >> indeed the last resort that made editing larger files possible.
 >
 > CC Mode doesn't use syntax-ppss, but I can't see how that's implied by
 > anything we've been discussing so far.  It does maintain its own caches
 > which fulfil the same purpose.  For example, there's a syntactic cache
 > which treats preprocessor constructs as comments, something syntax-ppss
 > can't do.

And how do you invalidate that cache?

 > open-..-start being t is a kludge which works for certain types of files
 > and situations and not others.  It was causing hard to fathom errors in
 > CC Mode, particularly C and C++.

I can live with such errors.  I can't live with the slowdown.

 > Do I take it you're not keen on enhancing find_defun_start in the manner
 > I suggested?

What you're asking for is impossible:

(1) You want to base find_defun_start on scan_lists starting at BOB.
     This means that you need a function that repeatedly calls
     scan_lists, stopping whenever depth reaches zero and remembers that
     position, returning the last such position before FROM.  Such a
     function exists already - it's called `parse-partial-sexp'.

(2) You want to avoid that function call scan_lists repeatedly by
     caching previously found positions.  Such a function exists already
     - it's called `syntax-ppss'.

(3) You want that function to work even if someone changes the syntax
     table or disables `before-change-functions' without flushing its
     cache.  Such a function does not exist and never will.

martin

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[Stefan Monnier]

> For example, there's a syntactic cache which treats preprocessor
> constructs as comments, something syntax-ppss can't do.

Can you expand on why you think syntax-ppss can't do that?


        Stefan

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[Alan Mackenzie]

Hello, Stefan.

On Sun, Jun 29, 2014 at 06:14:45PM -0400, Stefan Monnier wrote:
> > scan-lists, a primitive, must be utterly robust.  syntax-ppss is too
> > fragile to be used here without a lot of hardening.

> FWIW, CC-mode is one of the last remaining major modes (at least the
> only one I know of that's bundled with Emacs) that doesn't play nice
> with syntax-ppss.

Doesn't it?  First I've heard of it.  Though of course, what comes out
of syntax-ppss is going to be of limited value when you've got lines
like this in your source:

    #define FOO BAR /* accidentally unclosed comment.

Just as a matter of interest, what code is there around which wants to
use syntax-ppss on a CC Mode buffer?

> Incidentally, AFAIK it's also the only code which uses
> font-lock-extend-after-change-region-function ....

Is that right?  Well, what do you know!  Still, it's a good facility to
have around for when it's needed.  It avoids the need to have to advise
font-lock after change functions, whilst still doing fontification
right.

> .... (and yes, I think the two are (indirectly) related).

Well, everything's related to everything else, more or less indirectly.
Do you think there's any significance to this indirect relationship?

> > If one were to harden syntax-ppss against all these things, one would
> > probably end up calculating the cache from scratch every time, in effect.

> Exactly.  IOW a cache can only be robust if the major mode plays by the
> rules, which cc-mode doesn't do.

CC Mode does what it has to do to do its job.  As you yourself, amongst
others, have remarked, CC Mode deals with utter bastards of languages.
Its caches are quite capable of being robust.  I'm not convinced that
syntax-ppss is capable of being robust.  Some of its shortcomings are
listed in syntax.el.

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[Stefan Monnier]

>> FWIW, CC-mode is one of the last remaining major modes (at least the
>> only one I know of that's bundled with Emacs) that doesn't play nice
>> with syntax-ppss.
> Doesn't it?  First I've heard of it.

Well, I go by what you said:

    No, syntax-ppss wouldn't do here without considerable modification.

> Though of course, what comes out of syntax-ppss is going to be of
> limited value when you've got lines like this in your source:
>
>     #define FOO BAR /* accidentally unclosed comment.

Yes, sometimes you need to be a bit creative to get syntax-ppss
to behave.  But so far, I've always managed to coerce it acceptably.

> Just as a matter of interest, what code is there around which wants to
> use syntax-ppss on a CC Mode buffer?

It's used by various obscure packages like font-lock, newcomment, ...

>> Incidentally, AFAIK it's also the only code which uses
>> font-lock-extend-after-change-region-function ....
> Is that right?  Well, what do you know!  Still, it's a good facility to
> have around for when it's needed.  It avoids the need to have to advise
> font-lock after change functions, whilst still doing fontification right.

Other packages manage to do it right without it.
As soon as CC-mode is changed to use syntax-propertize-function,
I expect that it won't use font-lock-extend-after-change-region-function
any longer.

> CC Mode does what it has to do to do its job.  As you yourself, amongst
> others, have remarked, CC Mode deals with utter bastards of languages.
> Its caches are quite capable of being robust.  I'm not convinced that
> syntax-ppss is capable of being robust.  Some of its shortcomings are
> listed in syntax.el.

I don't think there's much point in comparing the track record of
caching bugs in syntax.el compared to cc-mode.


        Stefan

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[Alan Mackenzie]

Hi, Martin.

On Mon, Jun 30, 2014 at 09:55:02AM +0200, martin rudalics wrote:
>  >> How can any of these affect the cached start position of a defun before
>  >> the position where the buffer contents were changed?

>  > In this context, the "start of defun" means an outermost paren of some
>  > sort, nothing more, nothing less.  Any of the changes above could disturb
>  > this:  For example, adding an open-paren syntax-table property to a
>  > character which thereby becomes the BOD.  Or temporarily switching to the
>  > Pascal syntax table, where "{" and "}" are comment brackets.  I'm not
>  > saying that something like this will happen on a regular basis, but it's
>  > definitely possible.

> Then you can forget caching.

No.  The particular rogue invocation of scan-lists which caused this
(sub-)thread scans from BOB to near EOB 29 times.  Inside scan-lists,
_NO_ changes to syntax-tables, s-t text properties etc. are possible.
What I'm suggesting is that cacheing that first scan from BOB will be a
big win for many time-consuming scan-listses.

[ .... ]

> I meant that if someone is going to disable `before-change-functions'
> that someone has to take care of flushing the cache.

OK.

>  > An example of what?  What I meant was, each time you used syntax-ppss
>  > from scan-lists, you'd somehow need to be sure that a syntax-table entry
>  > hadn't been changed, etc.  I can't really see how this would be possible.
>  > You'd somehow need to handle constructs like

>  >      (with-syntax-table c++-template-syntax-table
>  >        ....
>  >        (scan-lists (point) -1 1)
>  >        ....)

>  > .

> And how would a defun start cache handle such constructs?

Such a cache would be initialised to nil within a particular invocation
of scan-lists, hence nothing like the above could cause any trouble.

>  > No.  scan-lists works according to its spec even if you start it inside a
>  > comment/string.  It's effect is well defined.  For example if you writen
>  > parens inside a comment, C-M-n and friends do the right thing there.

> Here C-M-n is bound to `forward-list' which according to its doc-string
> "assumes point is not in a string or comment".  `scan-lists' has
> precisely the same problems as `syntax-ppss' wrt syntax changes.

scan-lists is an atomic function, without memory of any state.  It's
functionality is wholly determined by the current buffer state and the
current setting of (quite a few) state variables.

syntax-ppss is motivated by cacheing historical state, hence is
critically different.

>  >> So `syntax-ppss' is at least as primitive as `scan-lists' (especially
>  >> when the latter is used with negative COUNT).

>  > Sorry, but that's utter rubbish.  syntax-ppss is a high-level convenience
>  > function, which if used carefully by the lisp programmer gives correct
>  > results.

> We're not talking about `scan-lists' alone.  We're talking about
> `scan-lists' (or more precisely find_defun_start) with a cache of
> previously calculated positions.

OK, I think I see what you're getting at.  You're thinking of a cache
which would endure over random buffer changes.  The cache I was intending
would be flushed at the beginning of each scan-lists.  "What I tell you
three times is true." (Lewis Carroll, The Hunting of the Snark).

>  > By contrast, scan-lists does precisely what it says, no ifs and no buts.
>  > Even with a negative COUNT.

> Any "problems" of `syntax-ppss' in this regard are the problems of
> scan_lists plus those of maintaining a cache.  No ifs and no buts.

>  >> Anyway, IIUC this implies that CC mode can't work with `syntax-ppss' at
>  >> all.  If that is true, then `open-paren-in-column-0-is-defun-start' was
>  >> indeed the last resort that made editing larger files possible.

>  > CC Mode doesn't use syntax-ppss, but I can't see how that's implied by
>  > anything we've been discussing so far.  It does maintain its own caches
>  > which fulfil the same purpose.  For example, there's a syntactic cache
>  > which treats preprocessor constructs as comments, something syntax-ppss
>  > can't do.

> And how do you invalidate that cache?

It's truncated at buffer changes.  It doesn't contain any information
which could become stale on any of the other sorts of modifications we've
been talking about, or it isn't used when they could hurt anything.  That
this cache has a tightly defined purpose is why it can be robust, whereas
syntax-ppss's cache, being very general purpose, is vulnerable to certain
changes.

>  > open-..-start being t is a kludge which works for certain types of files
>  > and situations and not others.  It was causing hard to fathom errors in
>  > CC Mode, particularly C and C++.

> I can live with such errors.  I can't live with the slowdown.

I'm hoping the slowdown will go away, or at least become tolerable, when
scan-lists is optimised.

>  > Do I take it you're not keen on enhancing find_defun_start in the manner
>  > I suggested?

> What you're asking for is impossible:

> (1) You want to base find_defun_start on scan_lists starting at BOB.

No, on scan_sexps_forward, which (despite the confusing name) is the core
of parse-partial-sexp without its lispy arguments.

>      This means that you need a function that repeatedly calls
>      scan_lists, stopping whenever depth reaches zero and remembers that
>      position, returning the last such position before FROM.  Such a
>      function exists already - it's called `parse-partial-sexp'.

Yes.  Or scan_sexps_forward which is the C core of it.

> (2) You want to avoid that function call scan_lists repeatedly by
>      caching previously found positions.  Such a function exists already
>      - it's called `syntax-ppss'.

I'd have nothing against using syntax-ppss, as long as its cache was
bound to nil for each scan-lists call - except it would cause some
slowdown (compared with a special-purpose cache written in C), since it
goes through the lisp bytecode interpreter.  

> (3) You want that function to work even if someone changes the syntax
>      table or disables `before-change-functions' without flushing its
>      cache.  Such a function does not exist and never will.

No, I want that cache to be flushed for each scan-lists call, so that the
first scan of (nearly) the whole buffer will create the cache, and the
other 28 scans will use that cache.

> martin

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[Stefan Monnier]

> No.  The particular rogue invocation of scan-lists which caused this
> (sub-)thread scans from BOB to near EOB 29 times.  Inside scan-lists,
> _NO_ changes to syntax-tables, s-t text properties etc. are possible.
> What I'm suggesting is that cacheing that first scan from BOB will be a
> big win for many time-consuming scan-listses.

Here's the situation: we can add some special code to handle your case.
But adding code which will use syntax-ppss would be more beneficial for
99% of the major modes.

Adding a cache that only works within a single scan-lists instance is
too much trouble for too little gain.  Adding a cache that is flushed
less often means either it will suffer from the same limitations as
syntax-ppss (so we may as well use syntax-ppss), or it needs to
additionally hook into the text-property and set-syntax-table code to be
flushed in those extra corner cases, and there again these extra checks
will be only useful for those rare modes like CC-mode and will be at
best harmless and in some cases detrimental to performance compared to
simply using syntax-ppss.

IOW, using syntax-ppss for find_defun_start is The Right Thing to do.
And CC-mode should then be fixed to take advantage of it.


        Stefan

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[martin rudalics]

 > No.  The particular rogue invocation of scan-lists which caused this
 > (sub-)thread scans from BOB to near EOB 29 times.  Inside scan-lists,
 > _NO_ changes to syntax-tables, s-t text properties etc. are possible.
 > What I'm suggesting is that cacheing that first scan from BOB will be a
 > big win for many time-consuming scan-listses.

To obtain a meaningful result from `scan-lists' with a negative COUNT,
we have to know whether the starting point is outside a comment or
string.  This means that we either first have to scan from BOB till that
point or have the results from a previous scan in a not yet invalidated
cache anyway.  Can we agree on that?

Now it's possible that CC-mode for some magic reason knows that the
starting point meets the requirement above.  In that case the magic used
should also assure that `scan-lists' stops at a reasonable position (one
known to have DEPTH zero when scanned from BOB) by narrowing the buffer
appropriately.

Otherwise we need some heuristics to provide a reasonable amount of
defun starters to be eventually used by back_comment (I suppose it's
that routine causing our problems).  In any case, these starters must be
provided by `parse-partial-sexp' (or forward_sexp as you suggest).  I
suppose we agree on that.

In any case the question is why any defun starters cached before
invoking `scan-lists' would be invalid.  I can't see a good reason.  Can
you give me one?.

 >> And how would a defun start cache handle such constructs?
 >
 > Such a cache would be initialised to nil within a particular invocation
 > of scan-lists, hence nothing like the above could cause any trouble.

It's a waste that two subsequent `scan-lists' would not share the same
cache but let's stay with this assumption for the rest.

 > scan-lists is an atomic function, without memory of any state.  It's
 > functionality is wholly determined by the current buffer state and the
 > current setting of (quite a few) state variables.
 >
 > syntax-ppss is motivated by cacheing historical state, hence is
 > critically different.

Then we have to flush its cache when calling `scan-lists'.

 > OK, I think I see what you're getting at.  You're thinking of a cache
 > which would endure over random buffer changes.  The cache I was intending
 > would be flushed at the beginning of each scan-lists.  "What I tell you
 > three times is true." (Lewis Carroll, The Hunting of the Snark).

I was thinking of one cache per buffer that would be produced by parsing
from BOB.  A second cache would mean to double the overhead for writing
and maintaining the respective code.

 > It's truncated at buffer changes.  It doesn't contain any information
 > which could become stale on any of the other sorts of modifications we've
 > been talking about, or it isn't used when they could hurt anything.  That
 > this cache has a tightly defined purpose is why it can be robust, whereas
 > syntax-ppss's cache, being very general purpose, is vulnerable to certain
 > changes.

Which obviously means that we'd have to improve the robustness of the
syntax-ppss cache.

 >> What you're asking for is impossible:
 >
 >> (1) You want to base find_defun_start on scan_lists starting at BOB.
 >
 > No, on scan_sexps_forward, which (despite the confusing name) is the core
 > of parse-partial-sexp without its lispy arguments.

OK

 >>       This means that you need a function that repeatedly calls
 >>       scan_lists, stopping whenever depth reaches zero and remembers that
 >>       position, returning the last such position before FROM.  Such a
 >>       function exists already - it's called `parse-partial-sexp'.
 >
 > Yes.  Or scan_sexps_forward which is the C core of it.

OK

 >> (2) You want to avoid that function call scan_lists repeatedly by
 >>       caching previously found positions.  Such a function exists already
 >>       - it's called `syntax-ppss'.
 >
 > I'd have nothing against using syntax-ppss, as long as its cache was
 > bound to nil for each scan-lists call - except it would cause some
 > slowdown (compared with a special-purpose cache written in C), since it
 > goes through the lisp bytecode interpreter.

Then we have to provide a C-level interface to that cache which can be
used by find_defun_start.

 >> (3) You want that function to work even if someone changes the syntax
 >>       table or disables `before-change-functions' without flushing its
 >>       cache.  Such a function does not exist and never will.
 >
 > No, I want that cache to be flushed for each scan-lists call, so that the
 > first scan of (nearly) the whole buffer will create the cache, and the
 > other 28 scans will use that cache.

This means that we'd have to specify which defun starts to store in that
cache and replace the `open-paren-in-column-0-is-defun-start' stuff in
find_defun_start by going to the last position stored in that cache.
The latter should be trivial but is certainly not suited for Emacs 24.4.
The former is less trivial (IMHO) and requires a general solution within
the context of `syntax-ppss' which might even mean to move the cache
building and flushing parts of the latter to C.

martin

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[Stefan Monnier]

Hi Alan,

Can you try the patch below (for the emacs-24 branch), which tries to
make the existing cache in find_defun_start do something useful in the
case where find_defun_start returns BEGV?


        Stefan


=== modified file 'src/syntax.c'
--- src/syntax.c	2014-02-08 05:12:47 +0000
+++ src/syntax.c	2014-07-03 14:57:04 +0000
@@ -530,17 +530,6 @@
 {
   ptrdiff_t opoint = PT, opoint_byte = PT_BYTE;
 
-  if (!open_paren_in_column_0_is_defun_start)
-    {
-      find_start_value = BEGV;
-      find_start_value_byte = BEGV_BYTE;
-      find_start_buffer = current_buffer;
-      find_start_modiff = MODIFF;
-      find_start_begv = BEGV;
-      find_start_pos = pos;
-      return BEGV;
-    }
-
   /* Use previous finding, if it's valid and applies to this inquiry.  */
   if (current_buffer == find_start_buffer
       /* Reuse the defun-start even if POS is a little farther on.
@@ -552,6 +541,13 @@
       && MODIFF == find_start_modiff)
     return find_start_value;
 
+  if (!open_paren_in_column_0_is_defun_start)
+    {
+      find_start_value = BEGV;
+      find_start_value_byte = BEGV_BYTE;
+      goto found;
+    }
+
   /* Back up to start of line.  */
   scan_newline (pos, pos_byte, BEGV, BEGV_BYTE, -1, 1);
 
@@ -582,13 +578,14 @@
   /* Record what we found, for the next try.  */
   find_start_value = PT;
   find_start_value_byte = PT_BYTE;
+  TEMP_SET_PT_BOTH (opoint, opoint_byte);
+
+ found:
   find_start_buffer = current_buffer;
   find_start_modiff = MODIFF;
   find_start_begv = BEGV;
   find_start_pos = pos;
 
-  TEMP_SET_PT_BOTH (opoint, opoint_byte);
-
   return find_start_value;
 }
 \f
@@ -841,6 +838,7 @@
   else
     {
       struct lisp_parse_state state;
+      bool adjusted = false;
     lossage:
       /* We had two kinds of string delimiters mixed up
 	 together.  Decode this going forwards.
@@ -852,6 +850,8 @@
 	{
 	  defun_start = find_defun_start (comment_end, comment_end_byte);
 	  defun_start_byte = find_start_value_byte;
+	  if (defun_start != BEGV)
+	    adjusted = true;
 	}
       do
 	{
@@ -860,6 +860,16 @@
 			      comment_end, TYPE_MINIMUM (EMACS_INT),
 			      0, Qnil, 0);
 	  defun_start = comment_end;
+	  if (!adjusted)
+	    {
+	      adjusted = true;
+	      find_start_value
+		= CONSP (state.levelstarts) ? XINT (XCAR (state.levelstarts))
+		: state.thislevelstart >= 0 ? state.thislevelstart
+		: find_start_value;
+	      find_start_value_byte = CHAR_TO_BYTE (find_start_value);
+	    }
+
 	  if (state.incomment == (comnested ? 1 : -1)
 	      && state.comstyle == comstyle)
 	    from = state.comstr_start;

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[Alan Mackenzie]

Hello, Stefan.

On Thu, Jul 03, 2014 at 10:59:58AM -0400, Stefan Monnier wrote:
> Can you try the patch below (for the emacs-24 branch), which tries to
> make the existing cache in find_defun_start do something useful in the
> case where find_defun_start returns BEGV?

[ patch snipped ]

I'm not sure what I'm meant to see, but it has made no perceptible
difference to the "slow" invocation of scan-lists.  In particular, that
invocation took 0.7196781635284424 seconds with the patch, which is
"exactly" (near enough) what it took before.

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[Stefan Monnier]

>> Can you try the patch below (for the emacs-24 branch), which tries to
>> make the existing cache in find_defun_start do something useful in the
>> case where find_defun_start returns BEGV?
> [ patch snipped ]
> I'm not sure what I'm meant to see,

An improvement in performance.

> but it has made no perceptible difference to the "slow" invocation of
> scan-lists.  In particular, that invocation took 0.7196781635284424
> seconds with the patch, which is "exactly" (near enough) what it
> took before.

Oh well.  What he patch does (or should do, at least) is that after
a "full scan from BOB", it remembers the last top-level open paren seen,
so that if the next back_comment's lossage happens in that same area, we
only rescan from that open paren rather than from BOB.

If it makes no measurable difference for your case, I see two explanations:
- a bug in the patch makes it do something else than what I wanted.
- lossage rarely happens more than "once per top-level paren", so the
  cache is almost never re-used anyway.

But since in your case it's a single scan-lists call that's slow and
that call only skips one single "sexp", the second explanation seems
wrong: you do see several occurrences of lossage (which together cause
the slowdown) and yet this is all within the same top-level paren.


        Stefan

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[Alan Mackenzie]

Hi, Stefan.

On Wed, Jul 02, 2014 at 09:57:23PM -0400, Stefan Monnier wrote:
> > No.  The particular rogue invocation of scan-lists which caused this
> > (sub-)thread scans from BOB to near EOB 29 times.  Inside scan-lists,
> > _NO_ changes to syntax-tables, s-t text properties etc. are possible.
> > What I'm suggesting is that cacheing that first scan from BOB will be a
> > big win for many time-consuming scan-listses.

> Here's the situation: we can add some special code to handle your case.
> But adding code which will use syntax-ppss would be more beneficial for
> 99% of the major modes.

My suggestion is less radical, in that it leaves the functionality of
scan-lists unchanged.  It's speedup may be indistinguishable from the
strategy of simply using syntax-ppss.

> Adding a cache that only works within a single scan-lists instance is
> too much trouble for too little gain.

Modulo a binding to nil (or two), it could be the same code as that
which simply uses the existing syntax-ppss cache.  Without trying it, we
don't know how big or little the gain is.

> Adding a cache that is flushed less often means either it will suffer
> from the same limitations as syntax-ppss (so we may as well use
> syntax-ppss), or it needs to additionally hook into the text-property
> and set-syntax-table code to be flushed in those extra corner cases,
> and there again these extra checks will be only useful for those rare
> modes like CC-mode and will be at best harmless and in some cases
> detrimental to performance compared to simply using syntax-ppss.

Yes, to all of that, except I doubt the performance penalty in the last
bit would be noticeable (it'd be in C code, surely?).

> IOW, using syntax-ppss for find_defun_start is The Right Thing to do.

It is the route of least effort, perhaps, which is important.  It will
give suboptimal results for modes where syntax-ppss's basic assumptions
don't hold.  But this is also true of the exising scan-lists, so isn't,
perhaps, too important.  

As a matter of interest, is there an existing mechanism for calling lisp
from random bits of the C code?

> And CC-mode should then be fixed to take advantage of it.

It being syntax-ppss.  What CC Mode frequently needs to know is whether
a given position is in a comment, string, preprocessor construct, or
other code.  parse-partial-sexp and syntax-ppss don't give enough
information for this, hence aren't used for this directly.

A long term solution may be to enhance the syntax bits of Emacs to
recognise "syntactic islands" (e.g. C preprocessor construct, shell
script "here documents" or literate programming chunks) somehow.

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[Stefan Monnier]

> I'm not sure what I'm meant to see, but it has made no perceptible
> difference to the "slow" invocation of scan-lists.  In particular, that
> invocation took 0.7196781635284424 seconds with the patch, which is
> "exactly" (near enough) what it took before.

OK, the initialization needs to be after the "lossage:" label,
of course.  Try the patch below instead,


        Stefan


=== modified file 'src/syntax.c'
--- src/syntax.c	2014-02-08 05:12:47 +0000
+++ src/syntax.c	2014-07-04 19:42:50 +0000
@@ -530,17 +530,6 @@
 {
   ptrdiff_t opoint = PT, opoint_byte = PT_BYTE;
 
-  if (!open_paren_in_column_0_is_defun_start)
-    {
-      find_start_value = BEGV;
-      find_start_value_byte = BEGV_BYTE;
-      find_start_buffer = current_buffer;
-      find_start_modiff = MODIFF;
-      find_start_begv = BEGV;
-      find_start_pos = pos;
-      return BEGV;
-    }
-
   /* Use previous finding, if it's valid and applies to this inquiry.  */
   if (current_buffer == find_start_buffer
       /* Reuse the defun-start even if POS is a little farther on.
@@ -550,7 +539,20 @@
       && pos >= find_start_value
       && BEGV == find_start_begv
       && MODIFF == find_start_modiff)
+    {
+      if (!open_paren_in_column_0_is_defun_start)
+	fprintf (stderr, "Optimized lossage by %d (%d%%)!\n",
+		 find_start_value - BEGV,
+		 100 * (find_start_value - BEGV) / (pos - BEGV));
     return find_start_value;
+    }
+
+  if (!open_paren_in_column_0_is_defun_start)
+    {
+      find_start_value = BEGV;
+      find_start_value_byte = BEGV_BYTE;
+      goto found;
+    }
 
   /* Back up to start of line.  */
   scan_newline (pos, pos_byte, BEGV, BEGV_BYTE, -1, 1);
@@ -582,13 +584,14 @@
   /* Record what we found, for the next try.  */
   find_start_value = PT;
   find_start_value_byte = PT_BYTE;
+  TEMP_SET_PT_BOTH (opoint, opoint_byte);
+
+ found:
   find_start_buffer = current_buffer;
   find_start_modiff = MODIFF;
   find_start_begv = BEGV;
   find_start_pos = pos;
 
-  TEMP_SET_PT_BOTH (opoint, opoint_byte);
-
   return find_start_value;
 }
 \f
@@ -841,7 +844,9 @@
   else
     {
       struct lisp_parse_state state;
+      bool adjusted;
     lossage:
+      adjusted = false;
       /* We had two kinds of string delimiters mixed up
 	 together.  Decode this going forwards.
 	 Scan fwd from a known safe place (beginning-of-defun)
@@ -852,6 +857,8 @@
 	{
 	  defun_start = find_defun_start (comment_end, comment_end_byte);
 	  defun_start_byte = find_start_value_byte;
+	  if (defun_start != BEGV)
+	    adjusted = true;
 	}
       do
 	{
@@ -860,6 +867,16 @@
 			      comment_end, TYPE_MINIMUM (EMACS_INT),
 			      0, Qnil, 0);
 	  defun_start = comment_end;
+	  if (!adjusted)
+	    {
+	      adjusted = true;
+	      find_start_value
+		= CONSP (state.levelstarts) ? XINT (XCAR (state.levelstarts))
+		: state.thislevelstart >= 0 ? state.thislevelstart
+		: find_start_value;
+	      find_start_value_byte = CHAR_TO_BYTE (find_start_value);
+	    }
+
 	  if (state.incomment == (comnested ? 1 : -1)
 	      && state.comstyle == comstyle)
 	    from = state.comstr_start;

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[Stefan Monnier]

> A long term solution may be to enhance the syntax bits of Emacs to
> recognise "syntactic islands" (e.g. C preprocessor construct, shell
> script "here documents" or literate programming chunks) somehow.

Shell script's here-documents work just fine when considered
as comments.  Literate programming also works well when considering the
non-code as comments.
Maybe the same would hold of CPP thingies.


        Stefan

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[Stefan Monnier]

> OK, the initialization needs to be after the "lossage:" label,
> of course.  Try the patch below instead,

Well, I installed the patch since it does speed up my test case
noticeably (M-> followed by <prior> <prior> in src/xdisp.c).

I do have a question about open-paren-in-column-0-is-defun-start: why do
you let-bind it at various places instead of setting it buffer-locally
once and for all.
I see you had such a buffer-local setting but that you commented it out
in 2006/12/17.  Do you remember what was the reason for it?

From my naive point of view a buffer-local setting would not only be
simpler but would also be better for the user who could more easily set
it back to non-nil if she wanted to.


        Stefan

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[Alan Mackenzie]

Hi, Stefan.

On Fri, Jul 04, 2014 at 09:52:09PM -0400, Stefan Monnier wrote:
> > A long term solution may be to enhance the syntax bits of Emacs to
> > recognise "syntactic islands" (e.g. C preprocessor construct, shell
> > script "here documents" or literate programming chunks) somehow.

> Shell script's here-documents work just fine when considered
> as comments.  Literate programming also works well when considering the
> non-code as comments.
> Maybe the same would hold of CPP thingies.

Except that the "commented out" bits have their own syntactic structure.
Ideally, a shell script here document as argument to awk ought to be
parsed in AWK Mode.  The non-code bits of literate programming are also
likely to have their own structure.  CPP sections are handled as
C/C++/ObjC, but the complication caused by keeping them separate from
the main code are considerable.

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[Alan Mackenzie]

Hi, Stefan.

On Fri, Jul 04, 2014 at 03:43:02PM -0400, Stefan Monnier wrote:
> > I'm not sure what I'm meant to see, but it has made no perceptible
> > difference to the "slow" invocation of scan-lists.  In particular, that
> > invocation took 0.7196781635284424 seconds with the patch, which is
> > "exactly" (near enough) what it took before.

> OK, the initialization needs to be after the "lossage:" label,
> of course.  Try the patch below instead,

This works considerably better.  :-)  The critical scan-lists took
0.12084102630615234 seconds, a speedup of a factor of ~7.  It may be even
larger than that because of the instrumentation in the patch.

CC Mode has a problem in that it does too many backward scan-lists.
There is no documentation anywhere of what open-paren-in-column-0-etc
does; its sole effect is in backwards scan-lists which encounter
"syntactically unbalanced" comments, this causing a scan of the buffer
from point-min.  When I optimised c-parse-state a few years back, I was
under the impression that a backwards scan-lists wasn't much more
expensive than a forwards one.  Often, this is not true.  I should think
about adjusting c-parse-state again.  It surely wouldn't hurt for the
documentation of open-paren-etc and scan-lists to be a bit more
forthcoming about backwards scan-listses and what the variable _really_
does.

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[Stefan Monnier]

> from point-min.  When I optimised c-parse-state a few years back, I was
> under the impression that a backwards scan-lists wasn't much more
> expensive than a forwards one.  Often, this is not true.

Indeed.  Programming language syntax is designed to be parsed forward,
so parsing it backward can sometimes be surprisingly difficult.

> about adjusting c-parse-state again.  It surely wouldn't hurt for the
> documentation of open-paren-etc and scan-lists to be a bit more
> forthcoming about backwards scan-listses and what the variable _really_
> does.

I think open-paren-in-column-0-is-defun-start is on the way out, FWIW.
`syntax-ppss' makes is largely irrelevant.


        Stefan

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[Alan Mackenzie]

Hello, Stefan.

On Fri, Jul 04, 2014 at 10:23:07PM -0400, Stefan Monnier wrote:
> > OK, the initialization needs to be after the "lossage:" label,
> > of course.  Try the patch below instead,

> Well, I installed the patch since it does speed up my test case
> noticeably (M-> followed by <prior> <prior> in src/xdisp.c).

Excellent!

Hello, Martin, how far does this patch go, from your point of view, in
solving the sluggishness in xdisp.c?

> I do have a question about open-paren-in-column-0-is-defun-start: why do
> you let-bind it at various places instead of setting it buffer-locally
> once and for all.

Misunderstanding/lack of documentation.  My aim was to restrict the
effect of binding open-paren-etc to nil to its supposed "subsidiary"
effects in scan-lists, leaving the "main" higher level effects
unchanged from the user's setting (or lack thereof).

> I see you had such a buffer-local setting but that you commented it out
> in 2006/12/17.  Do you remember what was the reason for it?

I've had a look at the thread "Mysterious fontification/C++ context
issue - Patch for beginning-of-defun-raw." in emacs-devel which started
around 2006-12-04.  It's long and rambling and most of the posts are
heavily context dependent, making it difficult to follow.

> >>From my naive point of view a buffer-local setting would not only be
> simpler but would also be better for the user who could more easily set
> it back to non-nil if she wanted to.

How about going back to Martin's suggestion, and letting the user decide
on the variable's setting?  Trouble is, open-paren-etc's system-wide
default is t, whereas here it really wants to be nil - parens which just
happen to be in column 0 are not that uncommon.  The crude way would be
to introduce c-open-paren-in-column-0-is-defun-start, but this has
problems of its own.  How, in general, can we cleanly give a user option
a mode specific default?

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[Stefan Monnier]

> How about going back to Martin's suggestion, and letting the user decide
> on the variable's setting?

That's exactly why I'm bringing this up: what was wrong with setting it
buffer-locally in the major-mode function?

> problems of its own.  How, in general, can we cleanly give a user option
> a mode specific default?

As above.  If the user doesn't like this mode-provided setting, she could
easily override it with

   (add-hook 'c-mode-hook
             (lambda ()
               (setq-local open-paren-in-column-0-is-defun-start t)))


-- Stefan

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[martin rudalics]

 > Hello, Martin, how far does this patch go, from your point of view, in
 > solving the sluggishness in xdisp.c?

FWIW the improvement is considerable and the performance doesn't seem
worse now than with `open-paren-in-column-0-is-defun-start' non-nil.

So from what I can tell here at the moment this issue seems resolved.

Thanks to Stefan and you, martin

bug#16526: 24.3.50; scroll-conservatively & c-mode regression

[martin rudalics]

 > As above.  If the user doesn't like this mode-provided setting, she could
 > easily override it with
 >
 >     (add-hook 'c-mode-hook
 >               (lambda ()
 >                 (setq-local open-paren-in-column-0-is-defun-start t)))

My c-mode-hook contains

     (set (make-local-variable 'open-paren-in-column-0-is-defun-start) t)

but this ceased to work after Alan's changes.

martin