Request for Review (XS) - 8149643: Remove check of counters in VirtualSpaceNode::inc_container_count

[Bengt Rutisson]

Hi Jon,

On 2016-02-12 06:39, Jon Masamitsu wrote:
> Version 2 of the webrev.   I've hoisted the first verification out of
> both loops in retire() and moved the second one out of the inner
> loop.  I've also added the verification to the purge() method because
> it is there that the count is used and an incorrect value could
> go unnoticed.
>
> http://cr.openjdk.java.net/~jmasa/8149643/webrev.01/

Looks good to me.

Thanks,
Bengt

>
> Thanks.
>
> Jon
>
> On 2/11/2016 12:28 PM, Bengt Rutisson wrote:
>>
>> Hi Jon and Dmitry,
>>
>> On 2016-02-11 18:56, Jon Masamitsu wrote:
>>> https://bugs.openjdk.java.net/browse/JDK-8149643
>>>
>>> As part of managing Metaspaces a running count of the
>>> number of Metachunks in a virtual-space is maintained.
>>> The count is incremented as chunks are allocated from the
>>> virtual-space.  The count can be verified by walking over
>>> all the Metachunks in the virtual space.  This change
>>> removes the verification code which was called each time
>>> the running count was incremented.  The verification is
>>> costly and no longer worth doing at each increment.  It is
>>> still verified when the virtual-space is being retired.
>>>
>>> Contributed by: dmitry.dmitriev at oracle.com
>>>
>>> http://cr.openjdk.java.net/~jmasa/8149643/webrev.00/
>>
>> Looks ok, but I have a question about the other place where the 
>> verification is done. In VirtualSpaceNode::retire().
>>
>> It does:
>>
>>
>>     while (free_words_in_vs() >= chunk_size) {
>>       DEBUG_ONLY(verify_container_count();)
>>       // do some stuff...
>>       DEBUG_ONLY(verify_container_count();)
>>     }
>>
>> The first verification call can only every fail the first time (after 
>> that it is just one more call to the verification just after we 
>> returned from doing verfication). So, if this is considered an 
>> expensive verification I would suggest changing this to:
>>
>>     DEBUG_ONLY(verify_container_count();)
>>     while (free_words_in_vs() >= chunk_size) {
>>       // do some stuff...
>>       DEBUG_ONLY(verify_container_count();)
>>     }
>>
>> This code still looks a bit paranoid so I think there are even more 
>> simplifications to be done if desired, but at least removing the 
>> double verification seems like worth while doing.
>>
>> Thanks,
>> Bengt
>>
>>
>>>
>>> The change is the deletion of 1 line.
>>>
>>> --- a/src/share/vm/memory/metaspace.cpp
>>>
>>> +++ b/src/share/vm/memory/metaspace.cpp
>>>
>>> @@ -791,7 +791,6 @@
>>>
>>>  void VirtualSpaceNode::inc_container_count() {
>>>
>>>    assert_lock_strong(SpaceManager::expand_lock());
>>>
>>>    _container_count++;
>>>
>>> -  DEBUG_ONLY(verify_container_count();)
>>>
>>>  }
>>>
>>>
>>> Testing with some specjvm benchmarks.
>>>
>>> Thanks.
>>>
>>> Jon
>>
>