Slow pushes on 'pu' - even when up-to-date..

This seems to be because I'm now on 'pu' as of a day or two ago in order to test the abbrev logic, but lookie here:

    time git ls-remote ra.kernel.org:/pub/scm/linux/kernel/git/torvalds/linux
    .. shows all the branches and tags ..
    real 0m0.655s
    user 0m0.011s
    sys 0m0.004s

so the remote is fast to connect to, and with network connection overhead and everything, it's just over half a second. But then:

time git push ra.kernel.org:/pub/scm/linux/kernel/git/torvalds/linux

and it just sits there, and it's at 100% CPU the whole time, until it says:

    Everything up-to-date

    real 1m7.307s
    user 1m2.761s
    sys 0m0.475s

Whaa? It took a *minute* of CPU time to decide that everything was up-to-date?

That's just not right. The branch is entirely up-to-date:

git rev-parse HEAD
af79ad2b1f337a00aa150b993635b10bc68dc842

git ls-remote ra.kernel.org:/pub/scm/linux/kernel/git/torvalds/linux master af79ad2b1f337a00aa150b993635b10bc68dc842 refs/heads/master

so there should be no need for any history walking. But it sure is doing *something*. A minute of CPU time on my machine is actually a pretty damn big deal.

Looking at the trace, there's no IO - there's no back-and-forth about "I have this, do you have it?" or anything like that. The system call trace is just a lot of allocations, which I think means that "git push" is walking a lot of objects but not doing anything useful.

I bisected it to commit 60cd66f "push: change submodule default to check", which makes little sense since I have no submodules, but there you go.. Apparently RECURSE_SUBMODULES_CHECK is just terminally broken.

* sb/push-make-submodule-check-the-default (2016-08-24) 1 commit
- push: change submodule default to check

Turn the default of "push.recurseSubmodules" to "check".

Will hold to wait for hv/submodule-not-yet-pushed-fix

This reveals that the "check" mode is too inefficient to use in real projects, even in ones as small as git itself. cf. <[email protected]>

Quoted 8 lines · Stefan Beller

* sb/push-make-submodule-check-the-default (2016-08-24) 1 commit
- push: change submodule default to check

Turn the default of "push.recurseSubmodules" to "check".

Will hold to wait for hv/submodule-not-yet-pushed-fix

Quoted 3 lines

This reveals that the "check" mode is too inefficient to use in real projects, even in ones as small as git itself. cf. <[email protected]>

So maybe we should eject this series from pu as long as hv/submodule-not-yet-pushed-fix is ejected to enable you running pu happily.

This seems to be because I'm now on 'pu' as of a day or two ago in order to test the abbrev logic, but lookie here:

    time git ls-remote ra.kernel.org:/pub/scm/linux/kernel/git/torvalds/linux
    .. shows all the branches and tags ..
    real 0m0.655s
    user 0m0.011s
    sys 0m0.004s

so the remote is fast to connect to, and with network connection overhead and everything, it's just over half a second. But then:

time git push ra.kernel.org:/pub/scm/linux/kernel/git/torvalds/linux

Quoted 9 lines · Linus Torvalds

This seems to be because I'm now on 'pu' as of a day or two ago in order to test the abbrev logic, but lookie here:

    time git ls-remote ra.kernel.org:/pub/scm/linux/kernel/git/torvalds/linux
    .. shows all the branches and tags ..
    real 0m0.655s
    user 0m0.011s
    sys 0m0.004s

Quoted 4 lines

so the remote is fast to connect to, and with network connection overhead and everything, it's just over half a second. But then:

time git push ra.kernel.org:/pub/scm/linux/kernel/git/torvalds/linux

The reason behind this is when pushing to an address we do not easily have the remote refs to compare available. When pushing an existing ref it would be easy and could get a shortcut but it gets more complicated for new refs. Currently we fall back to walking the whole history since that is "the most correct way" we have. But obviously it is not a practical solution in any way.

I mentioned this fact when discussing the current state and my patches to make this check less painful. So we still need to think about a solution for this check when passing an address.

IMO: It's definitely not ready to be switched on as default, unless we find something a lot cheaper for the above case.

My idea of a solution goes like this:
* collect all SHA1's of the remotes refs
* check if we have them locally
* if not we abort and tell the user to fetch them somehow into local refs or disable the check
* when we have them locally we proceed passing those SHA1's as bases

    instead of --remotes=<name>

Quoted 7 lines

My idea of a solution goes like this:
* collect all SHA1's of the remotes refs
* check if we have them locally
* if not we abort and tell the user to fetch them somehow into local refs or disable the check
* when we have them locally we proceed passing those SHA1's as bases

    instead of --remotes=<name>

As I argued in [1], I think it's not just "this must be cheaper" but "this must not be enabled if submodules are not in use at all". Most repositories don't have submodules enabled at all, so anything that cause any extra traversal, even of a portion of the history, is going to be a net negative for a lot of people.

I think the only sane default is going to be some kind of heuristic that says "submodules are probably in use". Something like "is there a .gitmodules file" is not perfect (you can have gitlink entries without it), but it's a really cheap constant-time check.

[1] Quoted in
http://public-inbox.org/git/[email protected]/

Quoted 4 lines

I think the only sane default is going to be some kind of heuristic that says "submodules are probably in use". Something like "is there a .gitmodules file" is not perfect (you can have gitlink entries without it), but it's a really cheap constant-time check.

I agree. We are adding convenience for submodules, so we can also say a checked out ".gitmodules" file is a must to have convenience.

I am not sure if I agree on another layer of options for this as suggested in your post. More options mean more implementation complexity and more confusion on the users side.

How about we choose our defaults based on the existence of a checked out .gitmodules file? So the default would only be --recurse-submodules=check if there is a .gitmodules file in the worktree. All other users need to either pass or explicitly configure it.

As I argued in [1], I think it's not just "this must be cheaper" but "this must not be enabled if submodules are not in use at all". Most repositories don't have submodules enabled at all, so anything that cause any extra traversal, even of a portion of the history, is going to be a net negative for a lot of people.

I think the only sane default is going to be some kind of heuristic that says "submodules are probably in use".

Quoted 6 lines · Jeff King

As I argued in [1], I think it's not just "this must be cheaper" but "this must not be enabled if submodules are not in use at all". Most repositories don't have submodules enabled at all, so anything that cause any extra traversal, even of a portion of the history, is going to be a net negative for a lot of people.

Quoted 2 lines

I think the only sane default is going to be some kind of heuristic that says "submodules are probably in use".

Why should we even have a default different from today's? If most repositories don't have submodules enabled at all, we can just let those working with submodules enabled to toggle their configuration and that is an very easy to understand solution, no?

Quoted 5 lines · Junio C Hamano

Why should we even have a default different from today's? If most repositories don't have submodules enabled at all, we can just let those working with submodules enabled to toggle their configuration and that is an very easy to understand solution, no?

You will not see any complaint from me on that. I was taking for granted that the current default is inconvenient to submodule users, but I don't have any experience myself.

Quoted 9 lines

Quoted 4 lines

Why should we even have a default different from today's? If most repositories don't have submodules enabled at all, we can just let those working with submodules enabled to toggle their configuration and that is an very easy to understand solution, no?

You will not see any complaint from me on that. I was taking for granted that the current default is inconvenient to submodule users, but I don't have any experience myself.

And there I was trying to help submodule users not shoot in their foot.

builtin/push.c +1 -1

index 3bb9d6b..d7d664a 100644
@@ -22,7 +22,7 @@ static int deleterefs;
 static const char *receivepack;
 static int verbosity;
 static int progress = -1;
-static int recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
+static int recurse_submodules;
 static enum transport_family family;
 
 static struct push_cas_option cas;

Quoted 5 lines · Stefan Beller

Quoted 5 lines

Quoted 4 lines

Why should we even have a default different from today's? If most repositories don't have submodules enabled at all, we can just let those working with submodules enabled to toggle their configuration and that is an very easy to understand solution, no?

Quoted 6 lines

Quoted 4 lines

You will not see any complaint from me on that. I was taking for granted that the current default is inconvenient to submodule users, but I don't have any experience myself.

And there I was trying to help submodule users not shoot in their foot.

Sorry if my reply came off as snarky.

builtin/push.c static const char **refspec; · line 40

Quoted 8 lines

builtin/push.c +7 -0

+static void preset_submodule_default(void)
+{
+     if (file_exists(".gitmodules"))
+             recurse_submodules = RECURSE_SUBMODULES_CHECK;
+     else
+             recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
+}

This does seem like a reasonable heuristic. I wonder if you want to confirm that we actually have a worktree (and are in it) before looking at file_exists(). It's unlikely that looking at ".gitmodules" in a bare repo would trigger in practice, but it does not hurt to be careful.

-Peff

Quoted 4 lines

This does seem like a reasonable heuristic. I wonder if you want to confirm that we actually have a worktree (and are in it) before looking at file_exists(). It's unlikely that looking at ".gitmodules" in a bare repo would trigger in practice, but it does not hurt to be careful.

In a bare repo we'd rather want to check for an entry of .gitmodules in HEAD ?

I considered it a non issue, as I don't think many people push from bare repositories.

Here is another thought:
.gitmodules may not exist (either in working dir or in HEADs git tree), so maybe the
"correct" heuristic is to check for directories in $GIT_DIR/modules/ That is "more correct", because it is inconceivable to change the submodule pointers without having the submodules checked out. (Who would do that? Why?)

Actually, I like that a bit better. It would not cover the case where you have not actually checked out any of the submodules (or at least not called "submodule init", I guess?). But arguably that is a sign that the auto-recurse behavior should not be kicking in anyway.

Quoted 4 lines · Jeff King

Actually, I like that a bit better. It would not cover the case where you have not actually checked out any of the submodules (or at least not called "submodule init", I guess?). But arguably that is a sign that the auto-recurse behavior should not be kicking in anyway.

Yeah, the "no init, no recursion" line of thought is very sensible. I like it.

Quoted 9 lines · Junio C Hamano

Quoted 4 lines · Jeff King

Actually, I like that a bit better. It would not cover the case where you have not actually checked out any of the submodules (or at least not called "submodule init", I guess?). But arguably that is a sign that the auto-recurse behavior should not be kicking in anyway.

Yeah, the "no init, no recursion" line of thought is very sensible. I like it.

Bear in mind that "submodule init" only fuzzes around with .git/config. It doesn't touch .git/modules (i.e. cloning/fetching), that is to be done with the update command.

So if we also want to cover the case of init'd submodules, but not cloned/checked out submodules, we'd rather want to consult .git/config whether there is any submodule.* option set, though that seems more expensive than just checking for files inside the modules directory.

Consulting .git/config is fine, I think. It's not like we don't read it (sometimes multiple times!) during the normal course of the program anyway. It's just a question of whether it makes more sense for the heuristic to kick in after "init", or only after "update". I don't know enough to have an opinion.

Quoted 5 lines · Jeff

Consulting .git/config is fine, I think. It's not like we don't read it (sometimes multiple times!) during the normal course of the program anyway. It's just a question of whether it makes more sense for the heuristic to kick in after "init", or only after "update". I don't know enough to have an opinion.

I think there is no difference in practice, however the "after update" is way easier to implement and hence more maintainable (one lstat instead of fiddeling with the config; that can go wrong easily).

Quoted 7 lines · Stefan Beller

Quoted 5 lines · Jeff

Consulting .git/config is fine, I think. It's not like we don't read it (sometimes multiple times!) during the normal course of the program anyway. It's just a question of whether it makes more sense for the heuristic to kick in after "init", or only after "update". I don't know enough to have an opinion.

Quoted 3 lines

I think there is no difference in practice, however the "after update" is way easier to implement and hence more maintainable (one lstat instead of fiddeling with the config; that can go wrong easily).

Hmm, I would have thought it is the opposite; can't submodules exist in the working tree in their own ".git" directory? I know that's the "old" way of doing it, but I didn't know if it was totally deprecated.

Anyway, the config version is probably just:

  int config_check_submodule(const char *var, const char *value, void *data)
  {
        if (starts_with(var, "submodule.") && ends_with(var, ".path"))

                *(int *)data = 1;
        return 0;

}

  ...
  int have_submodule = 0;
  git_config(config_check_submodule, &have_submodule);

But I don't care too much either way. that's just for reference.

builtin/push.c static const char **refspec; · line 40

Quoted 10 lines

builtin/push.c +6 -0

@@ -31,6 +32,19 @@ static const char **refspec;
 static int refspec_nr;
 static int refspec_alloc;
 
+static void preset_submodule_default(void)
+{
+     struct strbuf sb = STRBUF_INIT;
+     strbuf_addf(&sb, "%s/modules", get_git_dir());
+
+     if (file_exists(sb.buf))

Maybe just:

if (file_exists(git_path("modules"))

?

-Peff

Jeff,
thanks for the suggestions, both git_path(..) as well as checking the config, this seems quite readable to me:

Quoted 3 lines · Stefan Beller

Jeff,
thanks for the suggestions, both git_path(..) as well as checking the config, this seems quite readable to me:

When reading the discussion I thought the same: What about the "old-style" repositories. I like this one. Checking both locations is nice.

Quoted 5 lines · Heiko Voigt

Quoted 3 lines · Stefan Beller

Jeff,
thanks for the suggestions, both git_path(..) as well as checking the config, this seems quite readable to me:

Quoted 3 lines

When reading the discussion I thought the same: What about the "old-style" repositories. I like this one. Checking both locations is nice.

BTW, since it seems we all agree on the direction. Should we add some tests?

Quoted 6 lines · Heiko Voigt

Quoted 5 lines · Heiko Voigt

Quoted 3 lines · Stefan Beller

Jeff,
thanks for the suggestions, both git_path(..) as well as checking the config, this seems quite readable to me:

Quoted 7 lines

Quoted 3 lines

When reading the discussion I thought the same: What about the "old-style" repositories. I like this one. Checking both locations is nice.

BTW, since it seems we all agree on the direction. Should we add some tests?

Good call. What do we want to test for?
* Correctness in case of submodules? (just push and get rejected) I think that is easy to do.
* Performance with [no, lots of] submodules? That seems harder to me.

I'll add a test for the correctness part and resend.

thanks for the suggestions, both git_path(..) as well as checking the config, this seems quite readable to me:

 builtin/push.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

Quoted 5 lines · Stefan Beller

thanks for the suggestions, both git_path(..) as well as checking the config, this seems quite readable to me:

 builtin/push.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

Yeah, this seems like a good compromise to me.

I did have one other thought, but I don't think it's worth pursuing now. We care about finding gitlinks in objects we're pushing; is there any process which is already looking at those objects? Genreally, the "pack-objects" doing the push has to. Not to actually push the objects, which often are sent blindly off disk, but to determine the set of reachable objects in the first place. So in theory we could prepare the list of objects to pack, and as a side effect it could say "and here are gitlinks referenced by those objects". But that doesn't work if bitmaps are in effect, because then we don't access the objects directly at all. I think you could solve that by extending the bitmap format to include a bit for gitlinks that are reachable (but not necessarily included in the pack).

So I don't think that's worth thinking too much about now, but it might be an interesting optimization down the road.

-Peff

builtin/push.c +3 -0

+}
+
 static void add_refspec(const char *ref)
 {
 	refspec_nr++;

@@ -552,6 +560,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 	};
 
 	packet_trace_identity("push");
+	preset_submodule_default();
 	git_config(git_push_config, &flags);
 	argc = parse_options(argc, argv, prefix, options, push_usage, 0);
 	set_push_cert_flags(&flags, push_cert);

builtin/push.c static const char **refspec; · line 36

Quoted 3 lines · Stefan Beller

builtin/push.c +3 -0

+static void preset_submodule_default(void)
+{
+	if (file_exists(".gitmodules"))

Don't we need to see if we are in a bare repository?

builtin/push.c static const char **refspec; · line 39

Quoted 3 lines

builtin/push.c +3 -0

+		recurse_submodules = RECURSE_SUBMODULES_CHECK;
+	else
+		recurse_submodules = RECURSE_SUBMODULES_DEFAULT;

Hmph, why "_DEFAULT" not "_OFF"?

builtin/push.c static const char **refspec; · line 36

Quoted 3 lines · Stefan Beller

builtin/push.c +3 -0

+static void preset_submodule_default(void)
+{
+     if (file_exists(".gitmodules"))

Don't we need to see if we are in a bare repository?

builtin/push.c static const char **refspec; · line 39

Quoted 4 lines

builtin/push.c +3 -0

+             recurse_submodules = RECURSE_SUBMODULES_CHECK;
+     else
+             recurse_submodules = RECURSE_SUBMODULES_DEFAULT;

Hmph, why "_DEFAULT" not "_OFF"?