Subject: Re: bug#78773: [PATCH] Speedup url-retrieve-synchronously for low-latency connections

From: Steven Allen <[email-masked]>
Date: Sat, 14 Jun 2025 13:12:00 -0700

Ian (CCed): I've run into an issue where `accept-process-output' is
always waiting an extra 10ms (READ_OUTPUT_DELAY_INCREMENT) after
receiving output as long as the PROCESS passed to it is nil. This
appears to be related to the change you made in bug#20978 and I'm
wondering if you can shed more light on when `accept-process-output'
should wait for more input.

Eli Zaretskii <[email-masked]> writes:

>> From: Steven Allen <[email-masked]>
>> Cc: [email-masked], [email-masked], [email-masked]
>> Date: Sat, 14 Jun 2025 09:15:00 -0700
>>
>> Eli Zaretskii <[email-masked]> writes:
>>
>> > We wait the second time because there could be more input, so I don't
>> > see how this could be wrong in general.  I also don't quite see the
>> > "gotcha!" in your analysis below:
>>
>> There is no available input. There might be more input in 10ms
>
> That's what I meant by "there could be more input".

I'd rather make use of what we know: we don't know if we'll get more
data in the near future but we *do* know that we've just received some
data that the caller (of `accept-process-output`) may want to handle. So
we might as well return to the caller and give them a chance to do
something about it.

The exception to this rule is adaptive read buffering where explicitly
_don't_ want to yield too little data at a time if we expect we'll
receive more in the near future. I agree we should wait a bit longer in
that case (the attached patch should handle that).

>> My understanding is that `accept-process-output' is supposed to return
>> immediately when output has been read BOTH when PROCESS is nil and when
>> it is non-nil.
>
> That's true, but wait_reading_process_output is called not just by
> accept-process-output, it is called by several other callers, and in
> that case it is not necessarily true that we need to exit immediately
> upon the first available channel.  Keep in mind that reading from a
> subprocess also invokes a filter function, if there is one, so it
> might make sense in some cases to read from more than one source,
> perhaps even from all of the ones that have output ready to be read.
>
>> Looking at the history, it looks like the code in question was changed
>> in bug#20978. However, from what I can tell, we should only apply
>> adaptive read buffering when `process-adaptive-read-buffering' is
>> non-nil. I think we may need to change the check in question to:
>>
>> 		  if (!process_output_skip && (!wait_proc || wait_proc == XPROCESS (proc)))
>> 		    wait = MINIMUM;
>
> That could indeed fly, but process-adaptive-read-buffering is non-nil
> by default.  Does url set it to nil?

It is nil by default as of bug#75574 (8a669b6be523e043423b81571a8c94cb49ccc8e5).

> In any case, I think the addition of process-adaptive-read-buffering
> test should only affect the case when wait_proc is zero.

Looking at this more, I'm becoming more and more convinced that this was
a bug inadvertently introduced by
12a2691fb254db20607b2067a12b795a6d7c6649. That patch was trying to make
adaptive read buffering work again but it went too far and applied the
delay everywhere. I've attached what I think is the correct patch, but
I've also CCed Ian (author of #20978 and the associated patch) in hopes
of getting a bit more context here.

>From 8ab170b8ad4718e6d5d12bbc85e9bcda02fffc0d Mon Sep 17 00:00:00 2001
From: Steven Allen <[email-masked]>
Date: Sat, 14 Jun 2025 10:05:05 -0700
Subject: [PATCH] Exit accept-process-output early when we get output from any
 process

Previously, the caller requested to wait on output from any process (not
a specific process), we'd always end up waiting
10ms (READ_OUTPUT_DELAY_INCREMENT) one final time before returning, even
when adaptive read buffering was disabled.

* src/process.c (wait_reading_process_output): skip adaptive read
buffering delays when we're (a) not reading from any specific process
and (b) adaptive read buffering is disabled. (Bug#78773)
---
 src/process.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/src/process.c b/src/process.c
index e61ec425f7e..81a4aa32dcf 100644
--- a/src/process.c
+++ b/src/process.c
@@ -5671,13 +5671,12 @@ wait_reading_process_output (intmax_t time_limit, int nsecs, int read_kbd,
 	      process_output_skip = 0;
 	    }

-	  /* If we've got some output and haven't limited our timeout
-	     with adaptive read buffering, limit it. */
-	  if (got_some_output > 0 && !process_skipped
-	      && (timeout.tv_sec
-		  || timeout.tv_nsec > READ_OUTPUT_DELAY_INCREMENT))
-	    timeout = make_timespec (0, READ_OUTPUT_DELAY_INCREMENT);
-
+	  /* If we've got some output and aren't waiting on a process
+	     via adaptive read buffering, switch our waiting mode to
+	     MINIMUM vacuum up any data remaining to be read then
+             exit. */
+	  if (got_some_output > 0 && !process_skipped)
+	    wait = MINIMUM;

 	  if (NILP (wait_for_cell) && just_wait_proc >= 0
 	      && timespec_valid_p (timer_delay)
@@ -5974,9 +5973,6 @@ wait_reading_process_output (intmax_t time_limit, int nsecs, int read_kbd,
 		got_some_output = nread;
 	      if (nread > 0)
 		{
-		  /* Vacuum up any leftovers without waiting.  */
-		  if (wait_proc == XPROCESS (proc))
-		    wait = MINIMUM;
 		  /* Since read_process_output can run a filter,
 		     which can call accept-process-output,
 		     don't try to read from any other processes
-- 
2.49.0