RFR(xxs): 8199432: metaspace: fix wrong comment and condition in SpaceManager::verify()

[coleen.phillimore at oracle.com]

On 3/14/18 3:01 PM, Thomas Stüfe wrote:
> Hi Coleen,
>
> short question. This patch changes SpaceManager::verify in the sense 
> that now SpaceManager::verify does now do something useful (iterating 
> the chunks of a class loader). This - over several steps - will lead 
> to "VerifyBefore/AfterGC" making more verifications in fastdebug.

This is an odd conditional:

- if (block_freelists() != NULL && block_freelists()->total_size() == 0) {

block_freelists() are null if nothing has been individually deallocated 
(which is typical unless class loading failure, redefininition, or some 
special cases of interfaces not overriding default methods).  So I think 
removing the conditional would make verify run more, but it would be 
only when you run -XX:Verify though.  If it turns out to be a problem, 
we could add another metaspace_slow_verify to it.

>
> I ran this patch through submit-hs (what I suppose is "tier1"?) and 
> all was well. Should I test anything else? I am a bit unsure since 
> yesterday.
>

If you've tested it internally, and run submit-hs, that should be good.

thanks,
Coleen

> Thank you, Thomas
>
>
> On Tue, Mar 13, 2018 at 11:04 PM, <coleen.phillimore at oracle.com 
> <mailto:coleen.phillimore at oracle.com>> wrote:
>
>
>     This looks good and potentially trivial.
>
>     Coleen
>
>
>     On 3/12/18 3:23 AM, Thomas Stüfe wrote:
>
>         Hi all,
>
>         may I please have sponsor/reviewers for this tiny fix:
>
>         Bug: https://bugs.openjdk.java.net/browse/JDK-8199432
>         <https://bugs.openjdk.java.net/browse/JDK-8199432>
>         webrev:
>         http://cr.openjdk.java.net/~stuefe/webrevs/8199432-fix-spacemanager-
>         <http://cr.openjdk.java.net/%7Estuefe/webrevs/8199432-fix-spacemanager->
>         verify/webrev.00/webrev/
>
>         Basically, the condition and the comment make no sense anymore.
>
>         They used to make sense: in an earlier version of the
>         Metaspace ("6964458:
>         Reimplement class meta-data storage to use native memory") the
>         Metablocks
>         inside a metachunk always had headers and therefore were
>         walkable. So,
>         Metachunk::verify walked all the blocks - which was not
>         possible if were
>         returned with Metaspace::deallocate() and added to the block
>         dictionary.
>
>         This is not true anymore, now MetaBlocks generally do not have
>         a header, so
>         MetaChunk::verify() does not walk them and is generally
>         oblivious to
>         anything happening in the MetaChunk payload area. So, one can
>         now always
>         call MetaChunk::verify(), regardless if MetaBlocks are in the
>         block
>         dictionary or not.
>
>
>         Thanks, Thomas
>
>
>