RFR 8219492: Restore static callsite resolution for the current class (was: RFR: 8219233: 3x performance drop for some Clojure applications

[David Holmes]

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
>>> -----