Re: [PATCH 7/6] Enable threaded async procedures whenever pthreads is available

[Johannes Sixt <j6t@kdbg.org>]

On Mittwoch, 10. März 2010, Junio C Hamano wrote:
> Johannes Sixt <j6t@kdbg.org> writes:
> > On Samstag, 6. März 2010, Shawn O. Pearce wrote:
> >> I'm in favor of that.  If we have threaded delta search enabled,
> >> we probably can also run these async procedures in a POSIX thread
> >> rather than forking off a child.
> >
> > OK. The patch could look like this.
>
> Will queue in 'pu', but as Shawn said, we should probably give another
> closer look at the callees that are started with this interface before
> moving forward.

So here is my analysis.

There are currently these users of the async infrastructure:

builtin-fetch-pack.c
builtin-receive-pack.c
builtin-send-pack.c
convert.c
remote-curl.c
upload_pack.c

The list below shows all functions that read or write potentially global state 
that are called from the async procedure (except for upload_pack.c, see 
below). I ignored all functions that do not depend on global state, like 
strcmp, memcpy, sprintf, strerror, etc.

----------
remote-curl.c:write_discovery():
  write
  close

These two functions only communicate with the parent. This is absolutely 
thread-safe.

----------
builtin-fetch-pack.c:sideband_demux() and
builtin-send-pack.c:sideband_demux() are virtually identical, and
builtin-receive-pack.c:copy_to_sideband() has the same footprint:
  getenv
  read
  die_errno
  die
  write
  close

Again, the functions only communicate with the parent and nothing else. getenv 
could be a problem if the parent called setenv, but it doesn't. die (and 
die_errno) are overridden in the threaded version, and are uncritical. In 
total, this user is thread-safe.

(This is getenv("TERM") from recv_sideband, btw.)

----------
convert.c:filter_buffer()
  pipe
  close
  getenv
  fork
  read
  write
  fprintf
  waitpid

if GIT_TRACE is set:
  xrealloc
  die
  free

This one is less trivial. It calls start_command+finish_command from the async 
procedure. The parent calls
  read()
  xrealloc() (via strbuf_read())
  error()
(and nothing else) between start_async() and finish_async().

As long as GIT_TRACE is not set, we are safe, because the async procedure 
carefully releases all global resources that it allocated, and the parent is 
looking in the other direction while it does os. Even if GIT_TRACE is set, 
xrealloc() in the parent and the async procedure cannot be called 
simultanously because the procedure calls it only before it begins writing 
output, and the parent only after it received this output.

On Windows, the situation is slightly different, because we always call into 
xmalloc() during start_command, but again we do so before the output is 
produced.

I think we are safe in all cases.

----------
upload_pack:create_pack_file(): This user is different because it calls into 
the revision walker in the async procedure, which definitely affects global 
state. Therefore, here is the list of functions called by the parent until it 
exits:
  pipe
  close
  getenv
  fork
  read
  write
  fprintf
  waitpid
  alarm
  die
  die_errno
  pthread_join / waitpid

if GIT_TRACE is set:
  xrealloc
  die
  free

It also calls start_command+finish_command. If GIT_TRACE is set, we are less 
safe because this time, xrealloc() can be called simultanously with pack 
functions while the revision walker is operating.

But as long as GIT_TRACE is not set, it looks like we are safe, though I 
haven't looked through the revision walker whether it messes with alarms or 
the environment.

One aspect is, though, that the async procedure is used *only* for a shallow 
clone, and the reason that it is used is that pack-objects (or the way it is 
called from upload-pack?) has not been taught to produce a pack for a shallow 
clone.

This concludes my analysis.

-- Hannes