* sit-for and idle timers @ 2006-08-11 19:48 Noah Friedman 2006-08-11 21:00 ` Chong Yidong ` (2 more replies) 0 siblings, 3 replies; 10+ messages in thread From: Noah Friedman @ 2006-08-11 19:48 UTC (permalink / raw) The change to sit-for of 2006-07-26 ("Use new SECONDS arg of read-event instead of a timer") seems to cause problems with idle-timers which call sit-for. The problem is that read-event's call tree ultimately results in calling keyboard.c:read_char, which calls timer_start_idle. This resets the activation time for all the current idle timer events, which means that any function on an idle timer which calls sit-for is now getting scheduled to be run recursively if another interval of the appropriate length ensues. I imagine this can continue until max-lisp-eval-depth is reached. Here's a small test case which demonstrates the problem: (defvar itimer-test-wait 0.5) (defvar itimer-test-depth 0) (defun itimer-test () (setq itimer-test-depth (1+ itimer-test-depth)) (unwind-protect (let ((flag nil)) (while (sit-for itimer-test-wait) (setq flag (not flag)) (message "itimer-test-depth: %-3d%s" itimer-test-depth (if flag " (blink)" "")))) (setq itimer-test-depth (1- itimer-test-depth)) (message "itimer-test-depth: %d" itimer-test-depth))) (run-with-idle-timer 0.25 t 'itimer-test) In the pre-7/26 implementation, this timer should never print a depth greater than 1, and the "(blink)" text should blink on and off with a regular rhythm. In the post-7/26 implementation, itimer-test-depth increments indefinitely until an event is read. One solution to this problem is to bind timer-idle-list to nil while calling read-event. I tested this trivially with a small defadvice: (defadvice read-event (around idlefrob activate) (let ((timer-idle-list nil)) ad-do-it)) I'm not sure if this (binding the variable in sit-for that is, not the use of defadvice as a temporary kludge) is the correct solution since it will preempt the activation of other idle timers. But it's simple. The less simple solution is probably for sit-for to go back to using a timer itself. Thoughts? ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: sit-for and idle timers 2006-08-11 19:48 sit-for and idle timers Noah Friedman @ 2006-08-11 21:00 ` Chong Yidong 2006-08-14 18:34 ` Chong Yidong 2006-08-14 19:20 ` Richard Stallman 2 siblings, 0 replies; 10+ messages in thread From: Chong Yidong @ 2006-08-11 21:00 UTC (permalink / raw) Cc: emacs-devel Noah Friedman <friedman@splode.com> writes: > The less simple solution is probably for sit-for to go back to using > a timer itself. The reason for not using a timer is so that when a process filter runs during the sit-for, the expiry of the sit-for won't interrupt the filter (which might be waiting for keyboard input). Otherwise, process filters can't reliably call input reading functions. See the thread labelled "a bug of read-passwd" from July. > One solution to this problem is to bind timer-idle-list to nil while > calling read-event. This would inhibit all other idle timers from running during the sit-for. But jit-lock-stealth-fontify, for example, sits for 30 seconds, and I don't think we want to inhibit other idle timers during this entire period. > The problem is that read-event's call tree ultimately results in > calling keyboard.c:read_char, which calls timer_start_idle. This > resets the activation time for all the current idle timer events, > which means that any function on an idle timer which calls sit-for > is now getting scheduled to be run recursively if another interval > of the appropriate length ensues. I think the solution is to avoid calling timer_start_idle when read-event is given a non-nil SECONDS argument. What do people think? ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: sit-for and idle timers 2006-08-11 19:48 sit-for and idle timers Noah Friedman 2006-08-11 21:00 ` Chong Yidong @ 2006-08-14 18:34 ` Chong Yidong 2006-08-14 19:20 ` Richard Stallman 2 siblings, 0 replies; 10+ messages in thread From: Chong Yidong @ 2006-08-14 18:34 UTC (permalink / raw) Cc: emacs-devel Noah Friedman <friedman@splode.com> writes: > The change to sit-for of 2006-07-26 ("Use new SECONDS arg of read-event > instead of a timer") seems to cause problems with idle-timers > which call sit-for. This should now be fixed. > Here's a small test case which demonstrates the problem: > > (defvar itimer-test-wait 0.5) > (defvar itimer-test-depth 0) > > (defun itimer-test () > (setq itimer-test-depth (1+ itimer-test-depth)) > (unwind-protect > (let ((flag nil)) > (while (sit-for itimer-test-wait) > (setq flag (not flag)) > (message "itimer-test-depth: %-3d%s" > itimer-test-depth > (if flag " (blink)" "")))) > (setq itimer-test-depth (1- itimer-test-depth)) > (message "itimer-test-depth: %d" itimer-test-depth))) > > (run-with-idle-timer 0.25 t 'itimer-test) > > In the pre-7/26 implementation, this timer should never print a depth > greater than 1, and the "(blink)" text should blink on and off with a > regular rhythm. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: sit-for and idle timers 2006-08-11 19:48 sit-for and idle timers Noah Friedman 2006-08-11 21:00 ` Chong Yidong 2006-08-14 18:34 ` Chong Yidong @ 2006-08-14 19:20 ` Richard Stallman 2006-08-14 19:47 ` Chong Yidong 2 siblings, 1 reply; 10+ messages in thread From: Richard Stallman @ 2006-08-14 19:20 UTC (permalink / raw) Cc: cyd, emacs-devel The change to sit-for of 2006-07-26 ("Use new SECONDS arg of read-event instead of a timer") seems to cause problems with idle-timers which call sit-for. Why do these timer functions call sit-for? It is a strange thing for a timer to wait. It should reschedule itself instead. What are they really trying to do? > The problem is that read-event's call tree ultimately results in > calling keyboard.c:read_char, which calls timer_start_idle. This > resets the activation time for all the current idle timer events, > which means that any function on an idle timer which calls sit-for > is now getting scheduled to be run recursively if another interval > of the appropriate length ensues. I think the solution is to avoid calling timer_start_idle when read-event is given a non-nil SECONDS argument. What do people think? That is definitely not right. Emacs really is idle when it reads an event, even if there is a timeout. The way to find the right fix is to ask WHY the current behavior is wrong. Calling timer_start_idle is appropriate when Emacs changes from non-idle to idle. It is wrong to call that function when Emacs is already idle. So my conclusion is that when read-event is called from an idle timer, it should not change the state to idle at the beginning, and it should not change the state away from idle at the end. Does this change fix it? *** keyboard.c 12 Aug 2006 17:31:29 -0400 1.866 --- keyboard.c 14 Aug 2006 03:48:20 -0400 *************** *** 2416,2421 **** --- 2416,2424 ---- volatile int reread; struct gcpro gcpro1, gcpro2; int polling_stopped_here = 0; + /* If we are already in the idle state, for instance if we + are calling from an idle timer, stay idle when we exit. */ + int already_idle = ! EMACS_TIME_NEG_P (timer_idleness_start_time); also_record = Qnil; *************** *** 2679,2685 **** goto non_reread; } ! timer_start_idle (); /* If in middle of key sequence and minibuffer not active, start echoing if enough time elapses. */ --- 2682,2689 ---- goto non_reread; } ! if (! already_idle) ! timer_start_idle (); /* If in middle of key sequence and minibuffer not active, start echoing if enough time elapses. */ *************** *** 2749,2755 **** c = read_char_x_menu_prompt (nmaps, maps, prev_event, used_mouse_menu); /* Now that we have read an event, Emacs is not idle. */ ! timer_stop_idle (); goto exit; } --- 2753,2760 ---- c = read_char_x_menu_prompt (nmaps, maps, prev_event, used_mouse_menu); /* Now that we have read an event, Emacs is not idle. */ ! if (!already_idle) ! timer_stop_idle (); goto exit; } *************** *** 2931,2937 **** non_reread: ! timer_stop_idle (); RESUME_POLLING; if (NILP (c)) --- 2936,2943 ---- non_reread: ! if (!already_idle) ! timer_stop_idle (); RESUME_POLLING; if (NILP (c)) *************** *** 2970,2976 **** prevents automatic window selection (under mouse_autoselect_window from acting as a real input event, for example banishing the mouse under mouse-avoidance-mode. */ ! timer_resume_idle (); /* Resume allowing input from any kboard, if that was true before. */ if (!was_locked) --- 2976,2983 ---- prevents automatic window selection (under mouse_autoselect_window from acting as a real input event, for example banishing the mouse under mouse-avoidance-mode. */ ! if (!already_idle) ! timer_resume_idle (); /* Resume allowing input from any kboard, if that was true before. */ if (!was_locked) *************** *** 3170,3177 **** show_help_echo (help, window, object, position, 0); ! /* We stopped being idle for this event; undo that. */ ! timer_resume_idle (); goto retry; } --- 3177,3185 ---- show_help_echo (help, window, object, position, 0); ! /* If we stopped being idle for this event, undo that. */ ! if (!already_idle) ! timer_resume_idle (); goto retry; } ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: sit-for and idle timers 2006-08-14 19:20 ` Richard Stallman @ 2006-08-14 19:47 ` Chong Yidong 2006-08-14 20:05 ` Chong Yidong 0 siblings, 1 reply; 10+ messages in thread From: Chong Yidong @ 2006-08-14 19:47 UTC (permalink / raw) Cc: emacs-devel, Noah Friedman > Why do these timer functions call sit-for? It is a strange thing for > a timer to wait. It should reschedule itself instead. What are they > really trying to do? It varies from situation to situation. For example, jit-lock uses the idle timer function jit-lock-stealth-fontify, which calls sit-for. The goal, in that case, is to wait a certain amount of time between fontifying chunks. > I think the solution is to avoid calling timer_start_idle when > read-event is given a non-nil SECONDS argument. What do people think? > > That is definitely not right. Emacs really is idle when it reads an event, > even if there is a timeout. > > So my conclusion is that when read-event is called from an idle timer, > it should not change the state to idle at the beginning, and it should > not change the state away from idle at the end. > > Does this change fix it? I already checked in a different change to keyboard.c before your email came in. But I think your version makes more sense. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: sit-for and idle timers 2006-08-14 19:47 ` Chong Yidong @ 2006-08-14 20:05 ` Chong Yidong 2006-08-15 12:41 ` Richard Stallman 0 siblings, 1 reply; 10+ messages in thread From: Chong Yidong @ 2006-08-14 20:05 UTC (permalink / raw) Cc: Noah Friedman, emacs-devel >> That is definitely not right. Emacs really is idle when it reads an event, >> even if there is a timeout. >> >> So my conclusion is that when read-event is called from an idle timer, >> it should not change the state to idle at the beginning, and it should >> not change the state away from idle at the end. > > I already checked in a different change to keyboard.c before your > email came in. But I think your version makes more sense. I just thought of one inconsistency, though. Suppose Emacs is not idle, and is running Lisp code that calls sit-for. With your change, this starts idle timers: ! if (! already_idle) ! timer_start_idle (); This is inconsistent with the old (built-in) behavior of sit-for, which did not activate idle timers while waiting either. The reasoning, I think, is that `sit-for' means for the Lisp code to "spin" for that period of time or until input arrives, which is not the same as idling. (This is similar to why `sleep-for' does not run idle timers.) If this is the intended behavior, then read-event with a timeout should not start idle timers, since it only exists to be called by sit-for. We could simply document this behavior in the lispref manual. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: sit-for and idle timers 2006-08-14 20:05 ` Chong Yidong @ 2006-08-15 12:41 ` Richard Stallman 2006-08-15 20:12 ` Chong Yidong 0 siblings, 1 reply; 10+ messages in thread From: Richard Stallman @ 2006-08-15 12:41 UTC (permalink / raw) Cc: friedman, emacs-devel This is inconsistent with the old (built-in) behavior of sit-for, which did not activate idle timers while waiting either. Ok, I am convinced. Could you implement it? Meanwhile, before the call at line 1494 of keyboard.c, something ought to make Emacs idle. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: sit-for and idle timers 2006-08-15 12:41 ` Richard Stallman @ 2006-08-15 20:12 ` Chong Yidong 2006-08-16 19:27 ` Richard Stallman 0 siblings, 1 reply; 10+ messages in thread From: Chong Yidong @ 2006-08-15 20:12 UTC (permalink / raw) Cc: friedman, emacs-devel Richard Stallman <rms@gnu.org> writes: > This is inconsistent with the old (built-in) behavior of sit-for, > which did not activate idle timers while waiting either. > > Ok, I am convinced. Could you implement it? Done. I also updated the lispref manual to clarify this issue. > Meanwhile, before the call at line 1494 of keyboard.c, something ought > to make Emacs idle. You mean something like this? /* Bind inhibit-quit to t so that C-g gets read in rather than quitting back to the minibuffer. */ int count = SPECPDL_INDEX (); specbind (Qinhibit_quit, Qt); timer_start_idle (); sit_for (Vminibuffer_message_timeout, 0, 2); timer_stop_idle (); /* Clear the echo area. */ message2 (0, 0, 0); I don't really see why it's important (it's currently just a two second pause in which idle timers don't run), and I don't know the code well enough to know if it will break anything horribly. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: sit-for and idle timers 2006-08-15 20:12 ` Chong Yidong @ 2006-08-16 19:27 ` Richard Stallman 2006-08-16 19:38 ` Chong Yidong 0 siblings, 1 reply; 10+ messages in thread From: Richard Stallman @ 2006-08-16 19:27 UTC (permalink / raw) Cc: friedman, emacs-devel You mean something like this? /* Bind inhibit-quit to t so that C-g gets read in rather than quitting back to the minibuffer. */ int count = SPECPDL_INDEX (); specbind (Qinhibit_quit, Qt); timer_start_idle (); sit_for (Vminibuffer_message_timeout, 0, 2); timer_stop_idle (); Yes, except does it need timer_stop_idle? It is going to read input straightaway after. I don't really see why it's important (it's currently just a two second pause in which idle timers don't run), and I don't know the code well enough to know if it will break anything horribly. It isn't tremendously important, but it seems correct for Emacs' idleness to begin when the command finishes. If you see a risk, I don't insist on doing this now. How about if you install it in the unicode-2 branch? to be idle as soon as ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: sit-for and idle timers 2006-08-16 19:27 ` Richard Stallman @ 2006-08-16 19:38 ` Chong Yidong 0 siblings, 0 replies; 10+ messages in thread From: Chong Yidong @ 2006-08-16 19:38 UTC (permalink / raw) Cc: emacs-devel Richard Stallman <rms@gnu.org> writes: > If you see a risk, I don't insist on doing this now. > How about if you install it in the unicode-2 branch? OK. ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2006-08-16 19:38 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2006-08-11 19:48 sit-for and idle timers Noah Friedman 2006-08-11 21:00 ` Chong Yidong 2006-08-14 18:34 ` Chong Yidong 2006-08-14 19:20 ` Richard Stallman 2006-08-14 19:47 ` Chong Yidong 2006-08-14 20:05 ` Chong Yidong 2006-08-15 12:41 ` Richard Stallman 2006-08-15 20:12 ` Chong Yidong 2006-08-16 19:27 ` Richard Stallman 2006-08-16 19:38 ` Chong Yidong
Code repositories for project(s) associated with this external index https://git.savannah.gnu.org/cgit/emacs.git https://git.savannah.gnu.org/cgit/emacs/org-mode.git This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.