RFR(M): 8008287: Code cache statistic variables for event tracing needs atomicity

[Christian Thalinger]

Looks good.  -- Chros

On Jun 23, 2013, at 6:19 AM, Nils Eliasson <nils.eliasson at oracle.com> wrote:

> hs25 port:
> 
> http://cr.openjdk.java.net/~neliasso/8008287/webrev.hs25.01/ <http://cr.openjdk.java.net/%7Eneliasso/8008287/webrev.hs25.01/>
> 
> Thanks!
> //Nils
> 
> On 2013-06-22 00:20, Vladimir Kozlov wrote:
>> Nils,
>> 
>> Why it is based on hsx24 sources? It should be pushed first into hs25 and tested there. For hs24 we would need approval to push.
>> 
>> Changes are good.
>> 
>> Thanks,
>> Vladimir
>> 
>> On 6/21/13 2:16 PM, Nils Eliasson wrote:
>>> Some new updates:
>>> 
>>> - Atomic::writes replaces the sweeper statistics lock.
>>> 
>>> http://cr.openjdk.java.net/~neliasso/8008287/webrev.07/
>>> <http://cr.openjdk.java.net/%7Eneliasso/8008287/webrev.07/>
>>> 
>>> Thanks,
>>> Nils
>>> 
>>> On 2013-06-18 17:21, Vladimir Kozlov wrote:
>>>> Yes, I think it is acceptable now. I will ask Christian to look on this.
>>>> 
>>>> Thanks,
>>>> Vladimir
>>>> 
>>>> On 6/18/13 7:15 AM, Nils Eliasson wrote:
>>>>> I have minimized the critical zones, moving max out. Also guarding
>>>>> all calls to os::elapsed_counter() with
>>>>> Tracing::enabled() and INCLUDE_TRACE. Had to reintroduce a
>>>>> duplication of timers in sweeper for some PrintMethodFlushing
>>>>> statistics behind ifdef ASSERT. Not super pretty or compact, but at
>>>>> least straightforward and easy to read.
>>>>> 
>>>>> http://cr.openjdk.java.net/~neliasso/8008287/webrev.06/
>>>>> <http://cr.openjdk.java.net/%7Eneliasso/8008287/webrev.06/>
>>>>> 
>>>>> //Nils Eliasson
>>>>> 
>>>>> On 2013-06-17 01:06, Vladimir Kozlov wrote:
>>>>>> Nils,
>>>>>> 
>>>>>> NMethodSweeper::speculative_disconnect_nmethods() is called during
>>>>>> safepoint (VM operation) so lock in it will affect
>>>>>> safepoint time. But I don't know how to avoid the lock here except
>>>>>> use it only when atomic long store is not
>>>>>> available. One thing you can do to shorten time in lock is to move
>>>>>> the addition and MAX2 operations outside the lock
>>>>>> (leave only stores inside) since it is the only place where these
>>>>>> values are modified.
>>>>>> 
>>>>>> +  {
>>>>>> +    MutexLockerEx ml(CodeSweeperStatistics_lock);
>>>>>>   _total_disconnect_time += disconnect_time;
>>>>>>   _peak_disconnect_time = MAX2(disconnect_time,
>>>>>> _peak_disconnect_time);
>>>>>> +  }
>>>>>> 
>>>>>> thanks,
>>>>>> Vladimir
>>>>>> 
>>>>>> On 6/16/13 3:00 PM, Nils Eliasson wrote:
>>>>>>> Updated,
>>>>>>> 
>>>>>>> Unnecessary lock removed from scan_stacks.
>>>>>>> 
>>>>>>> http://cr.openjdk.java.net/~neliasso/8008287/webrev.05
>>>>>>> 
>>>>>>> //Nils Eliasson
>>>>>>> 
>>>>>>> On 2013-04-30 17:27, Nils Eliasson wrote:
>>>>>>>> Hi,
>>>>>>>> 
>>>>>>>> I have fixed some atomicity issues for the sweeper tracing and did
>>>>>>>> some refactoring at the same time, removing
>>>>>>>> duplicate counters.
>>>>>>>> 
>>>>>>>> I guess people might have opinions on this, but I have chosen to
>>>>>>>> only use the perf_counters when available instead of
>>>>>>>> always having a normal counter and an additional perf_counter in
>>>>>>>> specific scenarios.
>>>>>>>> 
>>>>>>>> Also changed the type of the traversal counter (and the
>>>>>>>> stack_traversal marker in nmethod). It should be good for 126
>>>>>>>> years of sweeping once a second anyway (and we don't complete full
>>>>>>>> sweeps that often.)
>>>>>>>> 
>>>>>>>> http://bugs.sun.com/view_bug.do?bug_id=8008287
>>>>>>>> https://jbs.oracle.com/bugs/browse/JDK-8008287
>>>>>>>> http://cr.openjdk.java.net/~neliasso/8008287/webrev.04
>>>>>>>> <http://cr.openjdk.java.net/%7Eneliasso/8008287/webrev.04>
>>>>>>>> 
>>>>>>>> Thanks,
>>>>>>>> Nils Eliasson
>>>>>>>> 
>>>>>>>> 
>>>>>>> 
>>>>> 
>>> 
>