sb/submodule-parallel-fetch, was: Re: What's cooking in git.git (Dec 2015, #03; Thu, 10)

[Johannes Sixt]

Am 11.12.2015 um 00:55 schrieb Stefan Beller:
> On Thu, Dec 10, 2015 at 3:51 PM, Junio C Hamano  wrote:
>> Stefan Beller  writes:
>>
 * sb/submodule-parallel-fetch (2015-11-24) 17 commits
 ...
>>>
>>> I assume you plan on merging this after 2.7 settled and then we can
>>> also get the above sb/submodule-parallel-update going again.
>>
>> Yeah, thanks for reminding me.  I think that would be a good plan
>> that gives us an opportunity to clean up this topic, some parts of
>> which are have "an early patch that was too hastily merged to 'next'
>> had to be tweaked by an 'oops' follow-up patch in the topic"
>> pattern, e.g. "make waitpid the secondary and closed pipe the
>> primary way to monitor children".
>>
>> Thanks.
> 
> This makes it sound as if you would drop it from next once 2.7 is out,
> expecting a complete reroll, which does the right thing from the beginning?
> That was not was I was expecting, but thanks for clarifying.

Also, rebuilding the topic such that it takes the direct route to its
current state would help bisectability on Windows.

Generally, I'm already quite satisfied with the state of the
infrastructure at the tip of the branch.

Nevertheless, I have a few niggles.

The primary one is that we are using a global variable of type
struct parallel_processes to keep track of the processes. Fortunately,
most functions already take a pointer (I gather you did anticipate an
object oriented use of the functions). The only exception is pp_init():
It returns a pointer to the global object, which is then passed around
to the other functions. This does not conform to our usual style,
however, where the initializer function takes a pointer to the object,
too.

After converting pp_init, we can have a nicely object oriented
collection of functions and get rid of the global object ... almost.

We still need a global variable for the signal handler. But since
signals and their handlers are a global resource, it is not that bad to
have a global variable that is dedicated to signal handling.

Another small nit is that I found it confusing that the closure
parameters are arranged differently in the callback functions. Granted,
in get_next_task one of them is an out parameter, but that is actually
an argument to move it to the end of the parameter list, IMHO.

On top of that I found an error or two in the documentation, and I have
a few suggestions for improvements.

All this is summarized in the patch below.

diff --git a/run-command.c b/run-command.c
index db4d916..f3addb9 100644
--- a/run-command.c
+++ b/run-command.c
@@ -864,7 +864,7 @@ enum child_state {
GIT_CP_WAIT_CLEANUP,
 };
 
-static struct parallel_processes {
+struct parallel_processes {
void *data;
 
int max_processes;
@@ -890,7 +890,7 @@ static struct parallel_processes {
 
int output_owner;
struct strbuf buffered_output; /* of finished children */
-} parallel_processes_struct;
+};
 
 static int default_start_failure(struct child_process *cp,
 struct strbuf *err,
@@ -933,23 +933,23 @@ static void kill_children(struct parallel_processes *pp, 
int signo)
kill(pp->children[i].process.pid, signo);
 }
 
+static struct parallel_processes *pp_for_signal;
+
 static void handle_children_on_signal(int signo)
 {
-   struct parallel_processes *pp = _processes_struct;
-
-   kill_children(pp, signo);
+   kill_children(pp_for_signal, signo);
sigchain_pop(signo);
raise(signo);
 }
 
-static struct parallel_processes *pp_init(int n,
- get_next_task_fn get_next_task,
- start_failure_fn start_failure,
- task_finished_fn task_finished,
- void *data)
+static void pp_init(struct parallel_processes *pp,
+   int n,
+   get_next_task_fn get_next_task,
+   start_failure_fn start_failure,
+   task_finished_fn task_finished,
+   void *data)
 {
int i;
-   struct parallel_processes *pp = _processes_struct;
 
if (n < 1)
n = online_cpus();
@@ -976,8 +976,9 @@ static struct parallel_processes *pp_init(int n,
pp->pfd[i].events = POLLIN | POLLHUP;
pp->pfd[i].fd = -1;
}
+
+   pp_for_signal = pp;
sigchain_push_common(handle_children_on_signal);
-   return pp;
 }
 
 static void pp_cleanup(struct parallel_processes *pp)
@@ -1019,10 +1020,10 @@ static int pp_start_one(struct parallel_processes *pp)
if (i == pp->max_processes)
die("BUG: bookkeeping is hard");
 
-   code = pp->get_next_task(>children[i].data,
->children[i].process,
+   code = pp->get_next_task(>children[i].process,
 

Re: sb/submodule-parallel-fetch, was: Re: What's cooking in git.git (Dec 2015, #03; Thu, 10)

[Stefan Beller]

On Fri, Dec 11, 2015 at 1:37 PM, Johannes Sixt  wrote:
>
> Generally, I'm already quite satisfied with the state of the
> infrastructure at the tip of the branch.
>

I was about the rework the patch series.

> Nevertheless, I have a few niggles.

If you don't mind I'll split your patch into chunks and
squash these where appropriate and resend the series
with the suggestions included without the intermediate stages
of non compiling code for Windows.

>
> The primary one is that we are using a global variable of type
> struct parallel_processes to keep track of the processes. Fortunately,
> most functions already take a pointer (I gather you did anticipate an
> object oriented use of the functions). The only exception is pp_init():
> It returns a pointer to the global object, which is then passed around
> to the other functions. This does not conform to our usual style,
> however, where the initializer function takes a pointer to the object,
> too.

Noted. I thought about the init function as a constructor,
such as

foo *pp = new foo();

in C++ would be just syntactic sugar for what I did there.
I'll adhere to Git style then instead.

>
> After converting pp_init, we can have a nicely object oriented
> collection of functions and get rid of the global object ... almost.
>
> We still need a global variable for the signal handler. But since
> signals and their handlers are a global resource, it is not that bad to
> have a global variable that is dedicated to signal handling.
>
> Another small nit is that I found it confusing that the closure
> parameters are arranged differently in the callback functions. Granted,
> in get_next_task one of them is an out parameter, but that is actually
> an argument to move it to the end of the parameter list, IMHO.
>
> On top of that I found an error or two in the documentation, and I have
> a few suggestions for improvements.
>
> All this is summarized in the patch below.

Thanks for the improvements!

Stefan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: sb/submodule-parallel-fetch, was: Re: What's cooking in git.git (Dec 2015, #03; Thu, 10)

[Johannes Sixt]

Am 11.12.2015 um 23:52 schrieb Stefan Beller:

If you don't mind I'll split your patch into chunks and
squash these where appropriate


I don't mind at all; please pick what you like.

-- Hannes

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: What's cooking in git.git (Dec 2015, #03; Thu, 10)

[Junio C Hamano]

Stefan Beller  writes:

> On Thu, Dec 10, 2015 at 3:51 PM, Junio C Hamano  wrote:
>> Stefan Beller  writes:
>>
 * sb/submodule-parallel-fetch (2015-11-24) 17 commits
 ...
>>>
>>> I assume you plan on merging this after 2.7 settled and then we can
>>> also get the above sb/submodule-parallel-update going again.
>>
>> Yeah, thanks for reminding me.  I think that would be a good plan
>> that gives us an opportunity to clean up this topic, some parts of
>> which are have "an early patch that was too hastily merged to 'next'
>> had to be tweaked by an 'oops' follow-up patch in the topic"
>> pattern, e.g. "make waitpid the secondary and closed pipe the
>> primary way to monitor children".
>>
>> Thanks.
>
> This makes it sound as if you would drop it from next once 2.7 is out,
> expecting a complete reroll, which does the right thing from the beginning?

Yes, what is still in 'next' when a new release is made has the
chance to (re)do the right thing from the beginning, and it also can
lose merges from other topics in the middle of the topic if they
have graduated to 'master'.

A topic that did the right thing from this cycle already, but needs
to stay in 'next' only because the area it touches is so important
and deserves more real world testing by those who run 'next', may
not have to reroll.

I think two sb/submodule-parallel-* topics fall into the former
category, i.e. ones that can take advantage of the chance to escape
the rigidity of 'next' at the release cycle boundary, and I think
we should grab that opportunity to clean these series up.

Thanks.



--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: What's cooking in git.git (Dec 2015, #03; Thu, 10)

[Junio C Hamano]

John Keeping  writes:

> On Thu, Dec 10, 2015 at 02:46:40PM -0800, Junio C Hamano wrote:
>> * jk/send-email-ssl-errors (2015-11-24) 1 commit
>>  - send-email: enable SSL level 1 debug output
>> 
>>  Improve error reporting when SMTP TLS fails.
>> 
>>  Waiting for a reroll.
>>  ($gmane/281693)
>
> It looks like this got lost in the noise:
>
> http://article.gmane.org/gmane.comp.version-control.git/281975

Indeed it was; thanks for a pointer.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: What's cooking in git.git (Dec 2015, #03; Thu, 10)

[Junio C Hamano]

Stefan Beller  writes:

>> * sb/submodule-parallel-fetch (2015-11-24) 17 commits
>> ...
>
> I assume you plan on merging this after 2.7 settled and then we can
> also get the above sb/submodule-parallel-update going again.

Yeah, thanks for reminding me.  I think that would be a good plan
that gives us an opportunity to clean up this topic, some parts of
which are have "an early patch that was too hastily merged to 'next'
had to be tweaked by an 'oops' follow-up patch in the topic"
pattern, e.g. "make waitpid the secondary and closed pipe the
primary way to monitor children".

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: What's cooking in git.git (Dec 2015, #03; Thu, 10)

[Stefan Beller]

On Thu, Dec 10, 2015 at 2:46 PM, Junio C Hamano  wrote:
>
> * sb/submodule-parallel-update (2015-11-20) 27 commits
...
>
>  Waiting for sb/submodule-parallel-fetch to stabilize.
>
>  It would be the cleanest to rebuild sb/submodule-parallel-fetch on
>  top of 2.7.0 once it ships and then build this directly on top;
>  that way, we do not have to have merges in this topic that
>  distracting (besides, some part of the other topic can be updated
>  in-place instead of this follow-up topic tweaking them as past
>  mistakes and inflexibilities).
>
>  I picked up v4 from the list, but it needs review.




>
> * sb/submodule-parallel-fetch (2015-11-24) 17 commits
>   (merged to 'next' on 2015-12-04 at 2c5ea47)
>  + run-command: detect finished children by closed pipe rather than waitpid
>   (merged to 'next' on 2015-11-20 at 89fc723)
>  + strbuf: update documentation for strbuf_read_once()
>  + run-command: remove set_nonblocking()
>   (merged to 'next' on 2015-10-23 at 8f04bbd)
>  + run-command: fix missing output from late callbacks
>  + test-run-command: increase test coverage
>  + test-run-command: test for gracefully aborting
>  + run-command: initialize the shutdown flag
>  + run-command: clear leftover state from child_process structure
>  + run-command: fix early shutdown
>   (merged to 'next' on 2015-10-15 at df63590)
>  + submodules: allow parallel fetching, add tests and documentation
>  + fetch_populated_submodules: use new parallel job processing
>  + run-command: add an asynchronous parallel child processor
>  + sigchain: add command to pop all common signals
>  + strbuf: add strbuf_read_once to read without blocking
>  + xread_nonblock: add functionality to read from fds without blocking
>  + xread: poll on non blocking fds
>  + submodule.c: write "Fetching submodule " to stderr
>  (this branch is tangled with sb/submodule-parallel-update.)
>
>  Add a framework to spawn a group of processes in parallel, and use
>  it to run "git fetch --recurse-submodules" in parallel.
>
>  Waiting for review.

What kind of review do you wait for?
Each patch had lots of discussion, both submodule people (Jens)
and Windows people (various) seem to be happy with this.

I assume you plan on merging this after 2.7 settled and then we can
also get the above sb/submodule-parallel-update going again.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: What's cooking in git.git (Dec 2015, #03; Thu, 10)

[John Keeping]

On Thu, Dec 10, 2015 at 02:46:40PM -0800, Junio C Hamano wrote:
> * jk/send-email-ssl-errors (2015-11-24) 1 commit
>  - send-email: enable SSL level 1 debug output
> 
>  Improve error reporting when SMTP TLS fails.
> 
>  Waiting for a reroll.
>  ($gmane/281693)

It looks like this got lost in the noise:

http://article.gmane.org/gmane.comp.version-control.git/281975
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: What's cooking in git.git (Dec 2015, #03; Thu, 10)

[Junio C Hamano]

Stefan Beller  writes:

>> * sb/submodule-parallel-fetch (2015-11-24) 17 commits
>>   (merged to 'next' on 2015-12-04 at 2c5ea47)
>>  + run-command: detect finished children by closed pipe rather than waitpid
>>...
>>  Waiting for review.
>
> What kind of review do you wait for?

This I think was inherited from previous issues.

The cleanest response that would move these topics forward is for
area experts to step up and say "This has been discussed and Acks
are given here (cf. $URL)" ;-)

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: What's cooking in git.git (Dec 2015, #03; Thu, 10)

[Stefan Beller]

On Thu, Dec 10, 2015 at 3:51 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>>> * sb/submodule-parallel-fetch (2015-11-24) 17 commits
>>> ...
>>
>> I assume you plan on merging this after 2.7 settled and then we can
>> also get the above sb/submodule-parallel-update going again.
>
> Yeah, thanks for reminding me.  I think that would be a good plan
> that gives us an opportunity to clean up this topic, some parts of
> which are have "an early patch that was too hastily merged to 'next'
> had to be tweaked by an 'oops' follow-up patch in the topic"
> pattern, e.g. "make waitpid the secondary and closed pipe the
> primary way to monitor children".
>
> Thanks.

This makes it sound as if you would drop it from next once 2.7 is out,
expecting a complete reroll, which does the right thing from the beginning?
That was not was I was expecting, but thanks for clarifying.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html