Request for review (s) - 7195789

[Jon Masamitsu]

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.

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".

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/
>>>
>>>
>