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

From: Johannes Sixt <j6t@kdbg.org>
Date: Mar 17, 2010, 21:28 UTC
Message-ID: <201003172228.18939.j6t@kdbg.org>
In-Reply-To: <7v7hpjq0aw.fsf@alter.siamese.dyndns.org>

On Mittwoch, 10. März 2010, Junio C Hamano wrote:

Show 11 quoted lines

> 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