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

[David Holmes]

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.

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
>>>
>>
>