[drlvm][threading] comments on Harmony-3288

[Weldon Washburn <we...@gmail.com>]

This JIRA does not specifically state the high-level motivation.  It would
be a good idea to discuss the motivation on the dev list so that Harmony
priorities are clearly set.  I throw the following observations onto the
list to get the conversation going.

I think a valid motivation for H3288 is basic threading subsystem
stability.  I agree with the observation that Apache Portable Runtime (APR)
adds much confusion and probably even bugs to the threading model.   It
would be good to reduce the amount of APR code where possible.  Even if this
means cluttering the native C code with "#ifdef LINUX.... #elseif
WINDOWS...#endif".

Bottom line:  I think Harmony-3288 is attacking the correct problem.  It
looks like it is going the right direction.  I have not looked at 3288 real
close thus I don't know yet if 3288 is ready for commit.

-- 
Weldon Washburn
Intel Enterprise Solutions Software Division

Re: [drlvm][threading] comments on Harmony-3288

[Salikh Zakirov <Sa...@Intel.com>]

HARMONY-3288 "Use Pthreads/Win32 rather than APR for mutexes and condition
variables" is about refactoring threading code to avoid using APR mutexes and
condition variables.

Weldon Washburn wrote:
> This JIRA does not specifically state the high-level motivation. 

The high-level motivation for avoiding APR mutexes and condition variables is
to streamline the thread and monitor memory management, making it easy to make
sure memory is freed appropriately.

APR interface for mutex and condition variable is tightly coupled with pool
memory model, making it necessary to have a APR memory pool. At the same time,
APR does not provide any memory release operations on the pool, besides
apr_pool_destroy() function.

DRLVM has a major leak associated with fat monitors, because they are never
destroyed. (* fat monitor is a native synchronization object created for java
objects which has been either waited on or contended on monitor enter *).
Fat monitor is implemented as a pair of mutex and condition variable, thus it
requires an APR pool to allocate them.

To be able to release monitor-associated memory, we will have to have a new
pool allocated for each fat monitor. This comprises major memory overhead, as
each APR pool will allocate 8k of virtual memory, and use 4k of physical memory
(because the amount allocated for the monitor will be less than one memory page).

IMHO, this memory overhead is quite high, and deserves fixing.

The proposed patch makes use of synchronization APIs provided by the operating
system (either Pthreads or Win32). Since both Pthreads and Win32 provide a
fully defined structure for mutex (pthread_mutex_t, CRITICAL_SECTION), it is
possible to avoid dynamic allocation for mutex at all.

Condition variables are provided by Pthreads in the same way (pthread_cond_t),
but are missing in Win32. Thus, we have to provide our own implemenation for
condition variables on Win32. However, this is no regression from APR, because
the condition variables provided by APR are so broken that DRLVM never could
actually use them. So, we now have a APR patch, which completely replaces APR
condition variable implementation with a custom one. While this solution worked
rather well, it certainly increases number of layers and confusion.

The suggested patch refactors condition variable implementation for Win32 from
being a patch for APR to a stand-alone code, included directly as HyCond
implementation, thus reducing number of layers by one. IMHO, this contributes
to more understandable and debuggable code.

> I think a valid motivation for H3288 is basic threading subsystem
> stability.  I agree with the observation that Apache Portable Runtime (APR)
> adds much confusion and probably even bugs to the threading model.   It
> would be good to reduce the amount of APR code where possible.  Even if
> this
> means cluttering the native C code with "#ifdef LINUX.... #elseif
> WINDOWS...#endif".

Yes, I agree that making threading code clearer will eventually lead to
increased system stability. Besides, the change is needed to fix a more
pressing reliability problem: memory leaks associated with fat monitors and
threads.

Concerning the #ifdefs, the number of platform-specific code hasn't changed.
It just moved out of APR sources to DRLVM sources. In total, #ifdef'ed code
amounts to about ~50 lines, and another ~100 lines for condition variable
implementation, so it is not too much from the maintenance perspective.

BTW, I used HAS_PTHREAD and _WIN32 macros, where HAS_PTHREAD is defined if
__linux__ macro is defined. _WIN32 and __linux__ macros are provided by the
compilers and do not require any specific makefile flags.

> Bottom line:  I think Harmony-3288 is attacking the correct problem.  It
> looks like it is going the right direction.  I have not looked at 3288 real
> close thus I don't know yet if 3288 is ready for commit.

I've tested the attached patches with 'build test', and had smoke and kernel
tests passed. CUnit tests fail on a linking stage with some obscure APR linking
problem; I will look into it.

Besides, when kernel tests were running, I've got a bunch of 'incorrect memory
access' windows on shutdown, which did not affect test results. This is likely
to be caused by the monitor, latch and semaphore explicit deallocation, which
was not done before, and hints that some bugs are present on the part of users
of HyLatch, HySemaphore or HyThreadMonitor.

The third thing that probably is worth doing before committing is completing
the change to avoid using APR functions in thread creation too. This could also
help in fixing APR linking problem, as it may help to avoid linking hythread to
APR at all.

-- 
Salikh Zakirov

Re: [drlvm][threading] comments on Harmony-3288

[Salikh Zakirov <Sa...@Intel.com>]

Weldon Washburn wrote:
>  I would caution
> that we
> should be careful of the performance impact of replacing #ifdefs with
> call/returns.  My guess is that this will not slow down things.  In any
> case, we need to measure.

I would say that we *decrease* the depths of call chain by 1 by eliminating APR
function layer.

We could probably go further and replace functions with macros,
but there is an issue with callers expecting status constants TM_ERROR_TIMEOUT,
TM_ERROR_EBUSY etc. Probably, we could define them using system constants. This
deserves some further thinking.

Re: [drlvm][threading] comments on Harmony-3288

[Weldon Washburn <we...@gmail.com>]

On 3/5/07, Geir Magnusson Jr. <ge...@pobox.com> wrote:
>
>
> On Mar 5, 2007, at 6:14 AM, Peter Novodvorsky wrote:
>
> > I fully agree with Weldon, I only want to notice that better
> > implementation would be not #ifdef'ing platform dependent lines, but
> > implementing hymutex/hyconds with platform dependent synchronization
> > primitives as separate files for windows and linux and then using them
> > for implementing threads synchronization. This way we will recreate
> > parts of APR, but it's not a problem, because we have different
> > focuses (APR authors focus on Apache HTTPD in the end).
>
> I think I agree with all of you.  I was fairly skeptical about APR
> from the very beginning, noting that we have a set of requirements
> that's different than the original customer for APR.
>
> Glad to see this moving forward this way.


Agreed.  This is finally going the right direction.  I would caution that we
should be careful of the performance impact of replacing #ifdefs with
call/returns.  My guess is that this will not slow down things.  In any
case, we need to measure.

geir
>
> >
> > Peter.
> >
> > On 3/5/07, Weldon Washburn <we...@gmail.com> wrote:
> >> This JIRA does not specifically state the high-level motivation.
> >> It would
> >> be a good idea to discuss the motivation on the dev list so that
> >> Harmony
> >> priorities are clearly set.  I throw the following observations
> >> onto the
> >> list to get the conversation going.
> >>
> >> I think a valid motivation for H3288 is basic threading subsystem
> >> stability.  I agree with the observation that Apache Portable
> >> Runtime (APR)
> >> adds much confusion and probably even bugs to the threading
> >> model.   It
> >> would be good to reduce the amount of APR code where possible.
> >> Even if this
> >> means cluttering the native C code with "#ifdef LINUX.... #elseif
> >> WINDOWS...#endif".
> >>
> >> Bottom line:  I think Harmony-3288 is attacking the correct
> >> problem.  It
> >> looks like it is going the right direction.  I have not looked at
> >> 3288 real
> >> close thus I don't know yet if 3288 is ready for commit.
> >>
> >> --
> >> Weldon Washburn
> >> Intel Enterprise Solutions Software Division
> >>
>
>


-- 
Weldon Washburn
Intel Enterprise Solutions Software Division

Re: [drlvm][threading] comments on Harmony-3288

[Xiao-Feng Li <xi...@gmail.com>]

Agree. This is a good move. I had the same feeling during GCv5
development, so GCv5 was designed not to use APR at all for its system
related operations, but use a wrapper to work around. Looks like it is
a good design decision.

Thanks,
xiaofeng


On 3/5/07, Geir Magnusson Jr. <ge...@pobox.com> wrote:
>
> On Mar 5, 2007, at 6:14 AM, Peter Novodvorsky wrote:
>
> > I fully agree with Weldon, I only want to notice that better
> > implementation would be not #ifdef'ing platform dependent lines, but
> > implementing hymutex/hyconds with platform dependent synchronization
> > primitives as separate files for windows and linux and then using them
> > for implementing threads synchronization. This way we will recreate
> > parts of APR, but it's not a problem, because we have different
> > focuses (APR authors focus on Apache HTTPD in the end).
>
> I think I agree with all of you.  I was fairly skeptical about APR
> from the very beginning, noting that we have a set of requirements
> that's different than the original customer for APR.
>
> Glad to see this moving forward this way.
>
> geir
>
> >
> > Peter.
> >
> > On 3/5/07, Weldon Washburn <we...@gmail.com> wrote:
> >> This JIRA does not specifically state the high-level motivation.
> >> It would
> >> be a good idea to discuss the motivation on the dev list so that
> >> Harmony
> >> priorities are clearly set.  I throw the following observations
> >> onto the
> >> list to get the conversation going.
> >>
> >> I think a valid motivation for H3288 is basic threading subsystem
> >> stability.  I agree with the observation that Apache Portable
> >> Runtime (APR)
> >> adds much confusion and probably even bugs to the threading
> >> model.   It
> >> would be good to reduce the amount of APR code where possible.
> >> Even if this
> >> means cluttering the native C code with "#ifdef LINUX.... #elseif
> >> WINDOWS...#endif".
> >>
> >> Bottom line:  I think Harmony-3288 is attacking the correct
> >> problem.  It
> >> looks like it is going the right direction.  I have not looked at
> >> 3288 real
> >> close thus I don't know yet if 3288 is ready for commit.
> >>
> >> --
> >> Weldon Washburn
> >> Intel Enterprise Solutions Software Division
> >>
>
>

Re: [drlvm][threading] comments on Harmony-3288

["Geir Magnusson Jr." <ge...@pobox.com>]

On Mar 5, 2007, at 6:14 AM, Peter Novodvorsky wrote:

> I fully agree with Weldon, I only want to notice that better
> implementation would be not #ifdef'ing platform dependent lines, but
> implementing hymutex/hyconds with platform dependent synchronization
> primitives as separate files for windows and linux and then using them
> for implementing threads synchronization. This way we will recreate
> parts of APR, but it's not a problem, because we have different
> focuses (APR authors focus on Apache HTTPD in the end).

I think I agree with all of you.  I was fairly skeptical about APR  
from the very beginning, noting that we have a set of requirements  
that's different than the original customer for APR.

Glad to see this moving forward this way.

geir

>
> Peter.
>
> On 3/5/07, Weldon Washburn <we...@gmail.com> wrote:
>> This JIRA does not specifically state the high-level motivation.   
>> It would
>> be a good idea to discuss the motivation on the dev list so that  
>> Harmony
>> priorities are clearly set.  I throw the following observations  
>> onto the
>> list to get the conversation going.
>>
>> I think a valid motivation for H3288 is basic threading subsystem
>> stability.  I agree with the observation that Apache Portable  
>> Runtime (APR)
>> adds much confusion and probably even bugs to the threading  
>> model.   It
>> would be good to reduce the amount of APR code where possible.   
>> Even if this
>> means cluttering the native C code with "#ifdef LINUX.... #elseif
>> WINDOWS...#endif".
>>
>> Bottom line:  I think Harmony-3288 is attacking the correct  
>> problem.  It
>> looks like it is going the right direction.  I have not looked at  
>> 3288 real
>> close thus I don't know yet if 3288 is ready for commit.
>>
>> --
>> Weldon Washburn
>> Intel Enterprise Solutions Software Division
>>

Re: [drlvm][threading] comments on Harmony-3288

[Peter Novodvorsky <pe...@gmail.com>]

I fully agree with Weldon, I only want to notice that better
implementation would be not #ifdef'ing platform dependent lines, but
implementing hymutex/hyconds with platform dependent synchronization
primitives as separate files for windows and linux and then using them
for implementing threads synchronization. This way we will recreate
parts of APR, but it's not a problem, because we have different
focuses (APR authors focus on Apache HTTPD in the end).

Peter.

On 3/5/07, Weldon Washburn <we...@gmail.com> wrote:
> This JIRA does not specifically state the high-level motivation.  It would
> be a good idea to discuss the motivation on the dev list so that Harmony
> priorities are clearly set.  I throw the following observations onto the
> list to get the conversation going.
>
> I think a valid motivation for H3288 is basic threading subsystem
> stability.  I agree with the observation that Apache Portable Runtime (APR)
> adds much confusion and probably even bugs to the threading model.   It
> would be good to reduce the amount of APR code where possible.  Even if this
> means cluttering the native C code with "#ifdef LINUX.... #elseif
> WINDOWS...#endif".
>
> Bottom line:  I think Harmony-3288 is attacking the correct problem.  It
> looks like it is going the right direction.  I have not looked at 3288 real
> close thus I don't know yet if 3288 is ready for commit.
>
> --
> Weldon Washburn
> Intel Enterprise Solutions Software Division
>