Request for review (s) - 7195789

[Bengt Rutisson]

Jon,

On 2012-09-06 01:50, Jon Masamitsu wrote:
> Bengt,
>
> The point of not complicating code unnecessarily is certainly
> worth considering.  I have to smile a little though since your
> suggestion does duplicate code.

Point taken. :-)

But to my defense the duplicated code with the check_consistency() 
method is just boiler plate in the same way that there is code 
duplication between MetaspaceAux::used_in_bytes(), 
MetaspaceAux::free_in_bytes() and MetaspaceAux::capacity_in_bytes(). If 
we want to change how used_in_bytes() is really calculated there is only 
one place to change with my suggestion.

> The reason I put the consistency check where it is,
> is that I'm doing the check without having to remember
> to call another function.  It's strictly a convenience.
>
> The current consistency check is actually not worth doing.
> The original check had to do with a running sum that was
> maintained as metadata was allocated but that's not there
> anymore.  I'm going to delete the check and revert to
> used_in_bytes() without the "bool unsafe".

I agree that the simplest solution is to remove the check, but I don't 
fully understand why the check is less important now than before.

Anyway, thanks for keeping the interest in this small issue and it 
sounds good to me to remove the assert.

Bengt

>
> Jon
>
> On 9/5/2012 2:13 PM, Bengt Rutisson wrote:
>>
>> 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/
>>>>
>>>>
>>