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

[Jon Masamitsu]

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();)
>     }

A definite yes to this.
>
> 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.

Let me refresh my memory on this code and think about what would be
adequate checks.

Thanks.

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