harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Aleksey Ignatenko" <aleksey.ignate...@gmail.com>
Subject Re: Remove VM statistics or make it thread safe?
Date Thu, 11 Jan 2007 11:13:57 GMT
What I see in the code beyond is that there is no crash possibility and it
could lead only to fault stat values. But it could be important if it adds
additional noise of measuring to Thread Checker tool then it should be fixed
to move next and find really importent hidden race conditions.

Aleksey.


On 11 Jan 2007 17:06:37 +0600, Egor Pasko <egor.pasko@gmail.com> wrote:
>
> On the 0x25B day of Apache Harmony Ilia A. Leviev wrote:
> > Hi,
> > Recently I have started to use Thread Checker tool for finding unsafe
> > thread access places in VM.
> > First I have tried to check VM that run simple application such HWA.
> > The tool shows that there is thread unsafe access to some of
> > VM_Statistics fields that result in race condition.
>
> JIT profilers and Jitrino.OPT instruction profiler are thread-unsafe
> too. But this is a *feature*. We do not need precise data here at a
> cost of extra synchronisations.
>
> Same arguments may be applied to vm stats...
>
> > \vm\vmcore\include\ vm_stats.h
> >
> > class VM_Statistics
> > {
> > public:
> >
> >    uint64 num_free_local_called;
> >    uint64 num_jni_handles_wasted_refs;
> >    uint64 num_free_local_called_free;
> >    uint64 num_local_jni_handles;
> >    uint64 num_jni_handles_freed;
> >
> >       ~~~~~~~~~~~~~~~~~~~~~
> >
> > The problem occurred during execution of free_local_object_handles2
> > function
> > from \vm\vmcore\src\object\object_handles.cpp.
> > The function increment fields of type uint64.
> >
> > ////////////////////////////////////////////////////////
> > void free_local_object_handles2(ObjectHandles* head)
> > {
> >     ObjectHandlesNew* h = (ObjectHandlesNew*)head;
> >     assert(h);
> > #ifdef VM_STATS
> >     VM_Statistics::get_vm_stats().num_free_local_called++;// race
> > condition
> >     if(h->next != NULL)
> >         VM_Statistics::get_vm_stats().num_free_local_called_free++;//
> > race condition
> > #endif //VM_STATS
> >     while(h->next) {
> > #ifdef VM_STATS
> >         unsigned size = h->size;
> >         VM_Statistics::get_vm_stats().num_local_jni_handles += size; //
> > race condition
> >         VM_Statistics::get_vm_stats().num_jni_handles_freed++; // race
> > condition
> >         VM_Statistics::get_vm_stats().num_jni_handles_wasted_refs +=
> > (h->capacity - size);// race condition
> > #endif //VM_STATS
> >         ObjectHandlesNew* next = h->next;
> >         STD_FREE(h);
> >         h = next;
> >     }
> > #ifdef VM_STATS
> >     VM_Statistics::get_vm_stats().num_jni_handles_wasted_refs +=
> > (h->capacity - h->size); // race condition
> > #endif //VM_STATS
> > }
> > ////////////////////////////////////////////////////////
> >
> > There are several solutions:
> > 1)   remove and do not use statistics at all, if anybody not needs it.
> > 2)   make this function synchronized using lock-unlock functions from
> > lock_manager,
> > 3)   make increment by atomic APR function such apr_atomic_inc32(
> > volatile apr_uint32_t *  mem ),
> >      but to use it we need to change the type of the fields from uint64
> > to volatile uint32.
> >
> > What solution will be more acceptable?
> >
> >
> > Best regards,
> > Leviev Ilya
> > Intel Java & XML Engineering
> >
>
> --
> Egor Pasko
>
>

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