Re: [xml] [patch] make libxml2 slightly more thread-friendly

[Daniel Veillard <veillard redhat com>]

On Mon, Feb 12, 2007 at 10:18:03AM -0600, Ted Phelps wrote:
> Hi Daniel,
>
> As always, thank you for your speedy reply.
>
> Daniel Veillard writes:
> > Okay, fixing the problem sounds fine of course, but I have a couple of 
> > issues with the patch:
> >
> >    - all thread implementation specific code in libxml2 is contained in
> >      threads.c module, that patch breal that containment, I would really
> >      prefer if this code was kept in threads.c
>
> I thought you might.  I initially chose not to because I didn't want to
> add new functions to the shared object, but I'm happy to do so if you
> are.  Or do you have some mechanism to prevent functions from being
> exported to the aplication?

  see libxml.h in same directory as the C sources, prefix the function names
by __ so even if it shows up in the shared object people are not tempted to
call them and dont export from include/libxml/*.h
 
> >    - it uses the keyword 'volatile' which is not used anywhere else in 
> >      libxml2, I would prefer to avoid it if possible to maximize portability
> >      (plus its semantic is IMHO rather fuzzy)
>
> I included the volatile keyword for the Windows code because the
> declaration of InterlockedCompareExchangePointer indicates that the
> first parameter should be of type 'PVOID volatile *'.  I believe that we
> can safely leave this out and I have removed it from the patch below.
>
> For the BeOS implementation, I have changed 'volatile' to vint32,
> following the definition of atomic_add:
>
>     http://www.beunited.org/bebook/The%20Support%20Kit/Functions.html
>
> You may want to consider changing run_once_init to be a vint32 too.

  Hum, if you know you're in a specific environment where volatile is expected
then go ahead, just avoid it for the platform agnostic code :-)

> > I'm not sure how to best achieve those improvements, as this code
> > really mix the xmlInitParser with really thread-specific code.
>
> I have shifted the thread-related code into threads.c, leaving only
> calls to lock and unlock a global mutex.

  Looks good except the __ renaming and include file.

> > Also we had trouble in the past with the xmlInitGlobals()/
> > xmlInitThreads()/xmlInitMemory() ordering of initializers, the
> > interraction are relatively complex to debug I hope the new code
> > doesn't have side effect, that seems true as it uses direct memory
> > allocations, it's just something to keep in mind.
>
> I believe this patch should be immmune to such interactions.

  yes I believe so too now :-)

> No worries.  Here's a second attempt.  Please have a look and let me
> know if you'd like me to try a different approach or make additional
> changes.

  Would you be so kind to make the last changes for the function name
and include, then I guess that should be commited,

  thanks !

Daniel

-- 
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard      | virtualization library  http://libvirt.org/
veillard redhat com  | libxml GNOME XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine  http://rpmfind.net/