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

Thu Sep 18 01:14:23 UTC 2014

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