{"thread":{"id":"28341","subject":"[PATCH] Disable useless check_for_new_submodule_commits() by default","startedAt":"2011-09-09T01:56:56Z","lastAt":"2011-09-09T16:29:21Z","messageCount":2,"participants":["Junio C Hamano","Jens Lehmann"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"175142","messageId":"7vy5xyv5p3.fsf@alter.siamese.dyndns.org","threadId":"28341","inReplyTo":null,"subject":"[PATCH] Disable useless check_for_new_submodule_commits() by default","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-09-09T01:56:56Z","receivedAt":"2011-09-09T01:56:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Imagine a project with 20k paths with a history 250k commits deep, without\nany submodule.\n\nImagine another project with the same depth of history but with 1k paths,\namong which there are 200 submodules.\n\nFurther, imagine fetching from one of the above repositories into a\nrepository that is very behind, and updating many remote tracking\nbranches.  Now, think about what check_for_new_submodule_commits() in\nsubmodule.c that is called from update_local_ref() in builtin/fetch.c\nwould do.\n\nFor each updated remote tracking branch (or anything that is not a tag),\nthe problem function will run the equivalent of:\n\n\tgit log --raw $new --not --all\n\nwhich would mean 250k rounds of diff-tree to enumerate the submodules that\nmay have been updated, and it does it for each and every refs outside the\nrefs/tags hierarchy.\n\nPresumably, this is so that it only has to actually fetch in the submodule\n\"on demand\", but even if in a project _with_ submodules (i.e. the latter\nexample above), this is simply not acceptable. You could just enumerate\nthose 200 submodules at the tip that _might_ matter and that would be\nmillion times cheaper. To add insult to injury, this \"on demand\" behaviour\nis on by default, which hurts projects without any submodules (i.e. the\nformer example above) a lot.\n\nIn short, if \"on demand\" check is million times more expensive than\nactually doing it, the check does not have any value.\n\nIn the longer term, people who want to have the on-demand behaviour need\nto come up with a cheaper way to determine if it is necessary to recurse\ninto submodules by fixing check_for_new_submodule_commits(), but until\nthat happens, we should disable submodule recursion by default unless the\nuser explicitly asks for it from the command line or from the configuration.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n * This is meant for 1.7.6.X maintenance track to fix the huge regression\n   caused by 88a2197 (fetch/pull: recurse into submodules when necessary,\n   2011-03-06).  I would very much appreciate a simpler and trivially\n   correct patch as a counter-proposal that makes sure that the expensive\n   check_for_new_submodule_commits() and fetch_populated_submodules()\n   functions are never called when the user does not have any command line\n   option or configuration variable to tell otherwise. I suspect that this\n   patch may be disabling more things than absolutely necessary.\n\n   I also suspect that it might be just a matter of finding any and all\n   comparison of the form:\n\n\tif (recurse != ON && recurse != OFF)\n\n   that was present before the on-demand stuff and replace that with\n        \n\tif (recurse == ON_DEMAND)\n\n   but I didn't look very closely. At least this patch is much less error\n   prone for the purpose of not triggering the expensive and pointless\n   check ;-)\n\n builtin/fetch.c |    2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 93c9938..71232c1 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -29,7 +29,7 @@ enum {\n };\n \n static int all, append, dry_run, force, keep, multiple, prune, update_head_ok, verbosity;\n-static int progress, recurse_submodules = RECURSE_SUBMODULES_DEFAULT;\n+static int progress, recurse_submodules = RECURSE_SUBMODULES_OFF;\n static int tags = TAGS_DEFAULT;\n static const char *depth;\n static const char *upload_pack;\n"},{"id":"175213","messageId":"4E6A3EE1.9080103@web.de","threadId":"28341","inReplyTo":"7vy5xyv5p3.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Disable useless check_for_new_submodule_commits() by default","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2011-09-09T16:29:21Z","receivedAt":"2011-09-09T16:29:21Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 09.09.2011 03:56, schrieb Junio C Hamano:\n> Imagine a project with 20k paths with a history 250k commits deep, without\n> any submodule.\n> \n> Imagine another project with the same depth of history but with 1k paths,\n> among which there are 200 submodules.\n> \n> Further, imagine fetching from one of the above repositories into a\n> repository that is very behind, and updating many remote tracking\n> branches.  Now, think about what check_for_new_submodule_commits() in\n> submodule.c that is called from update_local_ref() in builtin/fetch.c\n> would do.\n> \n> For each updated remote tracking branch (or anything that is not a tag),\n> the problem function will run the equivalent of:\n> \n> \tgit log --raw $new --not --all\n> \n> which would mean 250k rounds of diff-tree to enumerate the submodules that\n> may have been updated, and it does it for each and every refs outside the\n> refs/tags hierarchy.\n>\n> Presumably, this is so that it only has to actually fetch in the submodule\n> \"on demand\", but even if in a project _with_ submodules (i.e. the latter\n> example above), this is simply not acceptable.\n\nDefinitely.\n\n> You could just enumerate\n> those 200 submodules at the tip that _might_ matter and that would be\n> million times cheaper.\n\nBut that trick won't do it. I thought about that too when I implemented this\nfeature, but we need to look in all new commits (e.g. a submodule is not\npresent anymore in the new tip but is changed in the preceding but not yet\nfetched commits, then it still has to be checked if it has the necessary\ncommits locally and they need to be fetched if that is not the case).\n\n> To add insult to injury, this \"on demand\" behaviour\n> is on by default, which hurts projects without any submodules (i.e. the\n> former example above) a lot.\n\nRight, I'll post a patch disabling that check completely when a project has\nno submodules.\n\n> In short, if \"on demand\" check is million times more expensive than\n> actually doing it, the check does not have any value.\n> \n> In the longer term, people who want to have the on-demand behaviour need\n> to come up with a cheaper way to determine if it is necessary to recurse\n> into submodules by fixing check_for_new_submodule_commits(), but until\n> that happens, we should disable submodule recursion by default unless the\n> user explicitly asks for it from the command line or from the configuration.\n\nOk, I'll see if I can come up with a solution for the performance regression.\n"}]}