Request for review (s) - 7195789

[Bengt Rutisson]

Hi Jon,

Thanks for proposing a fix even after the first change was pushed. I 
appreciate it!

I think it is a good idea to clean up the previous fix a bit, so thanks 
for sending out a new webrev. But if you have more important PermGen 
bugs to fix I think you should focus on them and disregard my comments 
below for now. From my perspective we can save 7196298 until things have 
calmed down a bit in the PermGen work.

On 2012-09-05 17:46, Jon Masamitsu wrote:
> Based on review comments, this is a better fix for 7195789.
>
> 7196298: Better fix for 7195789
>
> http://cr.openjdk.java.net/~jmasa/7196298/webrev.00/

I'm not so sure I like how the "unsafe" parameter propagates out in the 
code. It is only important for the assert and, as you mentioned in an 
earlier email, the assert is placed in MetaspaceAux::used_in_bytes() 
just to be somewhere. It does not necessarily have to be in that method. 
So, to me we are making the surrounding code more difficult to follow 
just to be able to add the assert.

How about making it more explicit? Maybe we can add a method 
MetaspaceAux::check_consitency() that looks something like:

void MetaspaceAux::check_consitency(Metaspace::MetadataType mdtype) {
   assert(SafepointSynchronize::is_at_safepoint(), "must be at safepoint");
   ClassLoaderDataGraphMetaspaceIterator iter;
   while (iter.repeat()) {
     Metaspace* msp = iter.get_next();
     if (msp != NULL) {
       size_t used = msp->used_words(mdtype);
       size_t free = msp->free_words(mdtype);
       size_t capacity = msp->capacity_words(mdtype);
       assert(used + free == capacity,
         err_msg("Accounting is wrong used " SIZE_FORMAT
                 " free " SIZE_FORMAT " capacity " SIZE_FORMAT,
                 used, free, capacity));
     }
   }
}

And make MetaspaceAux::used_in_bytes() simply be:

size_t MetaspaceAux::used_in_bytes(Metaspace::MetadataType mdtype) {
   size_t used = 0;
   ClassLoaderDataGraphMetaspaceIterator iter;
   while (iter.repeat()) {
     Metaspace* msp = iter.get_next();
     // Sum allocation_total for each metaspace
     if (msp != NULL) {
       used += msp->used_words(mdtype);
     }
   }
   return used * BytesPerWord;
}

That way we can explicity do:

   size_t metadata_prev_used = MetaspaceAux::used_in_bytes();
   debug_only(MetaspaceAux::check_consistency());

In places where we know we are at a safepoint.

Bengt


>
>
> On 9/4/2012 4:16 PM, Jon Masamitsu wrote:
>>
>> 7195789: NPG: assert(used + free == capacity) failed: Accounting is 
>> wrong
>>
>> The assertion failure was occurring at a point where an allocation by 
>> another thread could invalidate
>> the expected consistency. Created an unsafe version of 
>> used_in_bytes() for use in those cases.
>>
>> http://cr.openjdk.java.net/~jmasa/7195789/webrev.00/
>>
>>