patchEnable parallelism in git submodule update.

The --jobs parameter may be used to set the degree of per-submodule parallel execution.

Signed-off-by: Stefan Zager <szager@google.com>
---
 Documentation/git-submodule.txt |  8 +++++++-
 git-submodule.sh                | 23 ++++++++++++++++++++++-
 2 files changed, 29 insertions(+), 2 deletions(-)

Stefan Zager <szager@google.com> writes:

Capital-P option to xargs is not even in POSIX, no?

Hi Stefan,

neat patch. See below for a few notes.

On Fri, Jul 27, 2012 at 11:37:34AM -0700, Stefan Zager wrote:

I do not see orig_args set anywhere in submodule.sh. It seems the existing usage of it in cmd_status() is a leftover from commit 98dbe63 when this variable got renamed to orig_flags.

I will follow up with a patch to that location.

Another problem here is the passing of arguments. Have a look at a7eff1a8 to see how this was solved for other locations.

The next thing I noticed is that the parallelism is not recursive. You drop the option and only execute the first depth in parallel. How about using the amount of modules defined by arguments left in $@ as an indicator whether you need to fork parallel execution or not. If there is exactly one you do the update if there are more you do the parallel thing. That way you can just keep passing the --jobs flag to the subprocesses.

The next question to solve is UI: Since the output lines of the parallel update jobs will be mixed we need some way to distinguish them. Imagine one of the update fails somewhere how do we find out which it was?

Two possible solutions come to my mind:

 1. Prefix each line with a job number. This way you can distinguish
    which process outputted what and still have immediate feedback.

 2. Cache the output (to stderr and stdout) of each job and output it
    once one job is done. I imagine this needs some infrastructure which
    we need to implement. We already have some ideas how to collect such
    output in C here[1].

I would prefer solution 2 since the output of 1 will be hard to read but I guess we could start with 1 and then move over to 2 later on.

Cheers Heiko

[1] http://article.gmane.org/gmane.comp.version-control.git/197747

In commit 98dbe63 the variable $orig_args was renamed to $orig_flags. One location in cmd_status() was missed.

Note: This is a code cleanup and does not fix any bugs. As a side effect
the variables containing the parsed flags to "git submodule status" are
passed down recursively. So everything was already behaving as expected.

Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
---
 git-submodule.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Heiko Voigt <hvoigt@hvoigt.net> writes:

If that is the case, shouldn't we stop passing anything down, if we want it to be a "clean-up only, no behaviour changes" patch? While at it, we may want to kill that code to accumulate the original options in orig_flags because we haven't been using the variable.

We _know_ $orig_args has been empty, i.e. the code has been working fine with only cmd_status there. Nobody has tried what happens when we pass the original arguments to cmd_status on that line. The patch changes the behaviour of the code; it makes the command line parsing "while" loop to run again, and if the code that accumulates original options in orig_flags have been buggy, now that bug will be exposed.

Am 29.07.2012 08:22, schrieb Junio C Hamano:

I tried today. Before this change no arguments got passed down and afterwards they are (but just the arguments, no submodule paths were passed on in either case; which is what Kevin fixed in the commit Heiko referenced). Three arguments are allowed for "git submodule status":

--recursive: It doesn't matter if we pass that on or not because $recursive is reused when "eval cmd_status" is executed.

--quiet: Same as recursive, GIT_QUIET is set the first time and then reused in the recursion.

--cached: This was dropped when recursing into submodules but isn't anymore with Heiko's change, so we do have a change in behavior here.

Hmm, when --cached is used together with --recursive, I would expect it to show the commit stored in the index for the deeper submodules too (and not magically switch to show their HEAD again after the first level of submodules). To me this looks like a bug which Kevin accidentally introduced and nobody noticed and/or reported until now.

So I'd vote for making this a bugfix patch for "git submodule status --cached --recursive" (and would love to see a test for it ;-).

Jens Lehmann <Jens.Lehmann@web.de> writes:

Yeah, I am not opposed to a "fix". I just wanted it to be labelled as such, and analysed correctly.

And with test ;-)

Thanks.

Am 27.07.2012 20:37, schrieb Stefan Zager:

I think this is a sound idea, but it would be good to see some actual measurements. What are the performance numbers with and without this change? Which cases do benefit and are there some which run slower when run in parallel?

Am 29.07.2012 17:37, schrieb Jens Lehmann:

ping?

Stefan Zager <szager@google.com> writes:

About "policy", we use POSIX as a rough yardstick to warn us that we might be breaking people on minority platforms. We do _not_ say "It is in POSIX, so it is safe to use it", but we say "It is not even in POSIX, so we need to think twice." We do not usually say "Linux, Mac and Windows are the only things that matter, and they all support it."

Of course, any set of rules have exceptions ;-) There are a few things to which we say "Even though it is not in POSIX, everybody who matters supports it, and without taking advantage of it, what we want to achieve will become too cumbersome to express".

In the core parts of the system, we try to be very conservative. In the fringe where nobody cares about, we tend to be looser.

Hi,

On Fri, Jul 27, 2012 at 04:25:58PM -0700, Junio C Hamano wrote:

I was about to write that since this is limited to a given --jobs options the majority platforms should be enough as a start and others could add a parallelism mechanism later. Its only a matter of efficiency and not features.

But if you look at my other post to this thread I described that we need some UI output extension so the user can still make sense of it. In short: The user should be able distinguish which job said what.

I was already thinking about how an output caching could be implemented in core git. How about exposing it as a git command like this?

	git run [-j<number>] ...

It works like the xargs call above except that it caches each jobs output to stderr and stdout until its done and then replays the output to stderr/out in the correct order.

We could design the code so that it can be reused later on to do the caching in parallel fetch/push/... .

What do you think? If we decide to go this route I would have a look into whipping something up.

Cheers Heiko

Heiko Voigt <hvoigt@hvoigt.net> writes:

As long as "git submodule --jobs 9" on a platform without GNU enhanced xargs does not error out and gracefully degrade to non parallel execution, I do not have any problem with it. As posted, the patch has not yet achieved that doneness yet.