RFR: 8219233: 3x performance drop for some Clojure applications

[David Holmes]

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