RFR 8219492: Restore static callsite resolution for the current class

[Lois Foltan]

Looks good.
Lois

On 2/28/2019 2:19 AM, David Holmes 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/
>
> Thanks,
> David
>
> 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
>>>> -----