RFR (S) 8216136: Don't take Compile_lock for SystemDictionary::_modification_counter

[coleen.phillimore at oracle.com]

Thanks, Erik!
Coleen

On 1/24/19 9:13 AM, Erik Österlund wrote:
> Hi Coleen,
>
> Looks good. Ship it!
>
> Thanks,
> /Erik
>
> On 2019-01-24 14:01, coleen.phillimore at oracle.com wrote:
>> Thanks Dean and thank you for the consulation on this RFE.
>> Coleen
>>
>> On 1/24/19 12:17 AM, dean.long at oracle.com wrote:
>>> Looks good.
>>>
>>> dl
>>>
>>> On 1/23/19 6:20 AM, coleen.phillimore at oracle.com wrote:
>>>>
>>>> After some internal discussion, Dean convinced me that removing the 
>>>> Compile_lock here might be too dangerous.   So for these asserts 
>>>> and the error condition, the compiler thread goes to VM from native 
>>>> to check the SystemDictionary::modification_counter under the 
>>>> Compile_lock, with safepoint checking always.
>>>>
>>>> Tested with tier1,2,6 and 8.
>>>>
>>>> open webrev at http://cr.openjdk.java.net/~coleenp/8216136.02/webrev
>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8216136
>>>>
>>>> Thanks,
>>>> Coleen
>>>>
>>>> On 1/17/19 7:15 AM, coleen.phillimore at oracle.com wrote:
>>>>>
>>>>>
>>>>> On 1/16/19 10:53 PM, dean.long at oracle.com wrote:
>>>>>> Hi Coleen.  You still can't safely call notice_modification() 
>>>>>> outside of Compile_lock, (at least not without other changes), so 
>>>>>> this:
>>>>>>
>>>>>> - static inline void notice_modification() { 
>>>>>> assert_locked_or_safepoint(Compile_lock); 
>>>>>> ++_number_of_modifications; }
>>>>>> + static inline void notice_modification() { 
>>>>>> Atomic::inc(&_number_of_modifications); }
>>>>>>
>>>>>> should be:
>>>>>>
>>>>>> static inline void notice_modification() { 
>>>>>> assert_locked_or_safepoint(Compile_lock); 
>>>>>> Atomic::inc(&_number_of_modifications); }
>>>>>>
>>>>>>
>>>>>> Are you trying to eventually remove Compile_lock completely? If 
>>>>>> so, then notice_modification() would have to be called *before* the
>>>>>> class hierarchy is changed, not after, and probably other changes 
>>>>>> would be needed as well.
>>>>> Dean, Thank you for looking at this and your comments.
>>>>>
>>>>> No, I'm not trying to remove Compile_lock entirely and I can 
>>>>> assert that notice_modification has the Compile_lock as above. The 
>>>>> class hierarchy code has been changed to be lock free rather than 
>>>>> requiring the Compile_lock, although I think the Compile_lock 
>>>>> still protects some of this code.
>>>>>
>>>>> There are also some Compile_lock free ways of getting to 
>>>>> dependencies, because putting notice_modification after 
>>>>> flush_dependencies caused bugs that I'll ask to you offline about.
>>>>>
>>>>> Thanks for your help.  I was just trying to peel off one place 
>>>>> where Compile_lock seemed wrong.
>>>>>
>>>>> Thanks,
>>>>> Coleen
>>>>>>
>>>>>> dl
>>>>>>
>>>>>>
>>>>>> On 1/16/19 8:43 AM, coleen.phillimore at oracle.com wrote:
>>>>>>> Summary: make SystemDictionary::modification_counter atomic so 
>>>>>>> not to require Compile_lock.
>>>>>>>
>>>>>>> I moved updating the modification counter when the class is 
>>>>>>> defined and added to the hierarchy.  I didn't remove the 
>>>>>>> Compile_lock completely because there may be other code 
>>>>>>> currently under the lock that needs it (flush_dependencies). Can 
>>>>>>> someone from the compiler area also review this?
>>>>>>>
>>>>>>> Made Compile_lock an always safepointing lock.
>>>>>>>
>>>>>>> Tested with mach5 tier1-6.
>>>>>>>
>>>>>>> open webrev at 
>>>>>>> http://cr.openjdk.java.net/~coleenp/8216136.01/webrev
>>>>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8216136
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Coleen
>>>>>>
>>>>>
>>>>
>>>
>>
>