harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Weldon Washburn" <weldon...@gmail.com>
Subject Re: [drlvm][threading] H3288 -- the mods look great. Is it ready to commit?
Date Fri, 16 Mar 2007 13:37:49 GMT
On 3/16/07, Alexey Varlamov <alexey.v.varlamov@gmail.com> wrote:
>
> 2007/3/16, Weldon Washburn <weldonwjw@gmail.com>:
> > On 3/15/07, Nathan Beyer <ndbeyer@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 <weldonwjw@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

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message