Re: Curl upgrade from 7.65.1 to 7.70.0 causing performance degradation

Hi Daniel,

On Wed, Sep 16, 2020 at 5:16 PM Kunal Chandarana <chandarana.kunal_at_gmail.com>
wrote:

>
>> The need for a "pause buffer" is unfortunate to begin with. There are
>> however
>> times (when using HTTP/2 primarily) when curl can't stop the transfer
>> immediately and thus it needs somewhere to store the incoming data that
>> arrives after the user told libcurl to pause.
>>
>
> Thanks for explaining it, I was curious about the need to use a pause
> buffer.
>
>>
>> The size of that dynamic buffer then simply depends on how much data that
>> arrives while the connection is paused. To me, it seems unlikely that the
>> commit mentioned above is able to add to that data amount. In fact, it
>> should
>> help drain the existing buffer bettere, since that change is in the
>> *unpause*
>> logic.
>>
>
> That makes sense. However let me briefly explain how we are using
> unpause/pause (maybe usage is not as anticipated). We use
> curl_multi_perform in our implementation.
> 1) We have a callback for CURLOPT_WRITEFUNCTION that injects 16KB (or
> less) of data received from the library into a queue (as one item).
> 2) If the queue size exceeds 20 after pushing data (that is 20 items), we
> *pause* the connection.
> 3) Another thread is reading from the queue and each dequeue call results
> in *unpause* the connection
> 4) After unpausing connection, write function callback again starts
> receiving data. If the queue reaches it's capacity, we *pause* the
> connection again.
> 5) Reading from the queue results in *unpausing* connection.
> This is how pausing and unpausing continues until the response is
> processed. In your opinion, do you see the usage flawed?
>
> The commit I mentioned earlier sets CURL_CSELECT_IN in case an unpause
> call occurs. It opens up read-write flow here
> <https://github.com/curl/curl/search?q=select_res+%26+CURL_CSELECT_IN&unscoped_q=select_res+%26+CURL_CSELECT_IN> and
> the buffer
> <https://github.com/curl/curl/search?q=%26s-%3Etempwrite%5Bi%5D.b%2C+DYN_PAUSE_BUFFER&unscoped_q=%26s-%3Etempwrite%5Bi%5D.b%2C+DYN_PAUSE_BUFFER>
> continues to grow. Please feel free to correct me if my understanding is
> incorrect.
> Your earlier recommendation was bisecting for pinpointing the commit that
> is causing this issue. I revert the commit locally and performance
> degradation is eliminated. What further investigation from my side would
> help here? Do let me know and I would be happy to make progress in that
> direction.
>

I am still blocked on the performance degradation issue so I further looked
at it. In case of a connection unpause, the library flushes out buffer data
via write callback
<https://github.com/curl/curl/search?q=result+%3D+Curl_client_write%28conn%2C+writebuf%5Bi%5D.type%2C>.
But the callback may return magic code
<https://curl.haxx.se/libcurl/c/CURLOPT_WRITEFUNCTION.html> for pausing the
connection if it is not ready to receive the entire data in the buffer and
in that case the buffer may still have some data. Shouldn't the reading
from the socket be forced only if the buffer is empty? I made the following
change (screenshot given below) in the 7.70.0 version of the code and it
eliminates the performance degradation issue. What do you think about this
code change?

[image: image.png]

Thanks,
Kunal

-------------------------------------------------------------------
Unsubscribe: https://cool.haxx.se/list/listinfo/curl-library
Etiquette: https://curl.haxx.se/mail/etiquette.html