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

[Vladimir Kozlov]

Yes, this looks good.

Thanks,
Vladimir

On 9/17/14 6: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
>>>>>
>>>>
>>>
>