[drlvm][threading] H3288 -- the mods look great. Is it ready to commit?

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

All,
This is a very nice clean patch.  This definitely helps maintainability and
probably even stability.  I quickly looked at all 10 patches.  build, build
test passes on win32 and lin32 rhel4.0.  I can't test lin64 yet.  Does
anyone have objection if I commit this patch in the next 15 hours?

-- 
Weldon Washburn
Intel Enterprise Solutions Software Division

Re: [drlvm][threading] H3288 -- the mods look great. Is it ready to commit?

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

Nathan Beyer wrote:
> This is the set of patches for replacing APR thread constructs with
> PThread constructs, correct?

Pthreads on Linux, Win32 on Windows.

> I don't have any objections to doing this, but I did have a question
> about the removal of the all APR thread constructs. The patch doesn't
> seem to remove all APR thread references, but indicates that this is
> being completed, what's the status on that?

The purpose of the patch is not to eliminate APR completely.
Rather, I wanted to be able to allocated and deallocated mutexes, conditions
and thread blocks freely and explicitly, which was not possible using APR
interfaces due to APR memory pools.

H-3288 makes it possible. The ultimate goal (H-3289 -- work in progress) is to
make use of possibility to deallocate thread blocks to fix the leak associated
with the thread blocks.

> I think that this patch simplifies things, but I want to make sure we
> also get the full benefit of not having to manage the APR memory pools
> and eliminate the seemingly unused overhead.

Actually, before the 3288 patch, each thread in DRLVM created as much as 4 (!)
distinct memory pools, that is
1) apr_thread_t 2) hythread_t 3) VM_thread 4) JVMTI_Thread,
and the hythread_t pool was never deallocated (in fact, it could not be
deallocated at all due to allocation of fat monitors, which can live much
longer than the thread).

After 3288, it reduces the number of pools per thread to two latter,
which I think are deallocated on thread termination.


Weldon asked:
> Re: [drlvm][threading] H3288 Is it ready to commit?

I've still has not pinpointed the root cause of occasional segfault on
gc.SynchronizedFinilazersTest.
Probably we should not commit regressing changes.

Re: [drlvm][threading] H3288 -- the mods look great. Is it ready to commit?

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

On 3/16/07, Alexey Varlamov <al...@gmail.com> wrote:
>
> 2007/3/16, Weldon Washburn <we...@gmail.com>:
> > On 3/15/07, Nathan Beyer <nd...@apache.org> wrote:
> > >
> > > This is the set of patches for replacing APR thread constructs with
> > > PThread constructs, correct?
> > >
> > > I don't have any objections to doing this, but I did have a question
> > > about the removal of the all APR thread constructs. The patch doesn't
> > > seem to remove all APR thread references, but indicates that this is
> > > being completed, what's the status on that?
> > >
> > > I think that this patch simplifies things, but I want to make sure we
> > > also get the full benefit of not having to manage the APR memory pools
> > > and eliminate the seemingly unused overhead.
> >
> >
> > Good point.  I did a grep for "apr_" in trunk/working_vm and found over
> 1600
> > hits.  And this is after applying H3288.  Clearly there is much more
> work to
> > cleaning up the apr problem.
>
> Weldon,
>
> AFAIU we never declared such goal - to drop using APR completely? If I
> correctly read Nathan's question, it is about APR threading only. With
> more selective grepping, I found the following list of APR functions
> used (52 items), not sure we really need/want to re-implement them
> all:
>
> apr_allocator_alloc(
> apr_allocator_create(
> apr_allocator_destroy(
> apr_allocator_free(
> apr_atomic_add32(
> apr_atomic_cas32(
> apr_atomic_casptr(
> apr_atomic_dec32(
> apr_atomic_inc32(
> apr_atomic_xchg32(
> apr_dir_close(
> apr_dir_open(
> apr_dir_read(
> apr_dso_error(
> apr_dso_load(
> apr_dso_sym(
> apr_dso_unload(
> apr_env_get(
> apr_file_close(
> apr_file_flush(
> apr_file_gets(
> apr_file_open(
> apr_filepath_get(
> apr_file_printf(
> apr_file_read(
> apr_file_write(
> apr_gethostname(
> apr_get_os_error(
> apr_hash_count(
> apr_hash_first(
> apr_hash_get(
> apr_hash_make(
> apr_hash_next(
> apr_hash_set(
> apr_hash_this(
> apr_initialize(
> apr_isdigit(
> apr_itoa(
> apr_palloc(
> apr_pcalloc(
> apr_pool_clear(
> apr_pool_create(
> apr_pool_destroy(
> apr_psprintf(
> apr_pstrcat(
> apr_pstrdup(
> apr_snprintf(
> apr_stat(
> apr_strerror(
> apr_temp_dir_get(
> apr_time_now(
> apr_vsnprintf(
>
> They are partially covered by Harmony's own portlib, but anyway this
> would be huge refactoring - do we have solid justification?


You are correct.  Given that there is no specific major bug on the
above, there is no solid justification at this time.

Some observations:  From the above list we are still using memory pool
functions like apr_palloc.  If something like the JIT or GC uses apr_palloc,
then later wants to free the memory we may have problems like we have in the
threading subsystem.  Do the apr_atomic_xxx_64 functions exist?  A quick
google of apr_atomic_inc64 shows nothing.  Also, from a stability standpoint
it would be good if someone looked at all the implementations of all the
above APIs on all platforms to see if there are any critical section
issues.  The reason is that JVM's by nature stress threading/sync in ways
that no other workload does.


>
> > -Nathan
> > >
> > > On 3/15/07, Weldon Washburn <we...@gmail.com> wrote:
> > > > All,
> > > > This is a very nice clean patch.  This definitely helps
> maintainability
> > > and
> > > > probably even stability.  I quickly looked at all 10
> patches.  build,
> > > build
> > > > test passes on win32 and lin32 rhel4.0.  I can't test lin64
> yet.  Does
> > > > anyone have objection if I commit this patch in the next 15 hours?
> > > >
> > > > --
> > > > Weldon Washburn
> > > > Intel Enterprise Solutions Software Division
> > > >
> > >
> >
> >
> >
> > --
> > Weldon Washburn
> > Intel Enterprise Solutions Software Division
> >
>



-- 
Weldon Washburn
Intel Enterprise Solutions Software Division

Re: [drlvm][threading] H3288 -- the mods look great. Is it ready to commit?

[Alexey Varlamov <al...@gmail.com>]

2007/3/16, Weldon Washburn <we...@gmail.com>:
> On 3/15/07, Nathan Beyer <nd...@apache.org> wrote:
> >
> > This is the set of patches for replacing APR thread constructs with
> > PThread constructs, correct?
> >
> > I don't have any objections to doing this, but I did have a question
> > about the removal of the all APR thread constructs. The patch doesn't
> > seem to remove all APR thread references, but indicates that this is
> > being completed, what's the status on that?
> >
> > I think that this patch simplifies things, but I want to make sure we
> > also get the full benefit of not having to manage the APR memory pools
> > and eliminate the seemingly unused overhead.
>
>
> Good point.  I did a grep for "apr_" in trunk/working_vm and found over 1600
> hits.  And this is after applying H3288.  Clearly there is much more work to
> cleaning up the apr problem.

Weldon,

AFAIU we never declared such goal - to drop using APR completely? If I
correctly read Nathan's question, it is about APR threading only. With
more selective grepping, I found the following list of APR functions
used (52 items), not sure we really need/want to re-implement them
all:

apr_allocator_alloc(
apr_allocator_create(
apr_allocator_destroy(
apr_allocator_free(
apr_atomic_add32(
apr_atomic_cas32(
apr_atomic_casptr(
apr_atomic_dec32(
apr_atomic_inc32(
apr_atomic_xchg32(
apr_dir_close(
apr_dir_open(
apr_dir_read(
apr_dso_error(
apr_dso_load(
apr_dso_sym(
apr_dso_unload(
apr_env_get(
apr_file_close(
apr_file_flush(
apr_file_gets(
apr_file_open(
apr_filepath_get(
apr_file_printf(
apr_file_read(
apr_file_write(
apr_gethostname(
apr_get_os_error(
apr_hash_count(
apr_hash_first(
apr_hash_get(
apr_hash_make(
apr_hash_next(
apr_hash_set(
apr_hash_this(
apr_initialize(
apr_isdigit(
apr_itoa(
apr_palloc(
apr_pcalloc(
apr_pool_clear(
apr_pool_create(
apr_pool_destroy(
apr_psprintf(
apr_pstrcat(
apr_pstrdup(
apr_snprintf(
apr_stat(
apr_strerror(
apr_temp_dir_get(
apr_time_now(
apr_vsnprintf(

They are partially covered by Harmony's own portlib, but anyway this
would be huge refactoring - do we have solid justification?

>
> -Nathan
> >
> > On 3/15/07, Weldon Washburn <we...@gmail.com> wrote:
> > > All,
> > > This is a very nice clean patch.  This definitely helps maintainability
> > and
> > > probably even stability.  I quickly looked at all 10 patches.  build,
> > build
> > > test passes on win32 and lin32 rhel4.0.  I can't test lin64 yet.  Does
> > > anyone have objection if I commit this patch in the next 15 hours?
> > >
> > > --
> > > Weldon Washburn
> > > Intel Enterprise Solutions Software Division
> > >
> >
>
>
>
> --
> Weldon Washburn
> Intel Enterprise Solutions Software Division
>

Re: [drlvm][threading] H3288 -- the mods look great. Is it ready to commit?

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

On 3/15/07, Nathan Beyer <nd...@apache.org> wrote:
>
> This is the set of patches for replacing APR thread constructs with
> PThread constructs, correct?
>
> I don't have any objections to doing this, but I did have a question
> about the removal of the all APR thread constructs. The patch doesn't
> seem to remove all APR thread references, but indicates that this is
> being completed, what's the status on that?
>
> I think that this patch simplifies things, but I want to make sure we
> also get the full benefit of not having to manage the APR memory pools
> and eliminate the seemingly unused overhead.


Good point.  I did a grep for "apr_" in trunk/working_vm and found over 1600
hits.  And this is after applying H3288.  Clearly there is much more work to
cleaning up the apr problem.

-Nathan
>
> On 3/15/07, Weldon Washburn <we...@gmail.com> wrote:
> > All,
> > This is a very nice clean patch.  This definitely helps maintainability
> and
> > probably even stability.  I quickly looked at all 10 patches.  build,
> build
> > test passes on win32 and lin32 rhel4.0.  I can't test lin64 yet.  Does
> > anyone have objection if I commit this patch in the next 15 hours?
> >
> > --
> > Weldon Washburn
> > Intel Enterprise Solutions Software Division
> >
>



-- 
Weldon Washburn
Intel Enterprise Solutions Software Division

Re: [drlvm][threading] H3288 -- the mods look great. Is it ready to commit?

[Nathan Beyer <nd...@apache.org>]

This is the set of patches for replacing APR thread constructs with
PThread constructs, correct?

I don't have any objections to doing this, but I did have a question
about the removal of the all APR thread constructs. The patch doesn't
seem to remove all APR thread references, but indicates that this is
being completed, what's the status on that?

I think that this patch simplifies things, but I want to make sure we
also get the full benefit of not having to manage the APR memory pools
and eliminate the seemingly unused overhead.

-Nathan

On 3/15/07, Weldon Washburn <we...@gmail.com> wrote:
> All,
> This is a very nice clean patch.  This definitely helps maintainability and
> probably even stability.  I quickly looked at all 10 patches.  build, build
> test passes on win32 and lin32 rhel4.0.  I can't test lin64 yet.  Does
> anyone have objection if I commit this patch in the next 15 hours?
>
> --
> Weldon Washburn
> Intel Enterprise Solutions Software Division
>