RFR (S) 8222446: assert(C->env()->system_dictionary_modification_counter_changed()) failed: Must invalidate if TypeFuncs differ

[coleen.phillimore at oracle.com]

Thanks, Erik!
Coleen

On 7/9/19 5:15 AM, Erik Österlund wrote:
> Hi Coleen,
>
> I like the counter removal. This looks good. Thanks for digging into 
> this and fixing it!
>
> /Erik
>
> On 2019-07-08 23:19, coleen.phillimore at oracle.com wrote:
>>
>> Hi,  From offline discussions, I updated the code in 
>> Parse::do_exits() to make the method not compilable if the return 
>> types don't match.  Otherwise it would revert a change that Volker 
>> made to prevent infinite compilation loops.  It seems that the 
>> compiler code has been changed to no longer exercise this path 
>> (ShouldNotReachHere never reached), so keeping the conservative path 
>> seemed safest.
>>
>> open webrev at 
>> http://cr.openjdk.java.net/~coleenp/2019/8222446.02/webrev
>>
>> I changed the comment Dean, it might need help rewording.
>>
>> Tested with tier1-8.
>>
>> Thanks,
>> Coleen
>>
>> On 6/21/19 4:44 PM, coleen.phillimore at oracle.com wrote:
>>>
>>> Dean,  Thank you for reviewing and for your help and discussion of 
>>> this change.
>>>
>>> On 6/21/19 3:48 PM, dean.long at oracle.com wrote:
>>>> For the most part, this looks good.  I only have a couple concerns:
>>>>
>>>> 1) The distinction in both validate_compile_task_dependencies 
>>>> functions between "dependencies failed" and "dependencies invalid" 
>>>> is even more fuzzy after this change.  I suggest filing an RFE to 
>>>> remove this distinction.
>>>
>>> Yes, in jvmciRuntime I had to carefully preserve this logic or some 
>>> tests failed.   I'll file an RFE for you.
>>>>
>>>> 2) In Parse::do_exits(), we don't know that concurrent class 
>>>> loading didn't cause the problem.  We should be optimistic and 
>>>> allow the retry:
>>>> C->record_failure(C2Compiler::retry_class_loading_during_parsing());
>>>> rather than more drastic
>>>>     C->record_method_not_compilable
>>>> This is actually what the code did in an earlier revision.
>>>
>>> Erik and I were trying to guess which was the right answer. It 
>>> seemed too lucky that you'd do concurrent class loading in this time 
>>> period, so we picked the more drastic answer, but I tested both.  So 
>>> I'll change it to the optimistic answer.
>>>
>>> Thanks!
>>> Coleen
>>>>
>>>> dl
>>>>
>>>> On 6/20/19 10:28 AM, coleen.phillimore at oracle.com wrote:
>>>>> Summary: Remove SystemDictionary::modification_counter optimization
>>>>>
>>>>> See bug for more details.  To avoid the assert in the bug report, 
>>>>> it's necessary to also increase the modification counter for class 
>>>>> unloading, which needs special code for concurrent class 
>>>>> unloading. The global counter is used to verify that 
>>>>> validate_dependencies() gets the same answer based on the subklass 
>>>>> hierarchy, but provides a quick exit in production mode.  Removing 
>>>>> it may allow more nmethods to be created that don't depend on the 
>>>>> classes that may be loaded while the Method is being compiled. 
>>>>> Performance testing was done on this with no change in 
>>>>> performance. Also investigated the breakpoint setting code which 
>>>>> incremented the modification counter. Dependent compilations are 
>>>>> invalidated using evol_method dependencies, so updating the system 
>>>>> dictionary modification counter isn't unnecessary.
>>>>>
>>>>> Tested with hs-tier1-8 testing, and CTW, and local jvmti/jdi/jdwp 
>>>>> test runs with -Xcomp.
>>>>>
>>>>> open webrev at 
>>>>> http://cr.openjdk.java.net/~coleenp/2019/8222446.01/webrev
>>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8222446
>>>>>
>>>>> Thanks,
>>>>> Coleen
>>>>
>>>
>>
>