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:40 PM, dean.long at oracle.com wrote:
> 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.

I see.  Thanks for the code pointer.  I'll add this to the RFE.

thanks,
Coleen
>
> 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
>>>>>>
>>>>>
>>>>
>>>
>>
>