RFR(S) 8058564: Tiered compilation performance drop in PIT

[Jiangli Zhou]

Hi Igor,

How about changing set_method_counters() instead of adding a new function?

  bool set_method_counters(MethodCounters* counters) {
     if (counters == NULL) {
       // The store into method must be released. On platforms without
       // total store order (TSO) the reference may become visible before
       // the initialization of data otherwise.
       OrderAccess::release_store_ptr((volatile void *)&_method_counters, NULL);
       return true;
     } else {
       bool res = Atomic::cmpxchg_ptr(counters, (volatile void*)&_method_counters, NULL) == NULL;
       if (res) {
         OrderAccess::release(); // is release need after cmpxchg?
       }
       return res;
     }
  }

I'm not very sure if we need the release aftercmpxchg. Please check with David Holmes.

Thanks,
Jiangli

On 09/17/2014 04:09 PM, Igor Veresov wrote:
> Ok, here the webrev that takes care of the leak: 
> http://cr.openjdk.java.net/~iveresov/8058564/webrev.02/ 
> <http://cr.openjdk.java.net/%7Eiveresov/8058564/webrev.02/>
>
> Thanks,
> igor
>
>
> On Sep 17, 2014, at 2:36 PM, Igor Veresov <igor.veresov at oracle.com 
> <mailto:igor.veresov at oracle.com>> wrote:
>
>>
>> On Sep 17, 2014, at 2:26 PM, Jiangli Zhou <jiangli.zhou at oracle.com 
>> <mailto:jiangli.zhou at oracle.com>> wrote:
>>
>>> Hi Igor,
>>>
>>> On 09/17/2014 10:53 AM, Igor Veresov wrote:
>>>> On Sep 17, 2014, at 8:32 AM, David Chase <david.r.chase at oracle.com 
>>>> <mailto:david.r.chase at oracle.com>> wrote:
>>>>
>>>>> On 2014-09-17, at 6:01 AM, Igor Veresov <igor.veresov at oracle.com 
>>>>> <mailto:igor.veresov at oracle.com>> wrote:
>>>>>
>>>>>> Alright, how about a shorter 
>>>>>> fix:http://cr.openjdk.java.net/~iveresov/8058564/webrev.01/ 
>>>>>> <http://cr.openjdk.java.net/%7Eiveresov/8058564/webrev.01/>
>>>>>>
>>>>>> igor
>>>>> Does that need to be protected by a lock?
>>>>> Other than that, it looked good to me (i.e., I plugged your patch 
>>>>> into netbeans and browsed around
>>>>> and it looked like it would do what you say it does — but it also 
>>>>> looked like it might need a lock).
>>>>>
>>>> Hm, it’s a good question. There is certainly a semi-benign race 
>>>> when creating method counters. It might be on purpose, since having 
>>>> a lock there may have a pretty big impact on the interpreter during 
>>>> startup. On the other hand, metaspace allocation has a lock. May be 
>>>> having two locks is too much?
>>>>
>>>> Jiangli, if you remember, why do method counters have a racy 
>>>> allocation?
>>>
>>> I measured the possibility of the memory leak, it was rare in my 
>>> experiments. So I ended up not using a lock to protect the allocation.
>>>
>>> Vladimir Ivanov was looking at fixing the possible memory leak a few 
>>> month back. He was proposing using a CAS based solution to update 
>>> the method counters. Using a lock here might potentially cause a 
>>> deadlock.
>>>
>>
>> Yup, CAS seems reasonable. I’ll make the change.
>>
>> igor
>>
>>> Thanks,
>>> Jiangli
>>>
>>>>
>>>> igor
>>>>
>>>>
>>>>> David
>>>>>
>>>>>> On Sep 17, 2014, at 2:35 AM, Igor Veresov 
>>>>>> <igor.veresov at oracle.com <mailto:igor.veresov at oracle.com>> wrote:
>>>>>>
>>>>>>> Sorry, my fix is not entirely good. MethodCounters should exist 
>>>>>>> before a method ends up in compile queue. I’ll get back with the 
>>>>>>> updated webrev.
>>>>>>>
>>>>>>> igor
>>>>>>>
>>>>>>> On Sep 17, 2014, at 2:01 AM, Igor Veresov 
>>>>>>> <igor.veresov at oracle.com <mailto:igor.veresov at oracle.com>> wrote:
>>>>>>>
>>>>>>>> We don’t always have MDOs. Level 1 & 2 are good examples. C2 
>>>>>>>> also doesn’t always require an MDO.
>>>>>>>> I also wanted it to work with other compilers, like Graal. By 
>>>>>>>> putting this logic in the policy it’s in one place and I don’t 
>>>>>>>> need to touch compilers. I could’ve put it in the broker, but 
>>>>>>>> it seemed that these level values are artifacts of the policy 
>>>>>>>> so it seems reasonable to put it in the policy.
>>>>>>>>
>>>>>>>> igor
>>>>>>>>
>>>>>>>> On Sep 17, 2014, at 12:52 AM, Vladimir Kozlov 
>>>>>>>> <vladimir.kozlov at oracle.com 
>>>>>>>> <mailto:vladimir.kozlov at oracle.com>> wrote:
>>>>>>>>
>>>>>>>>> Why not create MethodCounters in 
>>>>>>>>> Method::build_interpreter_method_data()? It is called at the 
>>>>>>>>> beginning of compilation (C1 and C2) from 
>>>>>>>>> ciMethod::ensure_method_data(). And not necessary that way. My 
>>>>>>>>> point is - why not crate them at the beginning of a 
>>>>>>>>> compilation as we do with MDO? Compiled code may need to 
>>>>>>>>> access it. May be not now but in a future.
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Vladimir
>>>>>>>>>
>>>>>>>>> On 9/17/14 12:39 AM, Igor Veresov wrote:
>>>>>>>>>> The problem here is that with -Xcomp we immediately compile a 
>>>>>>>>>> method at level 3, and we’re not creating MethodCounters 
>>>>>>>>>> since we never execute in the interpreter and hence not 
>>>>>>>>>> setting the “highest” level values. The solution is to 
>>>>>>>>>> allocate MethodCounters for every method compiled (unless it 
>>>>>>>>>> has been allocated naturally by the interpreter). I made it 
>>>>>>>>>> in a form of a callback to the policy, since only tiered 
>>>>>>>>>> policies cares about these values.
>>>>>>>>>>
>>>>>>>>>> JBS:https://bugs.openjdk.java.net/browse/JDK-8058564
>>>>>>>>>> Webrev:http://cr.openjdk.java.net/~iveresov/8058564/webrev.00 
>>>>>>>>>> <http://cr.openjdk.java.net/%7Eiveresov/8058564/webrev.00>
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>> igor
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20140917/f618b058/attachment-0001.html>