[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Emacs-diffs] master 1392ec7 2/3: A quicker check for quit
From: |
Eli Zaretskii |
Subject: |
Re: [Emacs-diffs] master 1392ec7 2/3: A quicker check for quit |
Date: |
Tue, 31 Jan 2017 17:48:45 +0200 |
> Cc: address@hidden, address@hidden
> From: Paul Eggert <address@hidden>
> Date: Mon, 30 Jan 2017 13:52:46 -0800
>
> I don't understand why we need both this and
> emacs_read, which cannot be interrupted. Why not have just emacs_read
> which can be interrupted, and use that all over?
>
> For example, filelock.c's read_lock_data opens a file, uses emacs_read to
> read it, and then closes the file. If read_lock_data used emacs_read_quit it
> might process a quit, which would skip the close and leak a file descriptor.
>
> The read_lock_data issue could be fixed by having it call
> record_unwind_protect_int (close_file_unwind, fd) before calling emacs_read.
> Possibly all these dicey uses of emacs_read could be fixed in a similar way.
> However, that would be a bigger and more-intrusive change, and in the
> read_lock_data case it arguably would be overkill and I wanted to keep the
> patch smaller. I used emacs_read_quit only in places that I verified were
> safe, and stuck with emacs_read when I wasn't sure, or where more-intrusive
> changes would be needed.
I indeed think that we should make emacs_read support quitting, and
add unwind_protect calls where we currently don't. This should be
safer in the long run, and also simpler. As for overhead, operations
like locking a file should indeed normally be very fast, but could
take perceptible time in some exceptional conditions, like networked
volumes or high I/O load, in which case users may wish to interrupt
that.
But yes, this could be done as a separate changeset.
> @@ -1252,6 +1256,7 @@ search_buffer (Lisp_Object string, ptrdiff_t
> pos,
> ptrdiff_t pos_byte,
> return (n);
> }
> n++;
> + maybe_quit ();
> }
> while (n > 0)
> {
> regex.c calls maybe_quit internally, so why do we need this additional
> call?
>
> The regex code does not always call maybe_quit. For example, without this
> additional call, (re-search-forward "[[:alpha:]]" nil nil
> most-positive-fixnum) would loop indefinitely in a buffer containing only
> alphabetic characters on a 64-bit platform.
Then maybe we should add maybe_quit calls in regex.c instead?
Currently, immediate_quit is non-zero all the time re_search_2 runs,
so on a TTY a C-g will can stop regex.c in its tracks anywhere. I
thought we wanted to make GUI sessions as responsive as TTY sessions.
> @@ -724,6 +725,8 @@ back_comment (ptrdiff_t from, ptrdiff_t from_byte,
> ptrdiff_t stop,
> that determines quote parity to the comment-end. */
> while (from != stop)
> {
> + incr_rarely_quit (&quit_count);
> +
>
> Is it safe to quit from back_comment? It manipulates a global
> variable gl_state, and I don't see unwind-protect calls anywhere in
> sight.
>
> It should be OK. The current master sets immediate_quit=true in
> back_comment's callers (both scan_lists and Fforward_comment), so current
> master already lets back_comment quit.
Yes, but that is why we have gl_state-related dance in
handle_interrupt, and your changes delete that part.
> If Emacs quits in back_comment, it should longjmp to code that reinitializes
> gl_state before using it.
But unwinding the Lisp stack could run some Lisp that uses syntax.c
functions, before we longjmp, right?
> This also applies to the other places you mentioned. The idea is to insert
> maybe_quit calls in code that was already subject to immediate_quit=true in
> the current master, so it should be safe to quit.
That assumes all the immediate_quit=true settings were safe.
Previously, they were only in effect on TTY frames, whereas now the
maybe_quit calls will be in effect everywhere, so their exposure to
various use cases will be much wider. That's why I think it's prudent
to take a good look at these places while we make these changes.
But I don't feel I know enough about this aspect of syntax.c. Stefan,
can you comment on this, please?
> @@ -10445,30 +10433,12 @@ handle_interrupt (bool in_signal_handler)
> }
> else
> {
> - /* If executing a function that wants to be interrupted out of
> - and the user has not deferred quitting by binding `inhibit-quit'
> - then quit right away. */
> - if (immediate_quit && NILP (Vinhibit_quit))
> - {
> - struct gl_state_s saved;
> -
> - immediate_quit = false;
> - pthread_sigmask (SIG_SETMASK, &empty_mask, 0);
> - saved = gl_state;
> - quit ();
> - gl_state = saved;
> - }
> - else
> - { /* Else request quit when it's safe. */
> - int count = NILP (Vquit_flag) ? 1 : force_quit_count + 1;
> - force_quit_count = count;
> - if (count == 3)
> - {
> - immediate_quit = true;
> - Vinhibit_quit = Qnil;
> - }
> - Vquit_flag = Qt;
> - }
> + /* Request quit when it's safe. */
> + int count = NILP (Vquit_flag) ? 1 : force_quit_count + 1;
> + force_quit_count = count;
> + if (count == 3)
> + Vinhibit_quit = Qnil;
> + Vquit_flag = Qt;
> }
>
> This loses the feature whereby C-g on a TTY would quit much faster.
> Why is this a good idea?
>
> Speed is not a problem, as C-g (with the proposed changes) should quit just
> as fast on a TTY as it already does under X, and it's been working that way
> under X for some time.
No, it will be slower. A signal handler will always react faster than
any solution based on polling. A signal handler is also capable of
interrupting calls to standard C library functions.
It is true that we already have this issue on GUI frames, but I still
feel uneasy about losing this feature. TTY frames are still quite
popular, even today, in particular for remote sessions.
What do others think about this?
> And if it is a good idea, why do we still
> generate SIGINT on C-g (and force GDB to handle SIGINT specially to
> support that)?
>
> Inertia, I think. Having C-g generate SIGINT made sense when we had
> immediate_quit. I expect that it is a useless appendage now, and that in a
> later patch we can change Emacs so that C-g no longer generates SIGINT but is
> instead processed like any other input character.
No, I don't think we can remove the SIGINT generation: if we do, there
will be nothing to set the quit-flag on TTY frames. Also, the
"emergency exit" feature is also based on SIGINT.
The new patches still include both rarely_quit and incr_rarely_quit
(in the second patchset), which I thought you decided to remove. Did
you send the correct patches?
Thanks.
- Re: [Emacs-diffs] master 1392ec7 2/3: A quicker check for quit, Stefan Monnier, 2017/01/26
- Re: [Emacs-diffs] master 1392ec7 2/3: A quicker check for quit, Paul Eggert, 2017/01/26
- Re: [Emacs-diffs] master 1392ec7 2/3: A quicker check for quit, Eli Zaretskii, 2017/01/26
- Re: [Emacs-diffs] master 1392ec7 2/3: A quicker check for quit, Eli Zaretskii, 2017/01/29
- Re: [Emacs-diffs] master 1392ec7 2/3: A quicker check for quit, Paul Eggert, 2017/01/29
- Re: [Emacs-diffs] master 1392ec7 2/3: A quicker check for quit, Eli Zaretskii, 2017/01/30
- Re: [Emacs-diffs] master 1392ec7 2/3: A quicker check for quit, Paul Eggert, 2017/01/30
- Re: [Emacs-diffs] master 1392ec7 2/3: A quicker check for quit,
Eli Zaretskii <=
- Re: [Emacs-diffs] master 1392ec7 2/3: A quicker check for quit, Stefan Monnier, 2017/01/31
- Re: [Emacs-diffs] master 1392ec7 2/3: A quicker check for quit, Paul Eggert, 2017/01/31