RFR(s): 8077417: Cleanup of Universe::initialize_heap()

[Per Liden]

On 2015-04-16 12:05, Stefan Karlsson wrote:
>
>
> On 2015-04-15 18:58, Per Liden wrote:
>> Hi Kim,
>>
>> Thanks for looking at this, sorry for the delayed reply.
>>
>>> On 13 Apr 2015, at 07:42, Kim Barrett <kim.barrett at oracle.com
>>> <mailto:kim.barrett at oracle.com>> wrote:
>>>
>>> On Apr 10, 2015, at 10:57 AM, Per Liden <per.liden at oracle.com
>>> <mailto:per.liden at oracle.com>> wrote:
>>>>
>>>> Hi,
>>>>
>>>> Universe::initialize_heap() is kind of messy as it contains lots of
>>>> #if INCLUDE_ALL_GCS, this patch tries to make that code a bit more
>>>> readable. Also, all collectors except ParallelScavenge take their
>>>> CollectorPolicy as an argument to the constructor. This patch aligns
>>>> ParallelScavenge to this pattern to further make the code in
>>>> initialize_heap() more readable.
>>>>
>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8077417
>>>>
>>>> Webrev: http://cr.openjdk.java.net/~pliden/8077417/webrev.0/
>>>> <http://cr.openjdk.java.net/%7Epliden/8077417/webrev.0/>
>>>
>>> ------------------------------------------------------------------------------
>>> src/share/vm/memory/universe.cpp
>>> 708   jint status;
>>> 709
>>> 710 #if !INCLUDE_ALL_GCS
>>> 711   if (UseParallelGC) {
>>> 712     fatal("UseParallelGC not supported in this VM.");
>>> ...
>>> 731   if (status != JNI_OK) {
>>> 732     return status;
>>>
>>> I think this might produce a compiler warning for uninitialized
>>> "status" at line 731 for at least some platforms. I think fatal() is
>>> not marked as non-returning, so to the compiler it appears that
>>> control flow can reach line 731 with an uninitialized "status”.
>>
>> I agree, I'll initialize it to JNI_ERR. It seems we don't get warnings
>> about this today because we don't enable -Wuninitialized. Stefan K has
>> a patch in the pipe to mark fatal() as not returning, but until then
>> I'll initialize it.
>>
>>>
>>> When do we build with INCLUDE_ALL_GCS being false?  I'm guessing only
>>> for embedded, and maybe client builds?  Were either of those part of
>>> the testing for these changes?
>>
>> It's false when we build the VM with --with-jvm-variants=minimal1,
>> which is used by some embedded targets (but not client).
>>
>>>
>>> The simplest fix is to initialize status to something, probably *not*
>>> JNI_OK; maybe JNI_ERR?
>>>
>>> ------------------------------------------------------------------------------
>>>
>>> src/share/vm/memory/universe.cpp
>>> 727   else { // UseSerialGC
>>>
>>> Add a guarantee that UseSerialGC is true here?
>>
>> It turns out that we have cases were no gc it selected, and then we
>> fall back to using serial. There's a bug to fix that:
>>
>> JDK-8068582: UseSerialGC not always set up properly
>>
>> I'll add a comment in the code to say that we can't assert there. Will
>> also add a comment to that bug to add an assert/guarantee when that
>> bug is fixed.
>>
>>>
>>> ------------------------------------------------------------------------------
>>> src/share/vm/memory/universe.cpp
>>> 735
>>>   ThreadLocalAllocBuffer::set_max_size(Universe::heap()->max_tlab_size());
>>>
>>> This used to be done before Universe::heap()->initialize().  Now it is
>>> being called after the heap initialization.  Is that change of order
>>> ok?
>>>
>>> I looked at it and it seems ok, in which case I think the change of
>>> order is actually an improvement.
>>
>> I looked at when I did the patch and concluded that it was ok. In
>> fact, it seemed a bit strange to set that up before calling initialize().
>>
>>>
>>> While I was looking around I noticed what might be a separate problem.
>>> It seems possible with a sufficiently large heap that G1's
>>> max_tlab_size computation might exceed the int[Integer.MAX_VALUE] size
>>> limit discussed in CollectedHeap::max_tlab_size. I'm not sure whether
>>> that limit is actually relevant to G1 though.  If it is relevant then
>>> a new CR should be filed.
>>
>> I don't think there's a problem there, G1's max_tlab_size will return
>> (region_size_in_words / 2) - 1 words, and G1's region size at most 32M
>> at the moment. Should be way below JINT_MAX.
>>
>>
>> Here's an updated webrev:
>> http://cr.openjdk.java.net/~pliden/8077417/webrev.1/
>> <http://cr.openjdk.java.net/%7Epliden/8077417/webrev.1/>
>>
>> Diff against first webrev:
>> http://cr.openjdk.java.net/~pliden/8077417/webrev.1-diff_0vs1/
>> <http://cr.openjdk.java.net/%7Epliden/8077417/webrev.1-diff_0vs1/>
>
> Looks good.
>

Thanks for reviewing Stefan!

/Per