RFR (S): 7066063: CMS: "Conservation Principle" assert failed

[Bengt Rutisson]

Hi Jon,

Thanks for looking at this!

On 5/21/13 7:54 AM, Jon Masamitsu wrote:
> Bengt,
>
> Your explanation is very compelling.  Change looks good.

Great! Thanks!

All set to push this now.

Bengt

>
> Jon
>
> On 5/20/2013 2:16 PM, Srinivas Ramakrishna wrote:
>> Hi Bengt --
>>
>> Thanks for the nice description! The change looks right to me. (I seem
>> to recall having spent a bunch of time on this many moons ago, and
>> still not managed to get rid of the last occurrence of this assert;
>> looks like you may have found the "last" case that was confounding the
>> assert! :-)
>>
>> Reviewed!
>> -- ramki
>>
>> On Mon, May 20, 2013 at 1:43 PM, Bengt Rutisson
>> <bengt.rutisson at oracle.com> wrote:
>>> Hi everyone,
>>>
>>> Could I have a couple of reviews for this small change?
>>> http://cr.openjdk.java.net/~brutisso/7066063/webrev.00/
>>>
>>> Background :
>>>
>>> We hit this assert in the allocation statistics for CMS:
>>>
>>> assert(prevSweep() + splitBirths() + coalBirths() >= splitDeaths() +
>>> coalDeaths() + (ssize_t)count) failed: Conservation Principle
>>>
>>> The problem is in CompactibleFreeListSpace::reset(). That method calls
>>> FreeList<Chunk>::return_chunk_at_head(), which calls increment_count().
>>>
>>> All other call sites that increment the count also updates either 
>>> the split
>>> birth or coal birth statistics, but 
>>> CompactibleFreeListSpace::reset() does
>>> not do that.
>>>
>>> My reproducer hits the assert every time I run it. If I add a call to
>>> coalBirth() in CompactibleFreeListSpace::reset() it never hits the 
>>> assert.
>>>
>>> Here are the other calls to return_chunk_at_head and how they update 
>>> the
>>> statistics:
>>>
>>> CompactibleFreeListSpace:: par_get_chunk_of_blocks()
>>>   -> calls set_split_births()
>>>
>>> CompactibleFreeListSpace::returnChunkToFreeList()
>>> -> coalBirth() in
>>> CompactibleFreeListSpace::addChunkToFreeListsAtEndRecordingStats()
>>> -> coalBirth() in SweepClosure::flush_cur_free_chunk()
>>> -> split_birth() in 
>>> CompactibleFreeListSpace::getChunkFromLinearAllocBlock()
>>>
>>> CompactibleFreeListSpace::splitChunkAndReturnRemainder()
>>> -> split_birth() in CompactibleFreeListSpace::split()
>>>
>>>
>>> So, all other calls than CompactibleFreeListSpace::reset() either 
>>> increment
>>> the split birth or coal birth counters. When we set up the heap we
>>> eventually end up in the
>>> CompactibleFreeListSpace::addChunkToFreeListsAtEndRecordingStats() 
>>> which
>>> increments the coal birth.
>>>
>>> Since CompactibleFreeListSpace::reset() is called from
>>> CompactibleFreeListSpace::reset_after_compaction() when the freed up 
>>> memory
>>> from the compaction is added to the free list it seems reasonable to 
>>> update
>>> the same counter as when we first set up the heap. Thus, 
>>> incrementing the
>>> coal birth seem more correct than incrementing the split birth count.
>>>
>>> Thanks,
>>> Bengt
>