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

[Per Liden]

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> wrote:
> 
> On Apr 10, 2015, at 10:57 AM, Per Liden <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/
> 
> ------------------------------------------------------------------------------
> 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/~pliden/8077417/webrev.1/>

Diff against first webrev:
http://cr.openjdk.java.net/~pliden/8077417/webrev.1-diff_0vs1/ <http://cr.openjdk.java.net/~pliden/8077417/webrev.1-diff_0vs1/>

cheers,
/Per

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