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

Previous Next

Full log

View this message in rfc822 format

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, ian <at> iankelling.org
Subject: bug#78773: [PATCH] Speedup url-retrieve-synchronously for low-latency connections
Date: Sun, 15 Jun 2025 09:55:52 -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, Ian Kelling
>>  <ian <at> iankelling.org>
>> 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.
>
> Yes, Ian's input will be appreciated, especially if he can explain the
> rationale for this part of the patch in enough detail for us to
> understand what parts of it are essential and which aren't.
>
>> 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: Sat, 14 Jun 2025 09:15:00 -0700
>> >>
>> >> Eli Zaretskii <eliz <at> gnu.org> 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.
>
> This would be wrong.  In most cases, especially those where
> performance matters, sub-process output arrives in many chunks, and
> there almost always is another chunk coming very soon.  That's how I
> understand the rationale for READ_OUTPUT_DELAY_INCREMENT stuff and its
> use.

Hence my second paragraph:

>> 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).

When adaptive read buffering is enabled, if we read less then 256 bytes
from one or more processes, we wait a bit for more output to arrive from
these processes (the process_output_skip flag is set to 1 when the
adaptive read buffering algorithm kicks in):

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

However, the current code unconditionally waits 10ms when we're not
applying the adaptive read algorithm:

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

>> > 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).
>
> So the problem happens only with the master branch, and doesn't exist
> in Emacs 30 and older?

The problem being discussed here happens in the latest release as well.
I'm simply pointing out that your assertion that
process-adaptive-read-buffering is non-nil by default is incorrect on
the latest master so we're all on the same page.

Additionally, I should note that adaptive read buffering only applies to
system processes and pipes, not network connections (it's not enabled in
`make-network-process') so it's not relevant to
`url-retrieve-synchronously'. I only bring it up is that I agree that
it's important to preserve its behavior in
`wait_reading_process_output'.

>> 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.
>
> This cannot be TRT, as you have already discovered.
>
> Can we please agree that as long as we think the problem happens due
> to some specific feature of the code, such as
> process-adaptive-read-buffering, the fix should be limited _only_ to
> that feature, and should not affect any other uses?  Because that's
> the only way to make changes in this code without risking regressions.

I don't think the problem happens because of
`process-adaptive-read-buffering'. I think the problem happens because,
in an attempt to make adaptive read buffering work again, an
unconditional 10ms delay was added to `accept-process-output'.

(see my first response at the top of this message)

Previous Next

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