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

[coleen.phillimore at oracle.com]

On 7/9/19 5:06 PM, dean.long at oracle.com wrote:
> The updated comment sounds good.  Now that you have removed the only 
> place that was failing with retry_class_loading_during_parsing(), we 
> should be able to remove that method and its uses.  That gets rid of 
> the only way to "retry forever" vs the remaining and presumably safe 
> "down-grade and retry just once more".  Or you can file an RFE to 
> clean that up.

Thanks Dean.  I noticed that 
C2Compiler::retry_class_loading_during_parsing());

is now not used with my change but didn't want to clean it up with this 
change.  I'll file an RFE to clean it up (or find some other use for it 
in the compiler code).  What is the remaining "downgrade and retry just 
once more" option?

Thanks for the help!
Coleen

>
> dl
>
> On 7/8/19 2:19 PM, 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
>>>>
>>>
>>
>