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

[Bengt Rutisson]

Hi Ramki,

Thanks for looking at this! I'm really happy you had time to review it! :-)

On 5/20/13 11: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! :-)

Yes, it was very tricky to find this occurrence. I had to use a single 
CPU Windows 32 bit machine to make it reproduce reliably. Let's hope it 
is the last case of this assert :-)

Thanks again for looking at this!
Bengt

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