bug#47283: Performance regression in narinfo fetching

[Ludovic Courtès]

Hi Chris,

Christopher Baines <mail@cbaines.net> skribis:

> Ludovic Courtès <ludo@gnu.org> writes:

[...]

>> Earlier, commit be5a75ebb5988b87b2392e2113f6590f353dd6cd (“substitute:
>> Reuse connections for '--query'.”) did not add such a ‘catch’ block in
>> ‘http-multiple-get’.  Instead, it wrapped its call in ‘do-fetch’ in
>> ‘fetch-narinfos’:
>>
>>    (define (do-fetch uri)
>>      (case (and=> uri uri-scheme)
>>        ((http https)
>> -       (let ((requests (map (cut narinfo-request url <>) paths)))
>> -         (match (open-connection-for-uri/maybe uri)
>> -           (#f
>> -            '())
>> -           (port
>> -            (update-progress!)
>>         ;; Note: Do not check HTTPS server certificates to avoid depending
>>         ;; on the X.509 PKI.  We can do it because we authenticate
>>         ;; narinfos, which provides a much stronger guarantee.
>> -            (let ((result (http-multiple-get uri
>> +       (let* ((requests (map (cut narinfo-request url <>) paths))
>> +              (result   (call-with-cached-connection uri
>> +                          (lambda (port)
>> +                            (if port
>> +                                (begin
>> +                                  (update-progress!)
>> +                                  (http-multiple-get uri
>>                                                       
>> handle-narinfo-response '()
>>                                                       requests
>> +                                                     #:open-connection
>> +                                                     
>> open-connection-for-uri/cached
>>                                                       #:verify-certificate? 
>> #f
>> -                                             #:port port)))
>>
>> This bit is still there in current ‘master’, so I think it’s not
>> necessary to catch these exceptions in ‘http-multiple-get’ itself, and I
>> would just remove the ‘catch’ wrap altogether.
>>
>> WDYT?
>
> I'm not sure what you're referring to as still being there on the master
> branch?

On ‘master’, ‘do-fetch’ has its ‘http-multiple-get’ call wrapped in
‘call-with-connection-error-handling’, which is equivalent to the change
made in be5a75ebb5988b87b2392e2113f6590f353dd6cd and shown above.

> Looking at the changes to this particular code path resulting from the
> changes I've made recently, starting at lookup-narinfos, before:
>
>  - lookup-narinfos calls fetch-narinfos, which calls do-fetch
>
>  - call-with-cached-connection is used, which catches a number of
>    exceptions relating to requests, and will retry PROC once upon a
>    matching exception
>
>  - open-connection-for-uri/maybe is also used, which is like
>    open-connection-for-uri/cached, except it includes error handling for
>    establishing connections to substitute servers
>
>  - http-multiple-get doesn't include error handling
>
> After:
>
>  - lookup-narinfos calls fetch-narinfos, which calls do-fetch
>
>  - call-with-connection-error-handling is used, which performs the same
>    role as the error handling previously within
>    open-connection-for-uri/maybe, catching exceptions relating to
>    establishing connections to substitute servers
>
>  - http-multiple-get now includes error handling similar to what was
>    previously done by call-with-cached-connection, although it's more
>    complicated since it's done with knowledge of what http-multiple-get
>    is doing
>
> I think that the error handling now in http-multiple-get isn't covered
> elsewhere. Moving this error handling back in to fetch-narinfos is
> possible, but then we'd be back to handling connection caching in that
> code, and avoiding that led to this refactoring in the first place.

The ‘http-multiple-get’ call in ‘fetch-narinfos’ is already wrapped in
‘call-with-connection-error-handling’, so it seems we’re good?

> Also, apart from the implementation problems, I do think that the error
> handling here is better than before. Previously, if you called
> lookup-narinfos, and a connection problem occurred, processing all the
> requests would start from scratch (as call-with-cached-connection calls
> PROC a second time), if a second connection error was to happen, well,
> call-with-cached-connection only handles one error, so that won't be
> caught.

Hmm true.  However, can that second exception happen?  Normally, if we
get a first exception, we open a new connection and that one should not
get another exception, unless something is wrong—in which case it’s
probably best to report it than to endlessly retry.

WDYT?

Even in tail call position, ‘catch’ calls introduce allocations and
extra work, so if we can avoid using one ‘catch’ per iteration, that’s
better.

Thank you,
Ludo’.