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

[coleen.phillimore at oracle.com]

On 1/23/19 9:31 PM, David Holmes wrote:
> Hi Coleen,
>
> On 24/01/2019 12: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.
>
> That sounds quite reasonable. Reviewed.
>
> Though perhaps the bug synopsis should be updated to reflect the 
> change in direction before pushing.

Done, thanks!
Coleen
>
> Thanks,
> David
> -----
>
>> 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
>>>>
>>>
>>