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

[Stefan Karlsson]

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.

StefanK

>
> cheers,
> /Per
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20150416/4bc2ea7e/attachment.htm>