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

[Bengt Rutisson]

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