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

[dean.long at oracle.com]

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.

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