GNU bug report logs - #78773
[PATCH] Speedup url-retrieve-synchronously for low-latency connections

Previous Next

Full log

Message #74 received at 78773 <at> debbugs.gnu.org (full text, mbox):

From: Steven Allen <steven <at> stebalien.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 78773 <at> debbugs.gnu.org, rpluim <at> gmail.com, larsi <at> gnus.org
Subject: Re: bug#78773: [PATCH] Speedup url-retrieve-synchronously for
 low-latency connections
Date: Sat, 14 Jun 2025 09:15:00 -0700

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: Steven Allen <steven <at> stebalien.com>
>> Cc: rpluim <at> gmail.com, 78773 <at> debbugs.gnu.org, larsi <at> gnus.org
>> Date: Thu, 12 Jun 2025 13:54:01 -0700
>> 
>> Ok, I think I've figured it out. We loop twice and wait on a timeout
>> (READ_OUTPUT_DELAY_INCREMENT) the second loop because we're already done
>> reading.
>
> 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, but we
do know that there's no more available right now because we poll pselect
with a zero timeout when `wait' is `MINIMAL'. The following check
prevents us from exiting the loop (in this case) when there are file
descriptors with waiting data, regardless of what the timeout is:

  https://cgit.git.savannah.gnu.org/cgit/emacs.git/tree/src/process.c?h=bec823b107ef7d3b51b8e430ccab82c81bd63d24#n5810

There is still a design question of how this SHOULD behave (discussed
at the end).

>> The first time through, we hit the following, recording that we've
>> gotten some output:
>> 
>>   https://cgit.git.savannah.gnu.org/cgit/emacs.git/tree/src/process.c?h=81a3e4e51167be51c63eae682331210bc62f7280#n5974
>
> OK.
>
>> We hit the following, setting our timeout to READ_OUTPUT_DELAY_INCREMENT:
>> 
>>   https://cgit.git.savannah.gnu.org/cgit/emacs.git/tree/src/process.c?h=81a3e4e51167be51c63eae682331210bc62f7280#n5679
>
> This is to _shorten_ the wait timeout:
>
> 	  /* 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);
>
> So I don't quite see how this could be a problem here.  Am I missing
> something?

Nothing is wrong here, I'm explaining why we're adding exactly 10ms to
every call to `accept-process-output' with a nil PROCESS. The fact that
it only decreases the timeout also explains why your patch to reduce the
timeout in `url-retrieve-synchronously' makes this faster (as long as
the new timeout is less than 10ms).

>> Then we hit the select call and wait the full timeout (nothing to read)
>> and we finally exit here (because our timeout has passed):
>> 
>>   https://cgit.git.savannah.gnu.org/cgit/emacs.git/tree/src/process.c?h=81a3e4e51167be51c63eae682331210bc62f7280#n5832
>
> The "full timeout" being the value of READ_OUTPUT_DELAY_INCREMENT, is
> that right?  Then why is it not TRT, since it _shortens_ the wait?

Yes.

>
>> I think what we need to do is modify:
>> 
>>   https://cgit.git.savannah.gnu.org/cgit/emacs.git/tree/src/process.c?h=81a3e4e51167be51c63eae682331210bc62f7280#n5978
>> 
>> from
>> 
>>     		  if (wait_proc == XPROCESS (proc))
>> 		    wait = MINIMUM;
>> 
>> to
>> 
>>     		  if (!wait_proc || wait_proc == XPROCESS (proc))
>> 		    wait = MINIMUM;
>> 
>> So that we set the timeout to 0 here:
>> 
>>   https://cgit.git.savannah.gnu.org/cgit/emacs.git/tree/src/process.c?h=81a3e4e51167be51c63eae682331210bc62f7280#n5439
>> 
>> And exit early.
>
> Why?  That code was there for a reason.  What if we are waiting for
> other processes as well?
>
> What did I miss in the above description that explains why the current
> code is wrong?

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. If this is not the case, then:

1. We now understand why passing a nil process to
`accept-process-output' is slower than passing a specific process.

2. The fix to the original bug report is to apply the first patch I sent
to have `url-retrieve-synchronously' wait on a specific process.

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;

But I'm very much not sure about that.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.