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

[Igor Veresov]

Thanks, Jiangli!

igor

On Sep 17, 2014, at 6:47 PM, Jiangli Zhou <jiangli.zhou at oracle.com> wrote:

> Hi Igor,
> 
> Looks good. Thank you for fixing the potential memory leak! It took me forever to get to it.
> 
> Thanks,
> Jiangli
> 
> On 09/17/2014 06:14 PM, Igor Veresov wrote:
>> Very well, more restrictive API is always better. I used the “clear_” prefix, that seems to be the convention..
>> 
>> http://cr.openjdk.java.net/~iveresov/8058564/webrev.05/
>> 
>> igor
>>  On Sep 17, 2014, at 5:48 PM, Vladimir Kozlov <vladimir.kozlov at oracle.com> wrote:
>> 
>>> On 9/17/14 5:30 PM, Igor Veresov wrote:
>>>> Well, I guess it’ll constant-fold, but I’d rather have different actions
>>>> be distinct.
>>>> You definitely don’t need a barrier after a CAS, it’s a two-way barrier
>>>> by itself.
>>>> Judging from the contexts from which set_method_counters() is called, I
>>>> don’t think the release barrier there is necessary, I’ll remove it.
>>> I agree. set_method_counters() is now used only for cleaning (store NULL). I think we can rename it to clean_method_counters() to use only for that purpose (and don't pass parameter).
>>> 
>>> Vladimir
>>> 
>>>> Webrev: http://cr.openjdk.java.net/~iveresov/8058564/webrev.04/
>>>> 
>>>> igor
>>>> 
>>>> On Sep 17, 2014, at 5:04 PM, Jiangli Zhou <jiangli.zhou at oracle.com
>>>> <mailto:jiangli.zhou at oracle.com>> wrote:
>>>> 
>>>>> 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
>