From fbab552a53a2558ec84a06034e6657fb95227266 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 12 Sep 2019 10:44:34 -0400 Subject: [PATCH] commit-graph: bump DIE_ON_LOAD check to actual load-time MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Commit 43d3561805 (commit-graph write: don't die if the existing graph is corrupt, 2019-03-25) added an environment variable we use only in the test suite, $GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD. But it put the check for this variable at the very top of prepare_commit_graph(), which is called every time we want to use the commit graph. Most importantly, it comes _before_ we check the fast-path "did we already try to load?", meaning we end up calling getenv() for every single use of the commit graph, rather than just when we load. getenv() is allowed to have unexpected side effects, but that shouldn't be a problem here; we're lazy-loading the graph so it's clear that at least _one_ invocation of this function is going to call it. But it is inefficient. getenv() typically has to do a linear search through the environment space. We could memoize the call, but it's simpler still to just bump the check down to the actual loading step. That's fine for our sole user in t5318, and produces this minor real-world speedup: [before] Benchmark #1: git -C linux rev-list HEAD >/dev/null Time (mean ± σ): 1.460 s ± 0.017 s [User: 1.174 s, System: 0.285 s] Range (min … max): 1.440 s … 1.491 s 10 runs [after] Benchmark #1: git -C linux rev-list HEAD >/dev/null Time (mean ± σ): 1.391 s ± 0.005 s [User: 1.118 s, System: 0.273 s] Range (min … max): 1.385 s … 1.399 s 10 runs Of course that actual speedup depends on how big your environment is. We can game it like this: for i in $(seq 10000); do export dummy$i=$i done in which case I get: [before] Benchmark #1: git -C linux rev-list HEAD >/dev/null Time (mean ± σ): 6.257 s ± 0.061 s [User: 6.005 s, System: 0.250 s] Range (min … max): 6.174 s … 6.337 s 10 runs [after] Benchmark #1: git -C linux rev-list HEAD >/dev/null Time (mean ± σ): 1.403 s ± 0.005 s [User: 1.146 s, System: 0.256 s] Range (min … max): 1.396 s … 1.412 s 10 runs So this is really more about avoiding the pathological case than providing a big real-world speedup. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- commit-graph.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/commit-graph.c b/commit-graph.c index 9b02d2c426..baeaf0d1bf 100644 --- a/commit-graph.c +++ b/commit-graph.c @@ -468,14 +468,14 @@ static int prepare_commit_graph(struct repository *r) { struct object_directory *odb; - if (git_env_bool(GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD, 0)) - die("dying as requested by the '%s' variable on commit-graph load!", - GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD); - if (r->objects->commit_graph_attempted) return !!r->objects->commit_graph; r->objects->commit_graph_attempted = 1; + if (git_env_bool(GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD, 0)) + die("dying as requested by the '%s' variable on commit-graph load!", + GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD); + prepare_repo_settings(r); if (!git_env_bool(GIT_TEST_COMMIT_GRAPH, 0) &&