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

[Jiangli Zhou]

Hi Vladimir,

On 09/17/2014 01:46 PM, Vladimir Kozlov wrote:
> I think in this place we trade off the update of a field without lock 
> vs small memory leak which can be fixed.

That was the reason. :)

> I think lock is not needed here.
>
> In Method::build_method_counters() we can free allocated space if we 
> load different value (_method_counters field should be volatile to 
> prevent optimization of the load):
>
> MethodCounters* Method::build_method_counters(Method* m, TRAPS) {
>   methodHandle mh(m);
>   ClassLoaderData* loader_data = 
> mh->method_holder()->class_loader_data();
>   MethodCounters* counters = MethodCounters::allocate(loader_data, 
> CHECK_NULL);
>   if (mh->method_counters() == NULL) {
>     mh->set_method_counters(counters);
>   }
>   MethodCounters* mcs = mh->method_counters();
>   if (counters != mcs) {
>     MetadataFactory::free_metadata(loader_data, counters);
>   }
>   return mcs;
> }

Right, we can free the method counter if someone beat us to set it. We 
also need to use Atomic::cmpxchg when setting the method counters. That 
would fix the leak.

Thanks,
Jiangli

>
> Vladimir
>
> On 9/17/14 10:53 AM, Igor Veresov wrote:
>>
>> On Sep 17, 2014, at 8:32 AM, David Chase <david.r.chase at oracle.com> 
>> wrote:
>>
>>>
>>> On 2014-09-17, at 6:01 AM, Igor Veresov <igor.veresov at oracle.com> 
>>> wrote:
>>>
>>>> Alright, how about a shorter fix: 
>>>> http://cr.openjdk.java.net/~iveresov/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?
>>
>> igor
>>
>>
>>> David
>>>
>>>> On Sep 17, 2014, at 2:35 AM, Igor Veresov <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> 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> 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
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> igor
>>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>