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

[Vladimir Kozlov]

Can you make build_method_counters() private and replace it with 
get_method_counters() call. There are 2 places where it is called 
directly, in NMethodSweeper::possibly_flush() and in 
InterpreterRuntime::build_method_counters(). My concern is that 
build_method_counters() assumes that caller does NULL check before 
calling it (so you can do cas == NULL check). It is true for 
possibly_flush() but the code there is the same as in 
get_method_counters(). InterpreterRuntime::build_method_counters() is 
called from template code which chekcs for NULL bat it is not obvious.

Thanks,
Vlaidmir

On 9/17/14 4:09 PM, Igor Veresov wrote:
> Ok, here the webrev that takes care of the leak:
> http://cr.openjdk.java.net/~iveresov/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/
>>>>>>
>>>>>> 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
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>> igor
>