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

[Vladimir Kozlov]

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