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

[Thomas Stüfe]

Hi Coleen,

On Thu, Mar 15, 2018 at 12:26 AM, <coleen.phillimore at oracle.com> wrote:

>
>
> 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.
>
>
Thanks for clarifying!

>
> 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.
>
>
Thank you. I just pushed it.


> thanks,
> Coleen
>

..Thomas


>
>
> Thank you, Thomas
>
>
> On Tue, Mar 13, 2018 at 11:04 PM, <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
>>> webrev: http://cr.openjdk.java.net/~stuefe/webrevs/8199432-fix-space
>>> manager-
>>> 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
>>>
>>
>>
>
>