review (M) for 6965184: possible races in make_not_entrant_or_zombie
Tom Rodriguez
tom.rodriguez at oracle.com
Tue Jul 6 14:22:15 PDT 2010
Thanks!
tom
On Jul 6, 2010, at 2:06 PM, Vladimir Kozlov wrote:
> 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