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

[Kim Barrett]

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


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