[v2] fetch: do not look for submodule changes in unchanged refs

Commit Message

Philippe Blain via GitGitGadget Sept. 4, 2020, 1:50 p.m. UTC

From: Orgad Shaneh <[e-mail]>

When fetching recursively with submodules, for each ref in the
superproject, we call check_for_new_submodule_commits() which collects all
the objects that have to be checked for submodule changes on
calculate_changed_submodule_paths(). On the first call, it also collects all
the existing refs for excluding them from the scan.

calculate_changed_submodule_paths() creates an argument array with all the
collected new objects, followed by --not and all the old objects. This argv
is passed to setup_revisions, which parses each argument, converts it back
to an oid and resolves the object. The parsing itself also does redundant
work, because it is treated like user input, while in fact it is a full
oid. So it needlessly attempts to look it up as ref (checks if it has ^, ~
etc.), checks if it is a file name etc.

For a repository with many refs, all of this is expensive. But if the fetch
in the superproject did not update the ref (i.e. the objects that are
required to exist in the submodule did not change), there is no need to
include it in the list.

Before commit be76c212 (fetch: ensure submodule objects fetched,
2018-12-06), submodule reference changes were only detected for refs that
were changed, but not for new refs. This commit covered also this case, but
what it did was to just include every ref.

This change should reduce the number of scanned refs by about half (except
the case of a no-op fetch, which will not scan any ref), because all the
existing refs will still be listed after --not.

The regression was reported here:
https://public-inbox.org/git/CAGHpTBKSUJzFSWc=uznSu2zB33qCSmKXM-
[e-mail]/

Signed-off-by: Orgad Shaneh <[e-mail]>
---
    fetch: do not look for submodule changes in unchanged refs
    
    This operation is very expensive, as it scans all the refs using
    setup_revisions, which resolves each ref, including checking if it is
    ambiguous, or if it is a file name etc.
    
    There is no reason to do all that for refs that hasn't changed in this
    fetch.
    
    Reported here:
    https://public-inbox.org/git/CAGHpTBKSUJzFSWc=[e-mail]/

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-720%2Forgads%2Ffetch-less-submodules-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-720/orgads/fetch-less-submodules-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/720

Range-diff vs v1:

 1:  0f978f487d ! 1:  5348cd7ec8 fetch: do not look for submodule changes in unchanged refs
     @@ Metadata
       ## Commit message ##
          fetch: do not look for submodule changes in unchanged refs
      
     -    This operation is very expensive, as it scans all the refs using
     -    setup_revisions, which resolves each ref, including checking if it
     -    is ambiguous, or if it is a file name etc.
     +    When fetching recursively with submodules, for each ref in the
     +    superproject, we call check_for_new_submodule_commits() which collects all
     +    the objects that have to be checked for submodule changes on
     +    calculate_changed_submodule_paths(). On the first call, it also collects all
     +    the existing refs for excluding them from the scan.
      
     -    There is no reason to do all that for refs that haven't changed in this
     -    fetch.
     +    calculate_changed_submodule_paths() creates an argument array with all the
     +    collected new objects, followed by --not and all the old objects. This argv
     +    is passed to setup_revisions, which parses each argument, converts it back
     +    to an oid and resolves the object. The parsing itself also does redundant
     +    work, because it is treated like user input, while in fact it is a full
     +    oid. So it needlessly attempts to look it up as ref (checks if it has ^, ~
     +    etc.), checks if it is a file name etc.
      
     -    Reported here:
     -    https://public-inbox.org/git/CAGHpTBKSUJzFSWc=[e-mail]/
     +    For a repository with many refs, all of this is expensive. But if the fetch
     +    in the superproject did not update the ref (i.e. the objects that are
     +    required to exist in the submodule did not change), there is no need to
     +    include it in the list.
      
     -    Amends commit be76c2128234d94b47f7087152ee55d08bb65d88.
     +    Before commit be76c212 (fetch: ensure submodule objects fetched,
     +    2018-12-06), submodule reference changes were only detected for refs that
     +    were changed, but not for new refs. This commit covered also this case, but
     +    what it did was to just include every ref.
     +
     +    This change should reduce the number of scanned refs by about half (except
     +    the case of a no-op fetch, which will not scan any ref), because all the
     +    existing refs will still be listed after --not.
     +
     +    The regression was reported here:
     +    https://public-inbox.org/git/CAGHpTBKSUJzFSWc=uznSu2zB33qCSmKXM-
     +    [e-mail]/
      
          Signed-off-by: Orgad Shaneh <[e-mail]>
      


 builtin/fetch.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)


base-commit: e19713638985533ce461db072b49112da5bd2042

Patch

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 0f23dd4b8c..d3f922fc89 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -958,8 +958,10 @@  static int store_updated_refs(const char *raw_url, const char *remote_name,
 				ref->force = rm->peer_ref->force;
 			}
 
-			if (recurse_submodules != RECURSE_SUBMODULES_OFF)
+			if (recurse_submodules != RECURSE_SUBMODULES_OFF &&
+			    (!rm->peer_ref || !oideq(&ref->old_oid, &ref->new_oid))) {
 				check_for_new_submodule_commits(&rm->old_oid);
+			}
 
 			if (!strcmp(rm->name, "HEAD")) {
 				kind = "";