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

[Jon Masamitsu]

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/

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
>