harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Salikh Zakirov <Salikh.Zaki...@Intel.com>
Subject Re: [drlvm][threading] comments on Harmony-3288
Date Mon, 05 Mar 2007 11:11:30 GMT
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


Mime
View raw message