Patrick SteinhardtIn 7a5d604443 (commit: detect commits that exist in commit-graph but not in the ODB, 2023-10-31), we have introduced a new object existence check into `repo_parse_commit_internal()` so that we do not parse commits via the commit-graph that don't have a corresponding object in the object database. This new check of course comes with a performance penalty, which the commit put at around 30% for `git rev-list --topo-order`. But there are in fact scenarios where the performance regression is even higher. The following benchmark against linux.git with a fully-build commit-graph:
Benchmark 1: git.v2.42.1 rev-list --count HEAD
Time (mean ± σ): 658.0 ms ± 5.2 ms [User: 613.5 ms, System: 44.4 ms]
Range (min … max): 650.2 ms … 666.0 ms 10 runs Benchmark 2: git.v2.43.0-rc1 rev-list --count HEAD
Time (mean ± σ): 1.333 s ± 0.019 s [User: 1.263 s, System: 0.069 s]
Range (min … max): 1.302 s … 1.361 s 10 runs Summary
git.v2.42.1 rev-list --count HEAD ran
2.03 ± 0.03 times faster than git.v2.43.0-rc1 rev-list --count HEADWhile it's a noble goal to ensure that results are the same regardless of whether or not we have a potentially stale commit-graph, taking twice as much time is a tough sell. Furthermore, we can generally assume that the commit-graph will be updated by git-gc(1) or git-maintenance(1) as required so that the case where the commit-graph is stale should not at all be common.
With that in mind, default-disable GIT_COMMIT_GRAPH_PARANOIA and restore the behaviour and thus performance previous to the mentioned commit. In order to not be inconsistent, also disable this behaviour by default in `lookup_commit_in_graph()`, where the object existence check has been introduced right at its inception via f559d6d45e (revision: avoid hitting packfiles when commits are in commit-graph, 2021-08-09).
This results in another speedup in commands that end up calling this function, even though it's less pronounced compared to the above benchmark. The following has been executed in linux.git with ~1.2 million references:
Benchmark 1: GIT_COMMIT_GRAPH_PARANOIA=true git rev-list --all --no-walk=unsorted
Time (mean ± σ): 2.947 s ± 0.003 s [User: 2.412 s, System: 0.534 s]
Range (min … max): 2.943 s … 2.949 s 3 runs Benchmark 2: GIT_COMMIT_GRAPH_PARANOIA=false git rev-list --all --no-walk=unsorted
Time (mean ± σ): 2.724 s ± 0.030 s [User: 2.207 s, System: 0.514 s]
Range (min … max): 2.704 s … 2.759 s 3 runs Summary
GIT_COMMIT_GRAPH_PARANOIA=false git rev-list --all --no-walk=unsorted ran
1.08 ± 0.01 times faster than GIT_COMMIT_GRAPH_PARANOIA=true git rev-list --all --no-walk=unsortedSo whereas 7a5d604443 initially introduced the logic to start doing an object existence check in `repo_parse_commit_internal()` by default, the updated logic will now instead cause `lookup_commit_in_graph()` to stop doing the check by default. This behaviour continues to be tweakable by the user via the GIT_COMMIT_GRAPH_PARANOIA environment variable.
Reported-by: Jeff King <peff@peff.net> Signed-off-by: Patrick Steinhardt <ps@pks.im> --- Documentation/git.txt | 6 +++--- commit-graph.c | 2 +- commit.c | 2 +- t/t5318-commit-graph.sh | 8 ++++---- 4 files changed, 9 insertions(+), 9 deletions(-)
Show changes to 4 files +9 −9
Documentation/git.txt, commit-graph.c, commit.c, t/t5318-commit-graph.sh
diff --git a/Documentation/git.txt b/Documentation/git.txt index 2535a30194..6c19fd1d76 100644 --- a/Documentation/git.txt +++ b/Documentation/git.txt @@ -917,9 +917,9 @@ for full details. avoid issues with stale commit-graphs that contain references to already-deleted commits, but comes with a performance penalty. + -The default is "true", which enables the aforementioned behavior. -Setting this to "false" disables the existence check. This can lead to -a performance improvement at the cost of consistency. +The default is "false", which disables the aforementioned behavior. +Setting this to "true" enables the existence check so that stale commits +will never be returned from the commit-graph at the cost of performance. `GIT_ALLOW_PROTOCOL`:: If set to a colon-separated list of protocols, behave as if diff --git a/commit-graph.c b/commit-graph.c index ee66098e07..a712917356 100644 --- a/commit-graph.c +++ b/commit-graph.c @@ -1029,7 +1029,7 @@ struct commit *lookup_commit_in_graph(struct repository *repo, const struct obje uint32_t pos; if (commit_graph_paranoia == -1) - commit_graph_paranoia = git_env_bool(GIT_COMMIT_GRAPH_PARANOIA, 1); + commit_graph_paranoia = git_env_bool(GIT_COMMIT_GRAPH_PARANOIA, 0); if (!prepare_commit_graph(repo)) return NULL; diff --git a/commit.c b/commit.c index 8405d7c3fc..37956b836c 100644 --- a/commit.c +++ b/commit.c @@ -577,7 +577,7 @@ int repo_parse_commit_internal(struct repository *r, static int commit_graph_paranoia = -1; if (commit_graph_paranoia == -1) - commit_graph_paranoia = git_env_bool(GIT_COMMIT_GRAPH_PARANOIA, 1); + commit_graph_paranoia = git_env_bool(GIT_COMMIT_GRAPH_PARANOIA, 0); if (commit_graph_paranoia && !has_object(r, &item->object.oid, 0)) { unparse_commit(r, &item->object.oid); diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh index d4fc65e078..4c751a6871 100755 --- a/t/t5318-commit-graph.sh +++ b/t/t5318-commit-graph.sh @@ -909,10 +909,10 @@ test_expect_success 'stale commit cannot be parsed when given directly' ' # Verify that it is possible to read the commit from the # commit graph when not being paranoid, ... - GIT_COMMIT_GRAPH_PARANOIA=false git rev-list B && + git rev-list B && # ... but parsing the commit when double checking that # it actually exists in the object database should fail. - test_must_fail git rev-list -1 B + test_must_fail env GIT_COMMIT_GRAPH_PARANOIA=true git rev-list -1 B ) ' @@ -936,9 +936,9 @@ test_expect_success 'stale commit cannot be parsed when traversing graph' ' # Again, we should be able to parse the commit when not # being paranoid about commit graph staleness... - GIT_COMMIT_GRAPH_PARANOIA=false git rev-parse HEAD~2 && + git rev-parse HEAD~2 && # ... but fail when we are paranoid. - test_must_fail git rev-parse HEAD~2 2>error && + test_must_fail env GIT_COMMIT_GRAPH_PARANOIA=true git rev-parse HEAD~2 2>error && grep "error: commit $oid exists in commit-graph but not in the object database" error ) '
-- 2.42.0
