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

[dean.long at oracle.com]

On 7/9/19 2:16 PM, coleen.phillimore at oracle.com wrote:
>
>
> 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?
>

The remaining are retry_no_subsuming_loads(), 
retry_no_escape_analysis(), and has_boxed_value() here:

https://java.se.oracle.com/source/xref/jdk-jdk/open/src/hotspot/share/opto/c2compiler.cpp#112

Notice that they all set some kind of flag to disable the current 
failure, preventing infinite loops.

dl

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