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

[Vladimir Ivanov]

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.

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