review (M) for 6965184: possible races in make_not_entrant_or_zombie

Vladimir Kozlov vladimir.kozlov at oracle.com
Tue Jul 6 14:06:42 PDT 2010


Then, I think, it is good to push.

Thanks,
Vladimir

Tom Rodriguez wrote:
> No just those lines testing invocations for 0 changed to 1.  Everything else stayed the same.
> 
> tom
> 
> On Jul 6, 2010, at 12:55 PM, Vladimir Kozlov wrote:
> 
>> Looks good.
>>
>> Did you change anything else I need to review in addition to _invocations at lines 161 and 178 in sweeper.cpp?
>>
>> Thanks,
>> Vladimir
>>
>> Tom Rodriguez wrote:
>>> I've correct those uses of _invocations and regenerated the webrev.  I'm rerunning my tests.
>>> tom
>>> On Jul 2, 2010, at 5:18 PM, Vladimir Kozlov wrote:
>>>> Tom,
>>>>
>>>> Nice VTune cleanup :)
>>>>
>>>> Thank you for consolidating sweep logging code.
>>>>
>>>> In nmethod.cpp why you don't use the_method() in your new code (// Remove nmethod from method) ?
>>>>
>>>> In sweeper.cpp why you need second (_invocations > 0) check? Only current thread will modify _invocations now after your changes.
>>>>
>>>> Why you replaced next code:
>>>>
>>>> !   // We want to visit all nmethods after NmethodSweepFraction invocations.
>>>> !   // If invocation is 1 we do the rest
>>>> !   int todo = CodeCache::nof_blobs();
>>>> !   if (_invocations > 1) {
>>>> !     todo = (CodeCache::nof_blobs() - _seen) / _invocations;
>>>> !   }
>>>> ...
>>>> !     for(int i = 0; i < todo && _current != NULL; i++) {
>>>>
>>>> whith this:
>>>>
>>>> !   int todo = (CodeCache::nof_nmethods() - _seen) / _invocations;
>>>> ...
>>>> !     // The last invocation iterates to the end of the code cache
>>>> !     for (int i = 0; (i < todo || _invocations == 0) && _current != NULL; i++) {
>>>>
>>>> First, _invocations can't be 0 according to the code in possibly_sweep()
>>>> Second, if it was 0 then you will get division by 0 in todo expression.
>>>>
>>>> Next assert also looks strange:
>>>> +   assert(_invocations > 0 || _current == NULL, "must have scanned the whole cache");
>>>>
>>>> Vladimir
>>>>
>>>> Tom Rodriguez wrote:
>>>>> http://cr.openjdk.java.net/~never/6965184
>>>>> 6965184: possible races in make_not_entrant_or_zombie
>>>>> Reviewed-by:
>>>>> While investigating the sweeper concurrency problems I identified a
>>>>> bunch of minor issues that could lead to races.  NMethod groups a
>>>>> bunch of flags into a struct using bitfield which was okay in the past
>>>>> because they were either written by the constructor, at a safepoint or
>>>>> with a single lock held.  Now some of those fields are written under
>>>>> one lock and some are written under another so they should be
>>>>> separated.  I've done away with nmFlags all together and moved a bunch
>>>>> of common initialization into a new method.  I also deleted a other of
>>>>> unused or dead things.
>>>>> In make_not_entrant_or_zombie, all the zombie logic used to be
>>>>> performed at a safepoint but now it can happen concurrently so the
>>>>> logic that is executed outside of the Patching_lock has to be more
>>>>> careful.  All the not_entrant logic has been moved into the region
>>>>> protected by the Patching_lock.  The link between the nmethod and the
>>>>> methodOop is also broken ealier though a strong reference to the
>>>>> methodOop is maintained to make sure it stays alive.
>>>>> There were problems with the maintenance of _sweep_started and
>>>>> _invocations that could mistakenly allow another thread into
>>>>> possibly_sweep but only if there was no work to be done.  Also threads
>>>>> besides the sweeping thread could clear _sweep_started.
>>>>> The sweeper divides the cache based on the number of blobs even though
>>>>> it only works on nmethod, so I added a nof_nmethods count along with a
>>>>> nof_adapters count just for informational purposes.
>>>>> As a cleanup I also deleted the dead and unused VTune logic since it
>>>>> doesn't work and VTune uses JVMTI agents these days.
>>>>> Tested with NSK jvmti,jdi,jdb,hprof,jit,regression and JDI_REGRESSION
>>>>> tests.
> 


More information about the hotspot-compiler-dev mailing list