diff mbox series

[bug#47288,v2] guix: http-client: Tweak http-multiple-get error handling.

Message ID 20210321005600.12551-1-mail@cbaines.net
State Accepted
Headers show
Series [bug#47288,v2] guix: http-client: Tweak http-multiple-get error handling. | expand

Checks

Context Check Description
cbaines/submitting builds success
cbaines/comparison success View comparision
cbaines/git branch success View Git branch
cbaines/applying patch success View Laminar job
cbaines/issue success View issue

Commit Message

Christopher Baines March 21, 2021, 12:56 a.m. UTC 
This isn't meant to change the way errors are handled, and arguably makes the
code harder to read, but it's a uninformed attempt to improve the
performance (following on from a performance regression in
205833b72c5517915a47a50dbe28e7024dc74e57).

I'm guessing something about Guile internals makes calling (loop ...) within
the catch bit less performant than avoiding this and calling (loop ...) after
the catch bit has finished. Since this happens lots, this seems to be
sufficient to make guix weather a lot slower than it was before.

Anecdotal testing of guix weather suggests this change might work.

* guix/http-client.scm (http-multiple-get): Tweak how the second catch
statement works.
---
 guix/http-client.scm | 77 +++++++++++++++++++++++++-------------------
 1 file changed, 43 insertions(+), 34 deletions(-)

Comments

Ludovic Courtès March 24, 2021, 2:55 p.m. UTC | #1 
As discussed at <https://issues.guix.gnu.org/47283>, I’m still unsure
these exceptions need to be caught within ‘http-multiple-get’ and at
each iteration.

Just minor comments:

Christopher Baines <mail@cbaines.net> skribis:

> +               (catch #t
> +                 (lambda ()
> +                   (let* ((resp   (read-response p))
> +                          (body   (response-body-port resp))
> +                          (result (proc head resp body result)))
> +                     ;; The server can choose to stop responding at any time,
> +                     ;; in which case we have to try again.  Check whether
> +                     ;; that is the case.  Note that even upon "Connection:
> +                     ;; close", we can read from BODY.
> +                     (match (assq 'connection (response-headers resp))
> +                       (('connection 'close)
> +                        (close-port p)
> +                        (list 'connect
> +                              #f
>                                (drop requests (+ 1 processed))
>                                result))
> -                   (apply throw key args))))))))))
> +                       (_
> +                        (list 'loop tail (+ 1 processed) result)))))
> +                 (lambda (key . args)
> +                   ;; If PORT was cached and the server closed the connection
> +                   ;; in the meantime, we get EPIPE.  In that case, open a
> +                   ;; fresh connection and retry.  We might also get
> +                   ;; 'bad-response or a similar exception from (web response)
> +                   ;; later on, once we've sent the request, or a
> +                   ;; ERROR/INVALID-SESSION from GnuTLS.
> +                   (if (or (and (eq? key 'system-error)
> +                                (= EPIPE (system-error-errno `(,key ,@args))))
> +                           (and (eq? key 'gnutls-error)
> +                                (eq? (first args) error/invalid-session))1
> +                                (memq key
> +                                      '(bad-response
> +                                        bad-header
> +                                        bad-header-component)))
> +                       (begin
> +                         (close-port p)
> +                         (list 'connect
> +                               #f
> +                               requests
> +                               result))
> +                       (apply throw key args))))
> +             (('connect . args)
> +              (apply connect args))
> +             (('loop . args)
> +              (apply loop args)))))))))

This is not new to this patch, but I think the whole exception handling
bit should be factorized and written in such a way that
‘http-multiple-get’ still first on a horizontal screen (even though mine
is actually vertical :-)).  Otherwise one might easily overlook the core
logic of the function.

Ludo’.
Ludovic Courtès March 24, 2021, 2:55 p.m. UTC | #2 
As discussed at <https://issues.guix.gnu.org/47283>, I’m still unsure
these exceptions need to be caught within ‘http-multiple-get’ and at
each iteration.

Just minor comments:

Christopher Baines <mail@cbaines.net> skribis:

> +               (catch #t
> +                 (lambda ()
> +                   (let* ((resp   (read-response p))
> +                          (body   (response-body-port resp))
> +                          (result (proc head resp body result)))
> +                     ;; The server can choose to stop responding at any time,
> +                     ;; in which case we have to try again.  Check whether
> +                     ;; that is the case.  Note that even upon "Connection:
> +                     ;; close", we can read from BODY.
> +                     (match (assq 'connection (response-headers resp))
> +                       (('connection 'close)
> +                        (close-port p)
> +                        (list 'connect
> +                              #f
>                                (drop requests (+ 1 processed))
>                                result))
> -                   (apply throw key args))))))))))
> +                       (_
> +                        (list 'loop tail (+ 1 processed) result)))))
> +                 (lambda (key . args)
> +                   ;; If PORT was cached and the server closed the connection
> +                   ;; in the meantime, we get EPIPE.  In that case, open a
> +                   ;; fresh connection and retry.  We might also get
> +                   ;; 'bad-response or a similar exception from (web response)
> +                   ;; later on, once we've sent the request, or a
> +                   ;; ERROR/INVALID-SESSION from GnuTLS.
> +                   (if (or (and (eq? key 'system-error)
> +                                (= EPIPE (system-error-errno `(,key ,@args))))
> +                           (and (eq? key 'gnutls-error)
> +                                (eq? (first args) error/invalid-session))1
> +                                (memq key
> +                                      '(bad-response
> +                                        bad-header
> +                                        bad-header-component)))
> +                       (begin
> +                         (close-port p)
> +                         (list 'connect
> +                               #f
> +                               requests
> +                               result))
> +                       (apply throw key args))))
> +             (('connect . args)
> +              (apply connect args))
> +             (('loop . args)
> +              (apply loop args)))))))))

This is not new to this patch, but I think the whole exception handling
bit should be factorized and written in such a way that
‘http-multiple-get’ still fits on a horizontal screen (even though mine
is actually vertical :-)).  Otherwise one might easily overlook the core
logic of the function.

Ludo’.
Christopher Baines March 25, 2021, 11:09 a.m. UTC | #3 
Ludovic Courtès <ludo@gnu.org> writes:

> As discussed at <https://issues.guix.gnu.org/47283>, I’m still unsure
> these exceptions need to be caught within ‘http-multiple-get’ and at
> each iteration.
>
> Just minor comments:
>
> Christopher Baines <mail@cbaines.net> skribis:
>
>> +               (catch #t
>> +                 (lambda ()
>> +                   (let* ((resp   (read-response p))
>> +                          (body   (response-body-port resp))
>> +                          (result (proc head resp body result)))
>> +                     ;; The server can choose to stop responding at any time,
>> +                     ;; in which case we have to try again.  Check whether
>> +                     ;; that is the case.  Note that even upon "Connection:
>> +                     ;; close", we can read from BODY.
>> +                     (match (assq 'connection (response-headers resp))
>> +                       (('connection 'close)
>> +                        (close-port p)
>> +                        (list 'connect
>> +                              #f
>>                                (drop requests (+ 1 processed))
>>                                result))
>> -                   (apply throw key args))))))))))
>> +                       (_
>> +                        (list 'loop tail (+ 1 processed) result)))))
>> +                 (lambda (key . args)
>> +                   ;; If PORT was cached and the server closed the connection
>> +                   ;; in the meantime, we get EPIPE.  In that case, open a
>> +                   ;; fresh connection and retry.  We might also get
>> +                   ;; 'bad-response or a similar exception from (web response)
>> +                   ;; later on, once we've sent the request, or a
>> +                   ;; ERROR/INVALID-SESSION from GnuTLS.
>> +                   (if (or (and (eq? key 'system-error)
>> +                                (= EPIPE (system-error-errno `(,key ,@args))))
>> +                           (and (eq? key 'gnutls-error)
>> +                                (eq? (first args) error/invalid-session))1
>> +                                (memq key
>> +                                      '(bad-response
>> +                                        bad-header
>> +                                        bad-header-component)))
>> +                       (begin
>> +                         (close-port p)
>> +                         (list 'connect
>> +                               #f
>> +                               requests
>> +                               result))
>> +                       (apply throw key args))))
>> +             (('connect . args)
>> +              (apply connect args))
>> +             (('loop . args)
>> +              (apply loop args)))))))))
>
> This is not new to this patch, but I think the whole exception handling
> bit should be factorized and written in such a way that
> ‘http-multiple-get’ still first on a horizontal screen (even though mine
> is actually vertical :-)).  Otherwise one might easily overlook the core
> logic of the function.

I've sent a v3 now, which makes a few changes to the original patch, and
includes a second patch for a potential refactoring.

I tested three variants for performance, http-multiple-get with no error
handling, the first v3 patch and the first and second v3 patches, and at
least with the test I'm using, the performance seems similar. Assuming
the performance is lower with error handling, the impact seems to be
within the margin of error, at least for test I was using.
(use-modules (ice-9 binary-ports)
             (srfi srfi-19)
             (web uri)
             (web request)
             (web response)
             (guix http-client))

(define (call-with-time-logging requests thunk)
  (let ((start   (current-time time-utc)))
    (call-with-values
        thunk
      (lambda vals
        (let* ((end     (current-time time-utc))
               (elapsed (time-difference end start)))
          (display
           (format #f
                   "~f seconds (~f microseconds per request)~%"
                   (+ (time-second elapsed)
                      (/ (time-nanosecond elapsed) 1e9))
                   (* (/ (+ (time-second elapsed)
                            (/ (time-nanosecond elapsed) 1e9))
                         requests)
                      1e6)))
          (apply values vals))))))

(define requests
  (map (lambda _
         (build-request (string->uri "http://localhost/does-not-exist")
                        #:method 'GET))
       (iota 200000)))

(call-with-time-logging
 (length requests)
 (lambda ()
   (http-multiple-get (string->uri "http://localhost/")
                      (lambda (request response port result)
                        (get-bytevector-n
                         port
                         (response-content-length response))
                        '())
                      '()
                      requests)))
diff mbox series

Patch

diff --git a/guix/http-client.scm b/guix/http-client.scm
index 4b4c14ed0b..a9609445c8 100644
--- a/guix/http-client.scm
+++ b/guix/http-client.scm
@@ -219,42 +219,51 @@  returning."
              (remainder
               (connect p remainder result))))
           ((head tail ...)
-           (catch #t
-             (lambda ()
-               (let* ((resp   (read-response p))
-                      (body   (response-body-port resp))
-                      (result (proc head resp body result)))
-                 ;; The server can choose to stop responding at any time,
-                 ;; in which case we have to try again.  Check whether
-                 ;; that is the case.  Note that even upon "Connection:
-                 ;; close", we can read from BODY.
-                 (match (assq 'connection (response-headers resp))
-                   (('connection 'close)
-                    (close-port p)
-                    (connect #f                       ;try again
-                             (drop requests (+ 1 processed))
-                             result))
-                   (_
-                    (loop tail (+ 1 processed) result))))) ;keep going
-             (lambda (key . args)
-               ;; If PORT was cached and the server closed the connection
-               ;; in the meantime, we get EPIPE.  In that case, open a
-               ;; fresh connection and retry.  We might also get
-               ;; 'bad-response or a similar exception from (web response)
-               ;; later on, once we've sent the request, or a
-               ;; ERROR/INVALID-SESSION from GnuTLS.
-               (if (or (and (eq? key 'system-error)
-                            (= EPIPE (system-error-errno `(,key ,@args))))
-                       (and (eq? key 'gnutls-error)
-                            (eq? (first args) error/invalid-session))
-                       (memq key
-                             '(bad-response bad-header bad-header-component)))
-                   (begin
-                     (close-port p)
-                     (connect #f      ; try again
+           (match
+               (catch #t
+                 (lambda ()
+                   (let* ((resp   (read-response p))
+                          (body   (response-body-port resp))
+                          (result (proc head resp body result)))
+                     ;; The server can choose to stop responding at any time,
+                     ;; in which case we have to try again.  Check whether
+                     ;; that is the case.  Note that even upon "Connection:
+                     ;; close", we can read from BODY.
+                     (match (assq 'connection (response-headers resp))
+                       (('connection 'close)
+                        (close-port p)
+                        (list 'connect
+                              #f
                               (drop requests (+ 1 processed))
                               result))
-                   (apply throw key args))))))))))
+                       (_
+                        (list 'loop tail (+ 1 processed) result)))))
+                 (lambda (key . args)
+                   ;; If PORT was cached and the server closed the connection
+                   ;; in the meantime, we get EPIPE.  In that case, open a
+                   ;; fresh connection and retry.  We might also get
+                   ;; 'bad-response or a similar exception from (web response)
+                   ;; later on, once we've sent the request, or a
+                   ;; ERROR/INVALID-SESSION from GnuTLS.
+                   (if (or (and (eq? key 'system-error)
+                                (= EPIPE (system-error-errno `(,key ,@args))))
+                           (and (eq? key 'gnutls-error)
+                                (eq? (first args) error/invalid-session))1
+                                (memq key
+                                      '(bad-response
+                                        bad-header
+                                        bad-header-component)))
+                       (begin
+                         (close-port p)
+                         (list 'connect
+                               #f
+                               requests
+                               result))
+                       (apply throw key args))))
+             (('connect . args)
+              (apply connect args))
+             (('loop . args)
+              (apply loop args)))))))))
 
 
 ;;;