Nested sit-for's

Nested sit-for's

[Kim F. Storm]

Chong Yidong <cyd@stupidchicken.com> writes:

>> What about the change that we discussed where nested sit-for calls
>> should not wait longer than any of the outer calls??
>
> I haven't had a chance to spend much time on this project.  One
> problem is that I haven't managed to find a simple test case that
> clearly demonstrates the old behavior is broken.
>

Try this evaluating this:

(defun st1 ()
  (with-current-buffer (get-buffer-create "*st1*")
    (goto-char (point-max))
    (insert "<")
    (sit-for 30)
    (insert ">")))

(run-with-timer 1 2 'st1)

(progn
  (message "sit-for...")
  (sit-for 5)
  (message "sit-for...done"))

Now, the sit-for...done message is shown after 30-35 seconds,
not after 5 seconds...

[repeat the last progn if you don't see the effect immediately]


The call to sit-for in the timer is probably "bad practice", but it
could just as well have happened in a process filter or some other
async handler.

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: Nested sit-for's

[Chong Yidong]

storm@cua.dk (Kim F. Storm) writes:

> (defun st1 ()
>   (with-current-buffer (get-buffer-create "*st1*")
>     (goto-char (point-max))
>     (insert "<")
>     (sit-for 30)
>     (insert ">")))
>
> (run-with-timer 1 2 'st1)
>
> (progn
>   (message "sit-for...")
>   (sit-for 5)
>   (message "sit-for...done"))
>
> Now, the sit-for...done message is shown after 30-35 seconds,
> not after 5 seconds...
>
> The call to sit-for in the timer is probably "bad practice", but it
> could just as well have happened in a process filter or some other
> async handler.

How about the following patch?  The preemption code takes place
read_filtered_event, so it affects nested `sit-for's while leaving
unchanged other uses of wait_reading_process_output, such as the
sit_for() function called in a handful of places in the C code.  I am
unsure whether it is important/correct/desirable to preempt those
cases.  Opinions welcome.

There's one part of the patch that I think may be incorrect.  It saves
the seconds and microseconds components of the end time (originally an
EMACS_TIME) in two Lisp integers, which is probably not big enough.  I
think the solution is to introduce a few utility functions to convert
between EMACS_TIMEs and the Lisp-level (HIGH LOW USECS) time
representation.  (This may let us reimplement `timer-relative-time'
more straightforwardly too).

*** emacs/src/lread.c.~1.362.~	2006-07-26 13:45:54.000000000 -0400
--- emacs/src/lread.c	2006-08-16 14:46:50.000000000 -0400
***************
*** 439,444 ****
--- 439,457 ----
  
  extern Lisp_Object read_char ();
  
+ static EMACS_TIME saved_end_time;
+ 
+ static Lisp_Object
+ read_filtered_event_unwind (data)
+      Lisp_Object data;
+ {
+   if (!NILP (data))
+     EMACS_SET_SECS_USECS (saved_end_time,
+ 			  XINT (XCAR (data)),
+ 			  XINT (XCDR (data)));
+   return Qnil;
+ }
+ 
  /* Read input events until we get one that's acceptable for our purposes.
  
     If NO_SWITCH_FRAME is non-zero, switch-frame events are stashed
***************
*** 468,473 ****
--- 481,487 ----
  {
    Lisp_Object val, delayed_switch_frame;
    EMACS_TIME end_time;
+   int count = SPECPDL_INDEX ();
  
  #ifdef HAVE_WINDOW_SYSTEM
    if (display_hourglass_p)
***************
*** 488,493 ****
--- 502,522 ----
        EMACS_GET_TIME (end_time);
        EMACS_SET_SECS_USECS (wait_time, sec, usec);
        EMACS_ADD_TIME (end_time, end_time, wait_time);
+ 
+       if (!EMACS_TIME_NEG_P (saved_end_time)
+ 	  && EMACS_TIME_GE (end_time, saved_end_time))
+ 	{
+ 	  EMACS_SET_SECS  (end_time, EMACS_SECS  (saved_end_time));
+ 	  EMACS_SET_USECS (end_time, EMACS_USECS (saved_end_time));
+ 	}
+       else
+ 	{
+ 	  record_unwind_protect (read_filtered_event_unwind, 
+ 				 Fcons (make_number (EMACS_SECS (saved_end_time)),
+ 					make_number (EMACS_USECS (saved_end_time))));
+ 	  EMACS_SET_SECS  (saved_end_time, EMACS_SECS  (end_time));
+ 	  EMACS_SET_USECS (saved_end_time, EMACS_USECS (end_time));
+ 	}
      }
  
    /* Read until we get an acceptable event.  */
***************
*** 553,558 ****
--- 582,588 ----
  
  #endif
  
+   unbind_to (count, Qnil);
    return val;
  }
  
***************
*** 4230,4235 ****
--- 4260,4267 ----
  
    Vloads_in_progress = Qnil;
    staticpro (&Vloads_in_progress);
+ 
+   EMACS_SET_SECS_USECS (saved_end_time, -1, -1);
  }
  
  /* arch-tag: a0d02733-0f96-4844-a659-9fd53c4f414d

Re: Nested sit-for's

[Richard Stallman]

    >> What about the change that we discussed where nested sit-for calls
    >> should not wait longer than any of the outer calls??

I am not sure it is really a bug.  Whether this behavior is incorrect
depends on how you think of sit-for's purpose, and there is a natural
way to think of it which makes this behavior correct.  For the inner
sit-for fail to wait for the time specified seems clearly wrong.

My conclusion is that it is wrong for a timer to do a sit-for that
lasts any substantial time.  It should instead schedule a new timer.
As long as jit-lock-stealth-nice is a short period such as 0.5, its
sit-for cannot cause a big delay to anything else.

The potential problem I do see is that jit-lock-stealth-fontify will
keep looping as long as input-pending-p is nil.  If it were to run
from inside some other idle timer, that other idle timer would not get
control back until fontification is finished.  Making
jit-lock-stealth-fontify's sit-for return faster won't avoid this
problem, only reduce it, since jit-lock-stealth-fontify still would
not return until it finishes fontification.  The only solutions are
(1) that jit-lock-stealth-fontify reschedule itself instead of using
sit-for, or (2) that the other timer function avoid using sit-for.

If several timers try this sit-for trick, then no matter what we make
sit-for do, they can't all get the behavior they want, which is to do
some more processing at a certain time in the future.  The only method
they can all use that enables them all to get this behavior is that of
rescheduling timers.

It would work to have ONE timer that does sit-for if we make a rule
that no others can do so.  We could define jit-lock as this one
exception.  (This has the advantage of not involving any change in the
code, just comments and the Lisp Manual.)

What do people think of that?

Re: Nested sit-for's

[Kim F. Storm]

Richard Stallman <rms@gnu.org> writes:

>     >> What about the change that we discussed where nested sit-for calls
>     >> should not wait longer than any of the outer calls??
>
> I am not sure it is really a bug.  Whether this behavior is incorrect
> depends on how you think of sit-for's purpose, and there is a natural
> way to think of it which makes this behavior correct.  For the inner
> sit-for fail to wait for the time specified seems clearly wrong.
>
> My conclusion is that it is wrong for a timer to do a sit-for that
> lasts any substantial time.  It should instead schedule a new timer.
> As long as jit-lock-stealth-nice is a short period such as 0.5, its
> sit-for cannot cause a big delay to anything else.
>
> The potential problem I do see is that jit-lock-stealth-fontify will
> keep looping as long as input-pending-p is nil.  If it were to run
> from inside some other idle timer, that other idle timer would not get
> control back until fontification is finished.  Making
> jit-lock-stealth-fontify's sit-for return faster won't avoid this
> problem, only reduce it, since jit-lock-stealth-fontify still would
> not return until it finishes fontification.  The only solutions are
> (1) that jit-lock-stealth-fontify reschedule itself instead of using
> sit-for, or (2) that the other timer function avoid using sit-for.

> If several timers try this sit-for trick, then no matter what we make
> sit-for do, they can't all get the behavior they want, which is to do
> some more processing at a certain time in the future.  The only method
> they can all use that enables them all to get this behavior is that of
> rescheduling timers.
>
> It would work to have ONE timer that does sit-for if we make a rule
> that no others can do so.  We could define jit-lock as this one
> exception.  (This has the advantage of not involving any change in the
> code, just comments and the Lisp Manual.)
>
> What do people think of that?

I agree with your analysis.  

The "max sit-for" timeout hack may cause more problems than it solves,
so it is not TRT.

In general, timers should never use sit-for, so I think we should document
that in the manual.

But, IMO, if we make it a rule that timers should generally not use
sit-for, then a central function like jit-lock should definitely not
use sit-for!

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: Nested sit-for's

[David Kastrup]

storm@cua.dk (Kim F. Storm) writes:

> Richard Stallman <rms@gnu.org> writes:
>
>> It would work to have ONE timer that does sit-for if we make a rule
>> that no others can do so.  We could define jit-lock as this one
>> exception.  (This has the advantage of not involving any change in
>> the code, just comments and the Lisp Manual.)
>>
>> What do people think of that?
>
> I agree with your analysis.  
>
> In general, timers should never use sit-for, so I think we should
> document that in the manual.
>
> But, IMO, if we make it a rule that timers should generally not use
> sit-for, then a central function like jit-lock should definitely not
> use sit-for!

If we are in the temptation to let a central function like jit-lock
use sit-for in a timer, that means that there is a general perceived
need to do that.  So we should create a convenient way to do the
equivalent, document it, and use it ourselves.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: Nested sit-for's

[Richard Stallman]

    If we are in the temptation to let a central function like jit-lock
    use sit-for in a timer, that means that there is a general perceived
    need to do that.

That does not follow.  But perhaps it is not clear to me what
"that" refers to here.

		      So we should create a convenient way to do the
    equivalent, document it, and use it ourselves.

I can't tell whether "a way to do the equivalent" includes
what I have just done, or not.

Re: Nested sit-for's

[Chong Yidong]

storm@cua.dk (Kim F. Storm) writes:

>> It would work to have ONE timer that does sit-for if we make a rule
>> that no others can do so.  We could define jit-lock as this one
>> exception.  (This has the advantage of not involving any change in the
>> code, just comments and the Lisp Manual.)
>
> I agree with your analysis.  
>
> But, IMO, if we make it a rule that timers should generally not use
> sit-for, then a central function like jit-lock should definitely not
> use sit-for!

If we simply document that "timers (and process filters) should avoid
using sit-for", it should be clear to the reader that rare exceptions
may exist (especially if we add a comment to jit-lock-stealth-fontify
stating this).  After the release, we can probably rework
jit-lock-stealth-fontify to avoid using sit-for, but I don't think the
current situation is bad enough to block the release.

OTOH, I don't remember any other timers or process filters in the
Emacs tree that use a long sit-for or loop waiting for input.  Anyone
know of any?

Re: Nested sit-for's

[Kim F. Storm]

Chong Yidong <cyd@stupidchicken.com> writes:

> storm@cua.dk (Kim F. Storm) writes:
>
>>> It would work to have ONE timer that does sit-for if we make a rule
>>> that no others can do so.  We could define jit-lock as this one
>>> exception.  (This has the advantage of not involving any change in the
>>> code, just comments and the Lisp Manual.)
>>
>> I agree with your analysis.  
>>
>> But, IMO, if we make it a rule that timers should generally not use
>> sit-for, then a central function like jit-lock should definitely not
>> use sit-for!
>
> If we simply document that "timers (and process filters) should avoid
> using sit-for", it should be clear to the reader that rare exceptions
> may exist (especially if we add a comment to jit-lock-stealth-fontify
> stating this).  After the release, we can probably rework
> jit-lock-stealth-fontify to avoid using sit-for, but I don't think the
> current situation is bad enough to block the release.
>
> OTOH, I don't remember any other timers or process filters in the
> Emacs tree that use a long sit-for or loop waiting for input.  Anyone
> know of any?

[We have discussed this pb before, but never found a solution].

I don't know if it is related, but from time to time (at least a few
times daily), the cursor disappears, and doesn't return until I
do "something" (e.g. click the mouse or type a key).

I use blinking cursor, but when this happens, it typically seems to
happen immediately after some event that updates part of the screen
(like buffer switching), i.e. not as an effect of first showing the
cursor and then "blinking it off".  Examples where this happens:

1) returning to the group buffer in Gnus (after closing a summary buffer).
[this is where I've seen this most]

2) returning to the summary buffer after responding to an article.

But I also think I've seen it hiding the cursor after typing some
characters...

I don't know if it is related to timers at all, but it seems to
happen (marginally) more often now than before the recent sit-for
changes ...

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: Nested sit-for's

[Chong Yidong]

> I don't know if it is related, but from time to time (at least a few
> times daily), the cursor disappears, and doesn't return until I
> do "something" (e.g. click the mouse or type a key).
>
> I use blinking cursor, but when this happens, it typically seems to
> happen immediately after some event that updates part of the screen
> (like buffer switching), i.e. not as an effect of first showing the
> cursor and then "blinking it off".

If this is true, maybe it is a redisplay bug.

Could you try interrupting Emacs through gdb when this occurs, and
inspecting Vtimer_list to see if the `blink-cursor-timer-function'
timer is still active?

Another avenue of investigation is to add (redisplay) to
`blink-cursor-timer-function'.  If that stops the problem, that means
there is a redisplay bug.

I have unfortunately been unable to reproduce this bug.

Re: Nested sit-for's

[Kim F. Storm]

Chong Yidong <cyd@stupidchicken.com> writes:

>> I don't know if it is related, but from time to time (at least a few
>> times daily), the cursor disappears, and doesn't return until I
>> do "something" (e.g. click the mouse or type a key).
>>
>> I use blinking cursor, but when this happens, it typically seems to
>> happen immediately after some event that updates part of the screen
>> (like buffer switching), i.e. not as an effect of first showing the
>> cursor and then "blinking it off".
>
> If this is true, maybe it is a redisplay bug.

It may be so, but IIRC, when I last tried to debug this problem
(months ago), the cursor did actually blink (shortly) every
30-something seconds.

>
> Could you try interrupting Emacs through gdb when this occurs, and
> inspecting Vtimer_list to see if the `blink-cursor-timer-function'
> timer is still active?

I can't.  As soon as emacs loses focus, it shows the (hollow) cursor.

>
> Another avenue of investigation is to add (redisplay) to
> `blink-cursor-timer-function'.  If that stops the problem, that means
> there is a redisplay bug.

I don't know how to provoke the bug -- it happens quite rarely, and
I never found a way to reproduce it.   When I tried to debug this, I
did add some trace-dump to the timer routines in timer.el, but I never
really got anything which could explain what the problem was.

> I have unfortunately been unable to reproduce this bug.

So have I...  :-|   It just happens...

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: Nested sit-for's

[Chong Yidong]

>>> I don't know if it is related, but from time to time (at least a few
>>> times daily), the cursor disappears, and doesn't return until I
>>> do "something" (e.g. click the mouse or type a key).
>>>
>>> I use blinking cursor, but when this happens, it typically seems to
>>> happen immediately after some event that updates part of the screen
>>> (like buffer switching), i.e. not as an effect of first showing the
>>> cursor and then "blinking it off".

Another possible thing to check is whether the cursor is simply not
being drawn, or simply not being updated.  Maybe this might help?

  (setq blink-cursor-alist (list (cons t 'hollow)))

Re: Nested sit-for's

[martin rudalics]

[-- Attachment #1: Type: text/plain, Size: 505 bytes --]

 > If we simply document that "timers (and process filters) should avoid
 > using sit-for", it should be clear to the reader that rare exceptions
 > may exist (especially if we add a comment to jit-lock-stealth-fontify
 > stating this).  After the release, we can probably rework
 > jit-lock-stealth-fontify to avoid using sit-for, but I don't think the
 > current situation is bad enough to block the release.

If you accept a pre-command-hook the attached patch would be a first
step in that direction.

[-- Attachment #2: jit-lock.patch --]
[-- Type: text/plain, Size: 7871 bytes --]

*** jit-lock.el.~1.53.~	Tue Aug 15 10:00:50 2006
--- jit-lock.el	Thu Aug 17 17:52:02 2006
***************
*** 220,229 ****
  	 (jit-lock-refontify)

  	 ;; Install an idle timer for stealth fontification.
! 	 (when (and jit-lock-stealth-time (null jit-lock-stealth-timer))
! 	   (setq jit-lock-stealth-timer
! 		 (run-with-idle-timer jit-lock-stealth-time t
! 				      'jit-lock-stealth-fontify)))

  	 ;; Init deferred fontification timer.
  	 (when (and jit-lock-defer-time (null jit-lock-defer-timer))
--- 220,231 ----
  	 (jit-lock-refontify)

  	 ;; Install an idle timer for stealth fontification.
! 	 (when jit-lock-stealth-time
! 	   (add-hook 'pre-command-hook 'jit-lock-reset-stealth-buffers nil t)
! 	   (when (null jit-lock-stealth-timer)
! 	     (setq jit-lock-stealth-timer
! 		   (run-with-idle-timer
! 		    jit-lock-stealth-time t 'jit-lock-stealth-fontify))))

  	 ;; Init deferred fontification timer.
  	 (when (and jit-lock-defer-time (null jit-lock-defer-timer))
***************
*** 265,270 ****
--- 267,273 ----
  	     (setq jit-lock-defer-timer nil)))

  	 ;; Remove hooks.
+ 	 (remove-hook 'pre-command-hook 'jit-lock-reset-stealth-buffers t)
  	 (remove-hook 'after-change-functions 'jit-lock-after-change t)
  	 (remove-hook 'fontification-functions 'jit-lock-function))))

***************
*** 386,399 ****
             ;; eagerly extend the refontified region with
             ;; jit-lock-after-change-extend-region-functions.
             (when (< start orig-start)
!              (lexical-let ((start start)
                             (orig-start orig-start)
                             (buf (current-buffer)))
                 (run-with-timer
                  0 nil (lambda ()
!                         (with-buffer-prepared-for-jit-lock
!                             (put-text-property start orig-start
!                                                'fontified t buf))))))

  	   ;; Find the start of the next chunk, if any.
  	   (setq start (text-property-any next end 'fontified nil))))))))
--- 389,403 ----
             ;; eagerly extend the refontified region with
             ;; jit-lock-after-change-extend-region-functions.
             (when (< start orig-start)
! 	     (lexical-let ((start start)
                             (orig-start orig-start)
                             (buf (current-buffer)))
                 (run-with-timer
                  0 nil (lambda ()
! 			(with-current-buffer buf
! 			  (with-buffer-prepared-for-jit-lock
! 			   (put-text-property start orig-start
! 					      'fontified t)))))))

  	   ;; Find the start of the next chunk, if any.
  	   (setq start (text-property-any next end 'fontified nil))))))))
***************
*** 402,408 ****
  ;;; Stealth fontification.

  (defsubst jit-lock-stealth-chunk-start (around)
!   "Return the start of the next chunk to fontify around position AROUND..
  Value is nil if there is nothing more to fontify."
    (if (zerop (buffer-size))
        nil
--- 406,412 ----
  ;;; Stealth fontification.

  (defsubst jit-lock-stealth-chunk-start (around)
!   "Return the start of the next chunk to fontify around position AROUND.
  Value is nil if there is nothing more to fontify."
    (if (zerop (buffer-size))
        nil
***************
*** 442,513 ****
  			   (t next))))
  	result))))


  (defun jit-lock-stealth-fontify ()
    "Fontify buffers stealthily.
  This functions is called after Emacs has been idle for
  `jit-lock-stealth-time' seconds."
!   ;; I used to check `inhibit-read-only' here, but I can't remember why.  -stef
!   (unless (or executing-kbd-macro
  	      memory-full
  	      (window-minibuffer-p (selected-window)))
!     (let ((buffers (buffer-list))
! 	  (outer-buffer (current-buffer))
  	  minibuffer-auto-raise
! 	  message-log-max)
!       (with-local-quit
! 	(while (and buffers (not (input-pending-p)))
! 	  (with-current-buffer (pop buffers)
! 	    (when jit-lock-mode
! 	      ;; This is funny.  Calling sit-for with 3rd arg non-nil
! 	      ;; so that it doesn't redisplay, internally calls
! 	      ;; wait_reading_process_input also with a parameter
! 	      ;; saying "don't redisplay."  Since this function here
! 	      ;; is called periodically, this effectively leads to
! 	      ;; process output not being redisplayed at all because
! 	      ;; redisplay_internal is never called.  (That didn't
! 	      ;; work in the old redisplay either.)  So, we learn that
! 	      ;; we mustn't call sit-for that way here.  But then, we
! 	      ;; have to be cautious not to call sit-for in a widened
! 	      ;; buffer, since this could display hidden parts of that
! 	      ;; buffer.  This explains the seemingly weird use of
! 	      ;; save-restriction/widen here.
! 
! 	      (with-temp-message (if jit-lock-stealth-verbose
! 				     (concat "JIT stealth lock "
! 					     (buffer-name)))
! 
! 		;; In the following code, the `sit-for' calls cause a
! 		;; redisplay, so it's required that the
! 		;; buffer-modified flag of a buffer that is displayed
! 		;; has the right value---otherwise the mode line of
! 		;; an unmodified buffer would show a `*'.
! 		(let (start
! 		      (nice (or jit-lock-stealth-nice 0))
! 		      (point (point-min)))
! 		  (while (and (setq start
! 				    (jit-lock-stealth-chunk-start point))
! 			      ;; In case sit-for runs any timers,
! 			      ;; give them the expected current buffer.
! 			      (with-current-buffer outer-buffer
! 				(sit-for nice)))
! 
! 		    ;; fontify a block.
! 		    (jit-lock-fontify-now start (+ start jit-lock-chunk-size))
! 		    ;; If stealth jit-locking is done backwards, this leads to
! 		    ;; excessive O(n^2) refontification.   -stef
! 		    ;; (when (>= jit-lock-context-unfontify-pos start)
! 		    ;;   (setq jit-lock-context-unfontify-pos end))
! 
! 		    ;; Wait a little if load is too high.
! 		    (when (and jit-lock-stealth-load
! 			       (> (car (load-average)) jit-lock-stealth-load))
! 		      ;; In case sit-for runs any timers,
! 		      ;; give them the expected current buffer.
! 		      (with-current-buffer outer-buffer
! 			(sit-for (or jit-lock-stealth-time 30))))))))))))))
! 
! 
  \f
  ;;; Deferred fontification.

--- 446,492 ----
  			   (t next))))
  	result))))

+ (defvar jit-lock-stealth-buffers nil
+   "List of buffers that should be fontified stealthily.
+ Installed in `pre-command-hook' when `jit-lock-stealth-time' is non-nil
+ and JIT lock is turned on.")
+ 
+ (defun jit-lock-reset-stealth-buffers ()
+   "Reset `jit-lock-stealth-buffers' to nil."
+   (setq jit-lock-stealth-buffers nil))

  (defun jit-lock-stealth-fontify ()
    "Fontify buffers stealthily.
  This functions is called after Emacs has been idle for
  `jit-lock-stealth-time' seconds."
!   (unless (or (input-pending-p)
! 	      (eq jit-lock-stealth-buffers t)
! 	      executing-kbd-macro
  	      memory-full
  	      (window-minibuffer-p (selected-window)))
!     (unless jit-lock-stealth-buffers
!       (setq jit-lock-stealth-buffers (or (buffer-list) t)))
!     (let ((buffer (car jit-lock-stealth-buffers))
  	  minibuffer-auto-raise
! 	  message-log-max
! 	  start)
!       (when (and buffer
! 		 (or (not jit-lock-stealth-load)
! 		     (<= (car (load-average)) jit-lock-stealth-load)))
! 	(with-local-quit
! 	  (with-current-buffer buffer
! 	    (if (and jit-lock-mode
! 		     (setq start (jit-lock-stealth-chunk-start (point))))
! 		;; fontify a block.
! 		(with-temp-message (if jit-lock-stealth-verbose
! 				       (concat "JIT stealth lock "
! 					       (buffer-name)))
! 		  (jit-lock-fontify-now start (+ start jit-lock-chunk-size)))
! 	      (setq jit-lock-stealth-buffers
! 		    (or (cdr jit-lock-stealth-buffers) t)))))))
!     (unless (eq jit-lock-stealth-buffers t)
!       (run-at-time (min jit-lock-stealth-nice 0.1) nil
! 		   'jit-lock-stealth-fontify))))
  \f
  ;;; Deferred fontification.


[-- Attachment #3: Type: text/plain, Size: 142 bytes --]

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-devel

Re: Nested sit-for's

[Kim F. Storm]

martin rudalics <rudalics@gmx.at> writes:

>> If we simply document that "timers (and process filters) should avoid
>> using sit-for", it should be clear to the reader that rare exceptions
>> may exist (especially if we add a comment to jit-lock-stealth-fontify
>> stating this).  After the release, we can probably rework
>> jit-lock-stealth-fontify to avoid using sit-for, but I don't think the
>> current situation is bad enough to block the release.
>
> If you accept a pre-command-hook the attached patch would be a first
> step in that direction.

Is the pre-command-hook installed permanently, or only when jit-lock is
active?

IMHO, using a permanently installed pre-command-hook is not acceptable, but
it may be ok if only used temporarily...

But why is this needed at all?

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: Nested sit-for's

[martin rudalics]

[-- Attachment #1: Type: text/plain, Size: 283 bytes --]

> Is the pre-command-hook installed permanently, or only when jit-lock is
> active?
> 
> IMHO, using a permanently installed pre-command-hook is not acceptable, but
> it may be ok if only used temporarily...
> 
> But why is this needed at all?

Just a first shot.  Try the revision.

[-- Attachment #2: jit-lock.patch --]
[-- Type: text/plain, Size: 6596 bytes --]

*** jit-lock.el.~1.53.~	Tue Aug 15 10:00:50 2006
--- jit-lock.el	Fri Aug 18 10:57:54 2006
***************
*** 386,399 ****
             ;; eagerly extend the refontified region with
             ;; jit-lock-after-change-extend-region-functions.
             (when (< start orig-start)
!              (lexical-let ((start start)
                             (orig-start orig-start)
                             (buf (current-buffer)))
                 (run-with-timer
                  0 nil (lambda ()
!                         (with-buffer-prepared-for-jit-lock
!                             (put-text-property start orig-start
!                                                'fontified t buf))))))

  	   ;; Find the start of the next chunk, if any.
  	   (setq start (text-property-any next end 'fontified nil))))))))
--- 386,400 ----
             ;; eagerly extend the refontified region with
             ;; jit-lock-after-change-extend-region-functions.
             (when (< start orig-start)
! 	     (lexical-let ((start start)
                             (orig-start orig-start)
                             (buf (current-buffer)))
                 (run-with-timer
                  0 nil (lambda ()
! 			(with-current-buffer buf
! 			  (with-buffer-prepared-for-jit-lock
! 			   (put-text-property start orig-start
! 					      'fontified t)))))))

  	   ;; Find the start of the next chunk, if any.
  	   (setq start (text-property-any next end 'fontified nil))))))))
***************
*** 402,408 ****
  ;;; Stealth fontification.

  (defsubst jit-lock-stealth-chunk-start (around)
!   "Return the start of the next chunk to fontify around position AROUND..
  Value is nil if there is nothing more to fontify."
    (if (zerop (buffer-size))
        nil
--- 403,409 ----
  ;;; Stealth fontification.

  (defsubst jit-lock-stealth-chunk-start (around)
!   "Return the start of the next chunk to fontify around position AROUND.
  Value is nil if there is nothing more to fontify."
    (if (zerop (buffer-size))
        nil
***************
*** 442,513 ****
  			   (t next))))
  	result))))


! (defun jit-lock-stealth-fontify ()
    "Fontify buffers stealthily.
! This functions is called after Emacs has been idle for
  `jit-lock-stealth-time' seconds."
!   ;; I used to check `inhibit-read-only' here, but I can't remember why.  -stef
!   (unless (or executing-kbd-macro
  	      memory-full
! 	      (window-minibuffer-p (selected-window)))
!     (let ((buffers (buffer-list))
! 	  (outer-buffer (current-buffer))
  	  minibuffer-auto-raise
! 	  message-log-max)
!       (with-local-quit
! 	(while (and buffers (not (input-pending-p)))
! 	  (with-current-buffer (pop buffers)
! 	    (when jit-lock-mode
! 	      ;; This is funny.  Calling sit-for with 3rd arg non-nil
! 	      ;; so that it doesn't redisplay, internally calls
! 	      ;; wait_reading_process_input also with a parameter
! 	      ;; saying "don't redisplay."  Since this function here
! 	      ;; is called periodically, this effectively leads to
! 	      ;; process output not being redisplayed at all because
! 	      ;; redisplay_internal is never called.  (That didn't
! 	      ;; work in the old redisplay either.)  So, we learn that
! 	      ;; we mustn't call sit-for that way here.  But then, we
! 	      ;; have to be cautious not to call sit-for in a widened
! 	      ;; buffer, since this could display hidden parts of that
! 	      ;; buffer.  This explains the seemingly weird use of
! 	      ;; save-restriction/widen here.
! 
  	      (with-temp-message (if jit-lock-stealth-verbose
  				     (concat "JIT stealth lock "
  					     (buffer-name)))
! 
! 		;; In the following code, the `sit-for' calls cause a
! 		;; redisplay, so it's required that the
! 		;; buffer-modified flag of a buffer that is displayed
! 		;; has the right value---otherwise the mode line of
! 		;; an unmodified buffer would show a `*'.
! 		(let (start
! 		      (nice (or jit-lock-stealth-nice 0))
! 		      (point (point-min)))
! 		  (while (and (setq start
! 				    (jit-lock-stealth-chunk-start point))
! 			      ;; In case sit-for runs any timers,
! 			      ;; give them the expected current buffer.
! 			      (with-current-buffer outer-buffer
! 				(sit-for nice)))
! 
! 		    ;; fontify a block.
! 		    (jit-lock-fontify-now start (+ start jit-lock-chunk-size))
! 		    ;; If stealth jit-locking is done backwards, this leads to
! 		    ;; excessive O(n^2) refontification.   -stef
! 		    ;; (when (>= jit-lock-context-unfontify-pos start)
! 		    ;;   (setq jit-lock-context-unfontify-pos end))
! 
! 		    ;; Wait a little if load is too high.
! 		    (when (and jit-lock-stealth-load
! 			       (> (car (load-average)) jit-lock-stealth-load))
! 		      ;; In case sit-for runs any timers,
! 		      ;; give them the expected current buffer.
! 		      (with-current-buffer outer-buffer
! 			(sit-for (or jit-lock-stealth-time 30))))))))))))))
! 
! 
  \f
  ;;; Deferred fontification.

--- 443,487 ----
  			   (t next))))
  	result))))

+ (defvar jit-lock-stealth-buffers nil
+   "List of buffers that should be fontified stealthily.")

! (defun jit-lock-stealth-fontify (&optional repeat)
    "Fontify buffers stealthily.
! This function is called repeatedly after Emacs has been idle for
  `jit-lock-stealth-time' seconds."
!   (unless (or (input-pending-p)
! 	      executing-kbd-macro
  	      memory-full
! 	      (window-minibuffer-p (selected-window))
! 	      (null (if repeat
! 			jit-lock-stealth-buffers
! 		      (setq jit-lock-stealth-buffers (buffer-list)))))
!     (let ((buffer (car jit-lock-stealth-buffers))
  	  minibuffer-auto-raise
! 	  message-log-max
! 	  start load-delay)
!       (when (or (not jit-lock-stealth-load)
! 		(or (<= (car (load-average)) jit-lock-stealth-load)
! 		    (not (setq load-delay t))))
! 	(with-current-buffer buffer
! 	  (if (and jit-lock-mode
! 		   (setq start (jit-lock-stealth-chunk-start (point))))
! 	      ;; Fontify one block.
  	      (with-temp-message (if jit-lock-stealth-verbose
  				     (concat "JIT stealth lock "
  					     (buffer-name)))
! 		(jit-lock-fontify-now start (+ start jit-lock-chunk-size)))
! 	    ;; Nothing to fontify.
! 	    (setq jit-lock-stealth-buffers (cdr jit-lock-stealth-buffers)))))
!       (when jit-lock-stealth-buffers
! 	;; The 0.1s are completely arbitrary here and should be replaced by
! 	;; something more appropriate.
! 	(run-at-time
! 	 (if load-delay
! 	     (min jit-lock-stealth-time 0.1)
! 	   (min jit-lock-stealth-nice 0.1))
! 	 nil 'jit-lock-stealth-fontify t)))))
  \f
  ;;; Deferred fontification.


[-- Attachment #3: Type: text/plain, Size: 142 bytes --]

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-devel

Re: Nested sit-for's

[Kim F. Storm]

martin rudalics <rudalics@gmx.at> writes:

>> Is the pre-command-hook installed permanently, or only when jit-lock is
>> active?
>>
>> IMHO, using a permanently installed pre-command-hook is not acceptable, but
>> it may be ok if only used temporarily...
>>
>> But why is this needed at all?
>
> Just a first shot.  Try the revision.

My initial testing/impression:  Works like a charm!  Thanks!

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: Nested sit-for's

[Chong Yidong]

>> Just a first shot.  Try the revision.
>
> My initial testing/impression:  Works like a charm!  Thanks!

Does it have any effect on the cursor blinking problem?

Re: Nested sit-for's

[Kim F. Storm]

Chong Yidong <cyd@stupidchicken.com> writes:

>>> Just a first shot.  Try the revision.
>>
>> My initial testing/impression:  Works like a charm!  Thanks!
>
> Does it have any effect on the cursor blinking problem?

I haven't seen it since I applied that patch ... but it is still too early
to make any firm conclusions.

IIUC, the recent addition of current-idle-time by RMS was supposed to
fix the sit-for in jit-lock problem in a different way.  But so far,
RMS' changes to jit-lock have not been installed...   What's up???

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: Nested sit-for's

[martin rudalics]

 > IIUC, the recent addition of current-idle-time by RMS was supposed to
 > fix the sit-for in jit-lock problem in a different way.  But so far,
 > RMS' changes to jit-lock have not been installed...   What's up???

IIUC, `current-idle-time' would permit to replace the ugliness of my
`run-at-time' with yet another `run-with-idle-timer'.  Hopefully.

Re: Nested sit-for's

[Kim F. Storm]

martin rudalics <rudalics@gmx.at> writes:

>> IIUC, the recent addition of current-idle-time by RMS was supposed to
>> fix the sit-for in jit-lock problem in a different way.  But so far,
>> RMS' changes to jit-lock have not been installed...   What's up???
>
> IIUC, `current-idle-time' would permit to replace the ugliness of my
> `run-at-time' with yet another `run-with-idle-timer'.  Hopefully.

Sounds plausible.  So I hope RMS will commit his jit-lock fix soon!

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: Nested sit-for's

[Richard Stallman]

    IIUC, the recent addition of current-idle-time by RMS was supposed to
    fix the sit-for in jit-lock problem in a different way.  But so far,
    RMS' changes to jit-lock have not been installed...   What's up???

Someone else proposed a different change to the same part of jit-lock,
and I have not had time (while sufficiently alert) to study that,
so I don't have any idea which solution is better.

I sent my changes to the list yesterday; what do others think about
them?

Re: Nested sit-for's

[Kim F. Storm]

Richard Stallman <rms@gnu.org> writes:

>     IIUC, the recent addition of current-idle-time by RMS was supposed to
>     fix the sit-for in jit-lock problem in a different way.  But so far,
>     RMS' changes to jit-lock have not been installed...   What's up???
>
> Someone else proposed a different change to the same part of jit-lock,
> and I have not had time (while sufficiently alert) to study that,
> so I don't have any idea which solution is better.
>
> I sent my changes to the list yesterday; what do others think about
> them?

I haven't fully understood either of the two approaches, but Martin's
approach which completely avoids using sit-for in the timer handler
looks vastly superior to me for that reason alone!

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: Nested sit-for's

[Stefan Monnier]

>> IIUC, the recent addition of current-idle-time by RMS was supposed to
>> fix the sit-for in jit-lock problem in a different way.  But so far,
>> RMS' changes to jit-lock have not been installed...   What's up???
>> 
>> Someone else proposed a different change to the same part of jit-lock,
>> and I have not had time (while sufficiently alert) to study that,
>> so I don't have any idea which solution is better.
>> 
>> I sent my changes to the list yesterday; what do others think about
>> them?

> I haven't fully understood either of the two approaches, but Martin's
> approach which completely avoids using sit-for in the timer handler
> looks vastly superior to me for that reason alone!

I think we could clean up the code even more by extending
run-with-idle-timer as follows:

  (run-with-idle-timer SECS REPEAT FUNCTION &rest ARGS)

   Perform an action the next time Emacs is idle for SECS seconds.
   The action is to call function with arguments ARGS.
   SECS may be an integer or a floating point number.

   If REPEAT is non-nil, do the action each time Emacs has been idle for
   exactly SECS seconds (that is, only once for each time Emacs becomes idle).

   Additionally, if REPEAT is a number, repeat the action every REPEAT
   seconds as long as Emacs stays idle.

   This function returns a timer object which you can use in `cancel-timer'.


-- Stefan

Re: Nested sit-for's

[martin rudalics]

 >    Additionally, if REPEAT is a number, repeat the action every REPEAT
 >    seconds as long as Emacs stays idle.

That's exactly what I hoped for in the first place.

However, in `jit-lock-stealth-fontify' I need different REPEAT
intervals: `jit-lock-stealth-time' if the load is too high and
`jit-lock-stealth-nice' otherwise.

Anyway, I think that would be a _very_ pleasant feature.

Re: Nested sit-for's

[Stefan Monnier]

>> Additionally, if REPEAT is a number, repeat the action every REPEAT
>> seconds as long as Emacs stays idle.

> That's exactly what I hoped for in the first place.

> However, in `jit-lock-stealth-fontify' I need different REPEAT
> intervals: `jit-lock-stealth-time' if the load is too high and
> `jit-lock-stealth-nice' otherwise.

> Anyway, I think that would be a _very_ pleasant feature.

Then, maybe a generic background.el package which provides a function
`run-in-the-background'.  That function can then take care to run those
background tasks when Emacs is idle, taking average-load into account.


        Stefan

Re: Nested sit-for's

[Richard Stallman]

    I think we could clean up the code even more by extending
    run-with-idle-timer as follows:

      (run-with-idle-timer SECS REPEAT FUNCTION &rest ARGS)

       Perform an action the next time Emacs is idle for SECS seconds.
       The action is to call function with arguments ARGS.
       SECS may be an integer or a floating point number.

       If REPEAT is non-nil, do the action each time Emacs has been idle for
       exactly SECS seconds (that is, only once for each time Emacs becomes idle).

       Additionally, if REPEAT is a number, repeat the action every REPEAT
       seconds as long as Emacs stays idle.

That feature could be useful for some cases, but unconditional
repetition isn't quite what we want for the jit-lock case.

So let's not add it now.

Re: Nested sit-for's

[Chong Yidong]

Richard Stallman <rms@gnu.org> writes:

> The potential problem I do see is that jit-lock-stealth-fontify will
> keep looping as long as input-pending-p is nil.  If it were to run
> from inside some other idle timer, that other idle timer would not get
> control back until fontification is finished.

In the case where (load-average) is too high, the idle timer may not
get control back until up to 30 seconds later:

    ;; Wait a little if load is too high.
    (when (and jit-lock-stealth-load
	       (> (car (load-average)) jit-lock-stealth-load))
      ;; In case sit-for runs any timers,
      ;; give them the expected current buffer.
      (with-current-buffer outer-buffer
	(sit-for (or jit-lock-stealth-time 30))))))))))))))

Re: Nested sit-for's

[Richard Stallman]

    In the case where (load-average) is too high, the idle timer may not
    get control back until up to 30 seconds later:

We certainly don't want this timer function, as currently written, to
run from within another timer.

I just tried writing the code to eliminate this loop and make the
timer reschedule itself.  The only hard part was that there is no way
for an idle timer to arrange to run something after a few seconds more
of idleness.  To enable that, I added a function current-idle-time.
With that, it is easy to write the code for jit-lock to avoid looping.

Which means we can simply say that timer functions should not call
sit-for.

I think some timer functions will still need to call
accept-process-output, but we can say they are supposed to call it in
the way that doesn't run timers.

Does anyone think that will cause a problem?

Re: Nested sit-for's

[Juanma Barranquero]

(From an old thread)

On 8/18/06, Richard Stallman <rms@gnu.org> wrote:

> To enable that, I added a function current-idle-time.

Is there any reason for `current-idle-time' returning nil when Emacs
is not idle, instead of (0 0 0)?

That return value is not documented, and though there are time
functions that understand nil as (0 0 0):

 ELISP> (timer-set-time (timer-create) nil)
 [t nil nil 0 nil nil nil nil]

there are others that do not:

 ELISP> (time-add (current-idle-time) '(0 0 0))
 *** Eval error ***  Wrong type argument: number-or-marker-p, nil

             Juanma

Re: Nested sit-for's

[Richard Stallman]

    > To enable that, I added a function current-idle-time.

    Is there any reason for `current-idle-time' returning nil when Emacs
    is not idle, instead of (0 0 0)?

I think it is convenient for it to return nil to indicate
"not idle", and if you wish it were (0 0 0), it is easy to
write (or ... '(0 0 0)).

Re: Nested sit-for's

[Juanma Barranquero]

On 10/18/07, Richard Stallman <rms@gnu.org> wrote:

> I think it is convenient for it to return nil to indicate
> "not idle", and if you wish it were (0 0 0), it is easy to
> write (or ... '(0 0 0)).

AFAICS, current-idle-time is used exactly once in the Emacs sources
(in jit-lock.el), so I'm not entirely sure what convenience are you
talking about; to my eyes it just adds a tiny bit of unneeded
complexity to the function's interface...

...But anyway, if you are certain that is the right interface, we
should at least document it. The fact that the function could return
nil is mentioned neither in the docstring nor the "Idle Timers" node
of the Elisp reference.

             Juanma

Re: Nested sit-for's

[Richard Stallman]

    AFAICS, current-idle-time is used exactly once in the Emacs sources
    (in jit-lock.el), so I'm not entirely sure what convenience are you
    talking about; to my eyes it just adds a tiny bit of unneeded
    complexity to the function's interface...

I think it is good to make it easy to test whether Emacs is idle,
given that the function already exists and does this.  So I will document
the current behavior when Emacs is not idle.

As far as I can see, jit-lock-stealth-fontify is only
called when Emacs is idle, so it does not need to be changed.

RE: Nested sit-for's

[Drew Adams]

    It would work to have ONE timer that does sit-for if we make a rule
    that no others can do so.  We could define jit-lock as this one
    exception.  (This has the advantage of not involving any change in the
    code, just comments and the Lisp Manual.)

    What do people think of that?

Caveat: I'm ignorant on this question, and I haven't been following the
thread closely.

It occurred to me that if you want only one timer to do something like this,
and you want to always use that timer for this functionality, then a special
timer should be designated specifically for that, and it should not have
"jit-lock" in its name.

This would make things (e.g. doc) clearer. Let jit-lock use the special
timer, but just don't name it "jit-lock".

Again, ignore if I'm missing the point here.