From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mp1 ([2001:41d0:8:6d80::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by ms11 with LMTPS id CWNgA3FUWmDmVgAA0tVLHw (envelope-from ) for ; Tue, 23 Mar 2021 20:49:53 +0000 Received: from aspmx1.migadu.com ([2001:41d0:8:6d80::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by mp1 with LMTPS id ACMqOnBUWmDYXwAAbx9fmQ (envelope-from ) for ; Tue, 23 Mar 2021 20:49:52 +0000 Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by aspmx1.migadu.com (Postfix) with ESMTPS id 3A28C1BB36 for ; Tue, 23 Mar 2021 21:49:52 +0100 (CET) Received: from localhost ([::1]:52420 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1lOnyI-0004AO-6s for larch@yhetil.org; Tue, 23 Mar 2021 16:49:50 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:57658) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1lOnwc-0002M2-H2 for bug-guix@gnu.org; Tue, 23 Mar 2021 16:48:07 -0400 Received: from debbugs.gnu.org ([209.51.188.43]:49880) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1lOnwY-0005df-H0 for bug-guix@gnu.org; Tue, 23 Mar 2021 16:48:06 -0400 Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1lOnwY-0001FZ-Ff for bug-guix@gnu.org; Tue, 23 Mar 2021 16:48:02 -0400 X-Loop: help-debbugs@gnu.org Subject: bug#47283: Performance regression in narinfo fetching Resent-From: Christopher Baines Original-Sender: "Debbugs-submit" Resent-CC: bug-guix@gnu.org Resent-Date: Tue, 23 Mar 2021 20:48:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 47283 X-GNU-PR-Package: guix X-GNU-PR-Keywords: To: Ludovic =?UTF-8?Q?Court=C3=A8s?= Received: via spool by 47283-submit@debbugs.gnu.org id=B47283.16165324384749 (code B ref 47283); Tue, 23 Mar 2021 20:48:02 +0000 Received: (at 47283) by debbugs.gnu.org; 23 Mar 2021 20:47:18 +0000 Received: from localhost ([127.0.0.1]:33193 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1lOnvp-0001EW-Sf for submit@debbugs.gnu.org; Tue, 23 Mar 2021 16:47:18 -0400 Received: from mira.cbaines.net ([212.71.252.8]:57424) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1lOnvo-0001EP-Ij for 47283@debbugs.gnu.org; Tue, 23 Mar 2021 16:47:17 -0400 Received: from localhost (unknown [IPv6:2a02:8010:68c1:0:8ac0:b4c7:f5c8:7caa]) by mira.cbaines.net (Postfix) with ESMTPSA id 6382627BC59; Tue, 23 Mar 2021 20:47:15 +0000 (GMT) Received: from capella (localhost [127.0.0.1]) by localhost (OpenSMTPD) with ESMTP id e8a34252; Tue, 23 Mar 2021 20:47:15 +0000 (UTC) References: <87ft0p67z4.fsf@inria.fr> <874kh5d0rg.fsf@cbaines.net> <87czvs19tp.fsf@gnu.org> User-agent: mu4e 1.4.15; emacs 27.1 From: Christopher Baines In-reply-to: <87czvs19tp.fsf@gnu.org> Date: Tue, 23 Mar 2021 20:47:12 +0000 Message-ID: <87k0pxbnsf.fsf@cbaines.net> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha512; protocol="application/pgp-signature" X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: bug-guix@gnu.org List-Id: Bug reports for GNU Guix List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: 47283@debbugs.gnu.org Errors-To: bug-guix-bounces+larch=yhetil.org@gnu.org Sender: "bug-Guix" X-Migadu-Flow: FLOW_IN ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=yhetil.org; s=key1; t=1616532592; h=from:from:sender:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:resent-cc:resent-from:resent-sender: resent-message-id:in-reply-to:in-reply-to:references:references: list-id:list-help:list-unsubscribe:list-subscribe:list-post; bh=FY1fd1g1xs8XVdqSyowHOJcpvrJU4bIM7kaLNK4QzzI=; b=INpdt4cmkw4OeVEgiA3PgsYr3J1ClNWXHSIs/2pCTz33zAIhb2KyR0l1V1W7jPZT1aPSsT VB2cP3XQK2PNCK2/e2H2gVHrtnv/U1MrLlh1MTnzWKYjE4Jpp6GRXduWFjUwlZyD4x1JpP u4eJyX3DOrZNeF1xOwYpIeK7eOXnDrT0KyOf3N9Pcks2CW8di3bMG5o28ZP6uCqYF5wOUl 2Cyigs29xbXr5V0ThWePUZs1s36kbY8zEE/7eY6K+ujFL/yQ1BD/RBPB6xz6ttrm3MWp5J o+NfkBeFn4W/C6bDo7A6JJU4NB8RjqlhLgzP7abDD1bZyQo7UhUFMkNPWQYuIg== ARC-Seal: i=1; s=key1; d=yhetil.org; t=1616532592; a=rsa-sha256; cv=none; b=uN9eiZpi6TRap9/aqD1rMke5cdGDAaRb6mxUEivil4/QmfLw8E/v8HqkXhBMQ3tlu+4DEh C8rS7zptyJmp3l/lqx8TnaJGioUFD235D4rbt3GAKqkkfGMHvYQI2vXuOYgyDaGNHLLej1 wKjUWnPq6llq3Fvh63sXu2EWXMXC7TtHUXFJkbBe/S1akVHS6n3aTt8Dw3AX971dr2IWVC B6798ZbHkG2DwrhB6szBS/6U3sqh2njVg8tXPknT8Dgo1vbpY20icVfmsCYoVYDnDkrfyf 7cwxlpTV7msoy4pua7UHt4JPlBE1ls32j4n6RhpsY33OSWNnXONi4iFpJvzu4Q== ARC-Authentication-Results: i=1; aspmx1.migadu.com; dkim=none; dmarc=none; spf=pass (aspmx1.migadu.com: domain of bug-guix-bounces@gnu.org designates 209.51.188.17 as permitted sender) smtp.mailfrom=bug-guix-bounces@gnu.org X-Migadu-Spam-Score: -4.52 Authentication-Results: aspmx1.migadu.com; dkim=none; dmarc=none; spf=pass (aspmx1.migadu.com: domain of bug-guix-bounces@gnu.org designates 209.51.188.17 as permitted sender) smtp.mailfrom=bug-guix-bounces@gnu.org X-Migadu-Queue-Id: 3A28C1BB36 X-Spam-Score: -4.52 X-Migadu-Scanner: scn0.migadu.com X-TUID: Q1+u9gJxnLsI --=-=-= Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Ludovic Court=C3=A8s writes: > Christopher Baines skribis: > >> Ludovic Court=C3=A8s writes: >> >>> Indeed, there=E2=80=99s one place on the hot path where we install exce= ption >>> handlers: in =E2=80=98http-multiple-get=E2=80=99 (from commit >>> 205833b72c5517915a47a50dbe28e7024dc74e57). I don=E2=80=99t think it=E2= =80=99s needed, >>> is it? (But if it is, let=E2=80=99s find another approach, this one is >>> prohibitively expensive.) >> >> I think the exception handling has moved around, but I guess the >> exceptions that could be caught in http-multiple-get could happen, >> right? I am really just guessing here, as Guile doesn't help tell you >> about possible exceptions, and I haven't spent enough time to read all >> the possible code involved to find out if these are definitely possible. > > Yeah. > > Commit 205833b72c5517915a47a50dbe28e7024dc74e57 added a =E2=80=98catch=E2= =80=99 block > that catches the same things as =E2=80=98with-cached-connection=E2=80=99 = did (it would > be better to not duplicate it IMO). That includes EPIPE, gnutls-error, > bad-response & co. So, my intention here was to move the error handling, to allow separating out the connection caching code from the code I wanted to move out to the (guix substitutes) module. I don't think there's currently duplication in the error handling for the code path involving http-multiple-get currently, at least for the exceptions in question here. > Earlier, commit be5a75ebb5988b87b2392e2113f6590f353dd6cd (=E2=80=9Csubsti= tute: > Reuse connections for '--query'.=E2=80=9D) did not add such a =E2=80=98ca= tch=E2=80=99 block in > =E2=80=98http-multiple-get=E2=80=99. Instead, it wrapped its call in =E2= =80=98do-fetch=E2=80=99 in > =E2=80=98fetch-narinfos=E2=80=99: > > (define (do-fetch uri) > (case (and=3D> 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-resp= onse '() > requests > + #:open-connection > + open-connection-for= -uri/cached > #:verify-certificat= e? #f > - #:port port))) > > This bit is still there in current =E2=80=98master=E2=80=99, so I think i= t=E2=80=99s not > necessary to catch these exceptions in =E2=80=98http-multiple-get=E2=80= =99 itself, and I > would just remove the =E2=80=98catch=E2=80=99 wrap altogether. > > WDYT? I'm not sure what you're referring to as still being there on the master branch? 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. 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. I think it's possible that http-multiple-get will be making thousands of requests, running guix weather with no cached results for example. The error handling in http-multiple-get is smarter than the previous approach, doing as little as possible again. It's also not limited to catching one exception. --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQKlBAEBCgCPFiEEPonu50WOcg2XVOCyXiijOwuE9XcFAmBaU9BfFIAAAAAALgAo aXNzdWVyLWZwckBub3RhdGlvbnMub3BlbnBncC5maWZ0aGhvcnNlbWFuLm5ldDNF ODlFRUU3NDU4RTcyMEQ5NzU0RTBCMjVFMjhBMzNCMEI4NEY1NzcRHG1haWxAY2Jh aW5lcy5uZXQACgkQXiijOwuE9XdvCxAAtsUzNiHvJL2S0jf1/VJUdqZw9VSb91BM GnyMd5hXQIe0ONd3r6XkdLdPpe9JT2Lh18U0F6fvJfk+zhDFWIAPZE9PiyweBarf 0ggJH45zr8nxbIrPp/dOuDvuCEhnEyovK0bPdvKQfzkC67ZQdlfkjnPjDKbtactA l2hgWdtoT7wN0QdwFpa45fQhbP1zHWh5Wd0kItyxSjM3B9xpnoYHP/IryuTUhtVd jx18SFdPKTHUYwWpESrjRM79tE53IfcChYz7b5toUgDnGSM63nZQHbLbmVcQXLrO m8rRhTcf3ZbDqg5qQr7RAQfwucO/1C9NIkP6Gjn0Hnk2e/q6fHdhLpjIDwmQy2S2 yIx28JcpKB4ua+ToyVDsEZGuJiacJan5P306B7LjrDVx8BRWF36Xt3PZEV2kHKrd uFwOlGe829zYsXtp45FnVRd4wHzP9uqDnQoh4f4vyLaoKqB/Wx4UNb0fqIJ4mY5Y lHIlbXIUpkEAb4ON9jm2D81bWUCFcfLTJj5mkrXer3J8II2pcg/Md7MgmyhXcbOa zYq5N3TyHaQs5PeWhuS3oXUePjnpxse0oAtH3V0cm8iGa/iggWUmC6+eu7b/X1v/ Fo3RXgu5/Zbx6xvVtrzSiq4s9CZKLh1qhUjSZBlG5SquHg2WJNSKZ3/Grwi+x78s ITZ9f0QQh4E= =O8Tt -----END PGP SIGNATURE----- --=-=-=--