[13] RFR (S): 8188133: C2: Static field accesses in clinit can trigger deoptimizations

[Vladimir Kozlov]

On 2/4/19 10:42 AM, Vladimir Ivanov wrote:
> 
> 
> On 01/02/2019 10:32, Vladimir Kozlov wrote:
>> I am more concern about C2 hidden assumptions about classes it compiles and static fields accesses order than about 
>> Java correctness of executing code in static initializers.
>>
>> On other hand we are at the beginning of JDK 13 and can do this experiment to see how it goes. Okay but please test it 
>> may be up-to tier7 before push.
>>
>> Changes itself seem fine.
> 
> Thanks, Vladimir. Tier1-7 testing finished successully. Will push the patch today.

Thank you for testing more.

Vladimir K

> 
> Best regards,
> Vladimir Ivanov
> 
>> On 2/1/19 10:13 AM, Vladimir Ivanov wrote:
>>>
>>>> So this is the case for OSR compilation of <clinit> with hot loop (iterations > 10000). Should we care about such 
>>>> cases?
>>>> We want people to write simple <clinit> so we can serialize results - this case is opposite direction.
>>>
>>> It's definitely a corner case and having long-running static initializers is discouraged, but I don't agree that it 
>>> should be ignored by JITs.
>>>
>>> The faster <clinit> is finished the better for startup & warmup: as you noted, <clinit> serializes execution and may 
>>> end up blocking the rest of the program. So, there are clear incentives to optimize <clinit>, though it may not be 
>>> the most important case to care about.
>>>
>>>> I understand that JITed code should perform better than Interpreter. And we should fix regression in jdk 9.
>>>
>>> Regression is handled separately (JDK-8192070) and it is not specific to static initializers. With this enhancement, 
>>> I'm interested solely in how JITs handle <clinit>.
>>>
>>>> There are several conditions for inlining. What if a called method is not inlined even with your relaxed condition? 
>>>> We will still deoptimize it on each iteration. Right?
>>>
>>> Yes, proposed patch doesn't solve the general case. It just extends existing heuristics. If iniling fails, callee has 
>>> to deoptimize.
>>>
>>>> I am fine to allow access to static fields from <clinit> code but I am not sure about allowing to inline general 
>>>> methods when class is not fully initialized. Can we limit what methods should be inlined? Leaf methods? Small code? 
>>>> No loops?
>>>
>>> We definitely can, but I don't see compelling reasons to do so. It's perfectly fine to run any code in the context of 
>>> static initializer. The only limitation is - only the thread which runs clinit is allowed to do so. I believe having 
>>> clinit as the root of compilation is a strong guarantee of that invariant.
>>>
>>>> I would prefer to not compile such <clinit> at all (if it has hot calls).
>>>
>>> It would have negative consequences on startup and warmup.
>>> In this particular case, the problem is not in clinit itself, but the code it calls. Methods accessing static members 
>>> have the very same problem when compiled into a separate nmethod.
>>>
>>> Best regards,
>>> Vladimir Ivanov
>>>
>>>> On 1/31/19 11:29 AM, Vladimir Ivanov wrote:
>>>>> http://cr.openjdk.java.net/~vlivanov/8188133/webrev.00
>>>>> https://bugs.openjdk.java.net/browse/JDK-8188133
>>>>>
>>>>> The test case in the bug demonstrates a pathological case with long-running static initializer: though it's allowed 
>>>>> to access static fields from the thread performing the initialization, C2 can't prove that in general.
>>>>>
>>>>> While solving the general problem doesn't seem worth the effort (requires barriers on method entries), I propose to 
>>>>> extend the logic to cover simple cases: when static initializer is the root of the compilation, all accesses to 
>>>>> static fields of the corresponding class are allowed. It extends the coverage to all inlinees from OSR compilation 
>>>>> of clinit.
>>>>>
>>>>> Testing: hs-precheckin-comp, tier1-5
>>>>>
>>>>> Best regards,
>>>>> Vladimir Ivanov