RFR: 8232365: Implementation for JEP 363: Remove the Concurrent Mark Sweep (CMS) Garbage Collector

[Leo Korinth]

On 22/10/2019 21:37, Kim Barrett wrote:
>> On Oct 22, 2019, at 11:17 AM, Leo Korinth <leo.korinth at oracle.com> wrote:
>>
>> On 21/10/2019 23:48, Kim Barrett wrote:
>>>> On Oct 18, 2019, at 4:20 AM, Leo Korinth <leo.korinth at oracle.com> wrote:
>>> src/hotspot/share/gc/shared/collectedHeap.cpp
>>> 336 void CollectedHeap::check_for_non_bad_heap_word_value(HeapWord* addr, size_t size) {
>>>> Please take an extra look at CollectedHeap::check_for_non_bad_heap_word_value,
>>>> it was buggy before (but never called), It is now called (and hopefully
>>>> correct).
>>> I don't understand "but never called" as it looks like it has been
>>> called all along from MemAllocator::allocate_outside_tlab. Except
>>> that's only when UseTLAB is false, and it always defaults true if any
>>> of C1, C2, or JVMCI are included. We do have some -UseTLAB tests
>>> though, and they aren't all with CMS, so I'm not sure how a problem
>>> here hasn't already appeared. >
>>> Because I don't understand "but never called" I'm failing to
>>> understand how this hasn't shown up as a problem before. Seems like it
>>> should never have worked and there would be test failures at least.
>>
>> Sorry, I was sloppy in my description, it has not been called from our /test suite/. Special command line option (CheckMemoryInitialization && ZapUnusedHeapArea) needs to be given to the JVM for the check to be done. -XX:+CheckMemoryInitialization is only used in one test that hard codes serial gc. That -XX:+CheckMemoryInitialization flag looks quite unused might be a problem in itself, I do not know.
> 
> Thanks for the explanation.  I'd overlooked CheckMemoryInitialization.
> That actually seems like a reasonable use of a develop/debug option.
> Seems like there should be more tests though :)  Another RFE for you
> to file...

https://bugs.openjdk.java.net/browse/JDK-8232966

> 
> 
>>> Part of the problem here is that Copy::fill_to_words takes a juint
>>> fill value that it duplicates in high/low on 64bit platforms.  That
>>> seems wrong; shouldn't it take a platform-word sized value and just
>>> store that?  But making any such change is going to require some care.
>>> And it results in the CollectedHeap implementation's use of intptr_t
>>> on 64bit platforms comparing badHeapWord (a juint, so 32bits) with
>>> 64bit (((julong)badHeapWord << 32) | badHeapWord). So how did this
>>> ever manage to pass testing?
>>> However, on a 64bit platform the new code (and the old
>>> GenCollectedHeap code) is only checking every other 32bit value. The
>>> step distance is 8 bytes but the value read at each step is only 4
>>> bytes.
>>
>> The old code did not work, but what is wrong with the new code? ++ju_addr increments /four/ bytes at a time (ju_addr is a pointer to uint32_t), and (if I am not mistaken) the full memory range is checked as the sum of reinterpret_cast<juint*>(addr + size) is done in words (not juints) and /then/ casted to juint*.
>>
>> Or did I miss something?
> 
> Sorry about that; you are correct.  I got confused by the fill_to_words
> weirdness and how it interacts here.
> 
> 

Thanks Kim!

/Leo