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

[Igor Veresov]

I simplified the code in the sweeper: http://cr.openjdk.java.net/~iveresov/8058564/webrev.03

igor

On Sep 17, 2014, at 4:32 PM, Vladimir Kozlov <vladimir.kozlov at oracle.com> wrote:

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