From patchwork Thu Sep 9 20:25:46 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: M X-Patchwork-Id: 32731 Return-Path: X-Original-To: patchwork@mira.cbaines.net Delivered-To: patchwork@mira.cbaines.net Received: by mira.cbaines.net (Postfix, from userid 113) id 1EDB227BBE3; Thu, 9 Sep 2021 21:31:44 +0100 (BST) X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on mira.cbaines.net X-Spam-Level: X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED, FREEMAIL_FROM,MAILING_LIST_MULTI,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS, T_DKIM_INVALID,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.2 Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) by mira.cbaines.net (Postfix) with ESMTPS id 1BA6127BBE1 for ; Thu, 9 Sep 2021 21:31:42 +0100 (BST) Received: from localhost ([::1]:45166 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1mOQhv-00089m-4g for patchwork@mira.cbaines.net; Thu, 09 Sep 2021 16:31:41 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:46468) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mOQdS-0007LB-5s for guix-patches@gnu.org; Thu, 09 Sep 2021 16:27:02 -0400 Received: from debbugs.gnu.org ([209.51.188.43]:53000) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1mOQdR-0000Yw-U9 for guix-patches@gnu.org; Thu, 09 Sep 2021 16:27:01 -0400 Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1mOQdR-0007I2-OW for guix-patches@gnu.org; Thu, 09 Sep 2021 16:27:01 -0400 X-Loop: help-debbugs@gnu.org Subject: [bug#50384] [PATCH v3] Optimise search-patch (reducing I/O) Resent-From: Maxime Devos Original-Sender: "Debbugs-submit" Resent-CC: guix-patches@gnu.org Resent-Date: Thu, 09 Sep 2021 20:27:01 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 50384 X-GNU-PR-Package: guix-patches X-GNU-PR-Keywords: patch To: 50384@debbugs.gnu.org Cc: Ludovic =?utf-8?q?Court=C3=A8s?= , zimoun Received: via spool by 50384-submit@debbugs.gnu.org id=B50384.163121918227978 (code B ref 50384); Thu, 09 Sep 2021 20:27:01 +0000 Received: (at 50384) by debbugs.gnu.org; 9 Sep 2021 20:26:22 +0000 Received: from localhost ([127.0.0.1]:36313 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1mOQce-0007H3-SS for submit@debbugs.gnu.org; Thu, 09 Sep 2021 16:26:22 -0400 Received: from albert.telenet-ops.be ([195.130.137.90]:56352) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1mOQca-0007Gs-JA for 50384@debbugs.gnu.org; Thu, 09 Sep 2021 16:26:11 -0400 Received: from [172.20.10.4] ([213.119.128.77]) by albert.telenet-ops.be with bizsmtp id rwS52500M1gKkhx06wS6Ps; Thu, 09 Sep 2021 22:26:06 +0200 Message-ID: <04603bca34f16b284a5e3052a4b0765b60952817.camel@telenet.be> From: Maxime Devos Date: Thu, 09 Sep 2021 22:25:46 +0200 In-Reply-To: <8900fa8c8eef7f72fc97adc2408be26c88de7803.camel@telenet.be> References: <8900fa8c8eef7f72fc97adc2408be26c88de7803.camel@telenet.be> User-Agent: Evolution 3.34.2 MIME-Version: 1.0 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=telenet.be; s=r21; t=1631219166; bh=NeSWZ9++rmDMz2rlTfIoHLEwpHJGgCJEl8ZYXLxYMlI=; h=Subject:From:To:Cc:Date:In-Reply-To:References; b=b/tCOePDAsJ7fkeqhWgY+RI6XiuqClxqEW5nEPxfnMMyVQ4j6yv9whOKbOZpkNe+g UAL17Ez7pEc1/tCV08LQzksy6nQ8t1rpAP2ExAuYF0MpczsN+9265BLjB16nXzFhuo 9PtAMEJqHJTMAqVHJFbHjzvDUecp5G/e8683GWzNv/2VcvzN36wl7jwetYjsArKuCb pg0YBBgL7Q9Y7mr5HLYPahDQa8hJ+4mxiwzZsdn+xHYqwDbryf7oLXkgE6AGPmGq75 zYGB70vnDij5RTEtb4RWt+Van/EN0YwYbaOwUctUmd8jMoWJjltWxtyaI2kyCBO6VA 4ylSZ0mWw3QmA== X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: guix-patches@gnu.org List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guix-patches-bounces+patchwork=mira.cbaines.net@gnu.org Sender: "Guix-patches" X-getmail-retrieved-from-mailbox: Patches Hi guix, This is a v3, without the base16 and base32 optimisations which are split-off into . It doesn't seem this patch series will bring improvements, but feel free to test (in particular, I wonder if this will help people using a remote daemon, where transmitting data can take (relatively) long?). (guix scripts hash) is broken, which would need to be fixed in the final version, if any. Ludovic has some concerns about dependency tracking in search-patch which need to be adressed. I think a more fruitful goal is to somehow parallelize the derivation computation, with multiple separate connections to the store, such that if one connection is blocking, the other one can be used for something separate (threads aren't necessary if current-read-waiter, current-write-waiter and non-blocking I/O are used). Now, what improvements does this version of the patch series bring? (Make sure to start the daemon with ./pre-inst-env guix daemon ..., and set --localstatedir=/var! Some changes to the daemon were made.) 1. RPC count (tested in a local checkout) After the patch series: make && GUIX_PROFILING=rpc ./pre-inst-env guix build -d pigx --no-grafts accepted connection from pid 4917, user [USER] /gnu/store/jfjfg7dnis7v6947a0rncxdn3y1nz0ad-pigx-0.0.3.drv Remote procedure call summary: 5754 RPCs built-in-builders ... 1 add-to-store ... 3 add-to-store/tree ... 26 add-temp-root-and-valid-path? ... 195 add-text-to-store ... 5529 After the patch series, with (if sha256 ...) replaced with (if #f ...) in (guix gexp), to simulate the situation before the patch series /gnu/store/jfjfg7dnis7v6947a0rncxdn3y1nz0ad-pigx-0.0.3.drv Remote procedure call summary: 5749 RPCs built-in-builders ... 1 add-to-store/tree ... 26 add-to-store ... 193 add-text-to-store ... 5529 (add-to-store RPCs are converted to add-temp-root-and-valid-path? RPCs) 2. Timing First do echo powersave | sudo tee /sys/devices/system/cpu/cpu{0,1,2,3}/cpufreq/scaling_governor to eliminate CPU frequency scaling effects. To automatically repeat the tests and compute the standard deviation, 'hyperfine' is used: HYP=/gnu/store/3ya4iw6fzq1ns73bv1g3a96jvwhbv60c-hyperfine-1.11.0/bin/hyperfine To determine the effect of the change to 'local-file-compiler' and 'search-patch' and nothing else, I will compare the performance of guix after the patch series with the performance of guix after the patch series and 'sha256' replaced by #false. With #f, --runs=60: make && ./pre-inst-env $HYP --runs=60 --warmup 1 -- 'guix build -d pigx --no-grafts' Time (mean ± σ): 15.428 s ± 0.385 s [User: 15.925 s, System: 0.652 s] Range (min … max): 14.768 s … 16.550 s 60 runs With sha256, --runs=60 make && ./pre-inst-env $HYP --runs=60 --warmup 1 -- 'guix build -d pigx --no-grafts' Time (mean ± σ): 15.493 s ± 0.252 s [User: 15.585 s, System: 0.680 s] Range (min … max): 14.981 s … 16.294 s 60 runs These numbers don't have a clear difference. Maybe statistics can help? First, formulate a null-hypothesis. As the total number of RPCs didn't change, the amount of data sent to the daemon is reduced and some "stats", "open" and "reads" are avoided, I would expect that the mean decreases. Thus, as null-hypothesis, I choose: H0: the (theoretical) mean for ‘with sha256’ is less than the mean for ‘with #f’ In the timing tests, the observed mean for 'with sha256’ is actually larger. But is this significant? guix environment --ad-hoc r before.mean = 15.428 before.stddev = 0.385 after.mean = 15.493 after.stddev = 0.252 samples = 60 # ‘statistical’ crate used by hyperfine # performs N/(N-1) correction XXX t = (before.mean - after.mean)/(sqrt(samples) * sqrt(before.stddev^2 + after.stddev^2)) v = (samples - 1) * (before.stddev^2 + after.stddev^2)^2/(before.stddev^4 + after.stddev^4) q = dt(-t, v); q # p-value: 0.5072571 # Null-hypothesis is not rejected It's not rejected, though that doesn't prove much since t is almost zero, so this test cannot reject the hypothesis ‘the means are equal’ or ‘the patch series makes things slower’ either. I don't think this patch series helps on my laptop (at least on a hot disk cache, I'd have to check for a cold cache). However, I wonder if this would help a little for people using a remote build daemon (with a nfs setup or something) (see GUIX_DAEMON_SOCKET)? Greetings, Maxime. From d359fefabf2831e42aea6edf646a9e0373be5d0f Mon Sep 17 00:00:00 2001 From: Maxime Devos Date: Sat, 4 Sep 2021 18:10:32 +0200 Subject: [PATCH 10/10] gexp: Do not intern if the file is already in the store. * guix/gexp.scm (local-file-compiler): When the file is already in the store, re-use the fixed output path instead of interning the file again. * guix/gexp.scm (add-temp-root-and-valid-path?*): New procedure. --- guix/gexp.scm | 32 +++++++++++++++++++++++++------- 1 file changed, 25 insertions(+), 7 deletions(-) diff --git a/guix/gexp.scm b/guix/gexp.scm index c69e4aa299..20c9d93170 100644 --- a/guix/gexp.scm +++ b/guix/gexp.scm @@ -528,16 +528,34 @@ appears." 'system-error' exception is raised if FILE could not be found." (force (%local-file-absolute-file-name file))) +(define add-temp-root-and-valid-path?* (store-lift add-temp-root-and-valid-path?)) + (define-gexp-compiler (local-file-compiler (file ) system target) ;; "Compile" FILE by adding it to the store. (match file - (($ file (= force absolute) name sha256 recursive? select?) - ;; Canonicalize FILE so that if it's a symlink, it is resolved. Failing - ;; to do that, when RECURSIVE? is #t, we could end up creating a dangling - ;; symlink in the store, and when RECURSIVE? is #f 'add-to-store' would - ;; just throw an error, both of which are inconvenient. - (interned-file absolute name - #:recursive? recursive? #:select? select?)))) + ;; Delay computing the absolute file name until 'intern', as this + ;; might be a relatively expensive computation (e.g. if search-patch + ;; is used), especially on a spinning disk. + (($ file absolute-promise name sha256 recursive? select?) + (let () + (define (intern) + ;; Canonicalize FILE so that if it's a symlink, it is resolved. + ;; Failing to do that, when RECURSIVE? is #t, we could end up creating + ;; a dangling symlink in the store, and when RECURSIVE? is #f + ;; 'add-to-store' would just throw an error, both of which are + ;; inconvenient. + (interned-file (force absolute-promise) name + #:recursive? recursive? #:select? select?)) + ;; If the hash is known in advance and the store already has the + ;; item, there is no need to intern the file. + (if sha256 + (let ((path (fixed-output-path name sha256 #:recursive? recursive?))) + (mlet %store-monad ((valid? (add-temp-root-and-valid-path?* path))) + (if valid? + (return path) + (intern)))) + ;; If PATH does not yet exist, fall back to interning. + (intern)))))) (define-record-type (%plain-file name content references) -- 2.33.0