RFR 8219492: Restore static callsite resolution for the current class

[David Holmes]

Thanks Vladimir!

David

On 1/03/2019 4:01 am, Vladimir Ivanov wrote:
> 
>> I switched the bug ids round so that this small fix is the subtask - 
>> 8219492. Please find the webrev at:
>>
>> http://cr.openjdk.java.net/~dholmes/8219492/webrev/
> 
> Looks good.
> 
> Best regards,
> Vladimir Ivanov
> 
>> On 27/02/2019 9:37 pm, David Holmes wrote:
>>> Hi Claes,
>>>
>>> On 27/02/2019 5:56 pm, Claes Redestad wrote:
>>>> Hi David,
>>>>
>>>> On 2019-02-27 05:17, David Holmes wrote:
>>>>> webrev: http://cr.openjdk.java.net/~dholmes/8219233/webrev.v2/
>>>>
>>>> patch looks good to me.
>>>
>>> Thanks for looking at this.
>>>
>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8219233
>>>>
>>>> As this is only a partial fix (about a 10% improvement in the clojure
>>>> startup repro), I'd suggest moving it to a subtask and leaving
>>>> JDK-8219233 open for the time being - if only to not give the false
>>>> impression the issue is resolved in its entirety.
>>>
>>> Yes I can do that. Though I may use the existing sub-task for this 
>>> part and let the main bug become the C1/C2 issue that Vladimir is 
>>> working on - the discussion is all in the comments of the main bug 
>>> anyway.
>>>
>>> Thanks,
>>> David
>>>
>>>> I've verified this recuperates performance completely in cases where
>>>> we're calling into static methods from the class itself, e.g., 
>>>> "Bad"[1],
>>>> and also that it doesn't help in cases where call are routed back via
>>>> another class, e.g., "AlsoBad".
>>>>
>>>> Thanks!
>>>>
>>>> /Claes
>>>>
>>>> [1] https://cl4es.github.io/2019/02/21/Cljinit-Woes.html
>>>>
>>>>>
>>>>> We had a bug whereby the callsite to a static method was resolved 
>>>>> as soon as it was executed, allowing other threads to execute the 
>>>>> same code and skip the class initialization check. The fix to that 
>>>>> was to not do the resolution if the target class was not fully 
>>>>> initialized. This has a consequence that calls to static methods of 
>>>>> the current class from the <clinit> (including transitively) can't 
>>>>> be resolved and effectively compiled and so we can take a huge 
>>>>> performance hit for those cases (which is a rare case and regular 
>>>>> benchmarking showed no issue). Clojure unfortunately uses a style 
>>>>> of coding that is significantly impacted by this.
>>>>>
>>>>> There is a simple fix for the direct case of calling a static 
>>>>> method from <clinit>: when we are dealing with the same class then 
>>>>> it's okay to do the resolution as the current thread must have 
>>>>> <clinit> on its callstack. No other thread can execute the code 
>>>>> being resolved until the initial thread completes the static 
>>>>> initialization.
>>>>>
>>>>> Unfortunately that doesn't address the main pattern used by 
>>>>> Clojure, but we have a subtask open where Vladimir Ivanov continues 
>>>>> to look into that case. And Claes Redestad has also given coding 
>>>>> advice on how to avoid the problem in the first place.
>>>>>
>>>>> Testing: tier 1-3
>>>>> Microbenchmarking - see bug report.
>>>>>
>>>>> Thanks,
>>>>> David
>>>>> -----