RFR 8219492: Restore static callsite resolution for the current class

[Claes Redestad]

Hi,

patch still looks good, thanks for moving this to a sub-task!

/Claes

On 2019-02-28 08:19, 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
>>>> -----