Request for review: 8009778: NPG: ClassMetaspaceSize is used before set in set_ergonomics_flags()

[Coleen Phillimore]

Hi Thomas,  It doesn't look like this change fixes the set before used 
problem of ClassMetaspaceSize but is a more comprehensive fix so I think 
you should fix Harold's bug with your fix.   I only looked at 
arguments.cpp,hpp though.

Actually, both changes are wrong for this because you only set 
ClassMetaspaceSize to 100*M (agreed, not a good size) if 
UseCompressedKlassPointers is set which you don't set until later.   
Harold is working on a fix move the class metaspace somewhere else, not 
attached to the Java heap calculations which will resolve this issue.   
So I think we should leave this broken for now.

Coleen

On 5/30/2013 4:59 PM, Thomas Schatzl wrote:
> Hi all,
>
> On Thu, 2013-05-30 at 14:05 -0400, harold seigel wrote:
>> Hi,
>>
>> Please review this fix for bug 8009778.
>>
>> The fix adds a function, class_metaspace_size_for_compressed_ptrs(),
>> that returns what the value will be for ClassMetaspaceSize if
>> CompressedKlassPointers are used.  This enables the correct value for
>> ClassMetaspaceSize to be used by max_heap_for_compressed_oops() before
>> ClassMetaspaceSize gets set in set_ergonomics_flags().
>>
> You may want to have a look at the change for 8010722 which is currently
> under review on the hotspot-gc list, which (probably) also fixes this
> one. Unfortunately I forgot to CC hotspot-runtime-dev.
>
> The mail at
> http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2013-May/007155.html is the start of that thread; it would be nice btw if somebody of you did have some time to look over it.
>
> That change is very similar, I have no particular preference for either
> solution.
>
> I have some comment about the choice of 100M for the default table size:
> it is a very "odd" size for heap areas. I mean, later in
> Universe::reserve_heap() this size is aligned up by the alignment given
> to that function.
>
> That alignment is always a power of two, and there are a few cases when
> this required alignment is quite big, i.e. 32M+.
>
> One case is if G1 selects a heap region size of 32M, the other much more
> common is probably on SPARC where if the "small" OS page size is set to
> 64k, this alignment is also always 32M regardless of collector.
>
> Hotspot further down actually expands the actual ClassMetaspaceSize
> size. So it is no real problem except that it seemed somewhat strange to
> me. This is just an observation, nothing to do about it (I think), I
> just asked myself why somebody would choose a value of 100M when doing
> the fix for 8010722 knowing that there's a lot of power-of-two
> alignments in that code.
>
> Btw, I do think there's some issue here with
> Universe::set_class_metaspace_size() and the alignment in
> Universe::reserve_heap() though: the Universe::_class_metaspace_size
> does not seem to be synchronized with the ClassMetaspaceSize global, so
> any tool (jmap I think) that later reads and prints this value from the
> global will give wrong output. I.e. return the unaligned, not the
> actual, value at startup.
>
>> The fix was tested with JCK lang and vm tests, UTE vm.quick.testlist and
>> vm.mlvm.testlist tests, JPRT, and jtreg tests. Tests were run on Linux
>> and Solaris. The debugger was used to step through the code to ensure
>> that the fix behaved as expected.
>>
>> Open webrev at http://cr.openjdk.java.net/~hseigel/bug_8009778/
>> <http://cr.openjdk.java.net/%7Ehseigel/bug_8009778/>
>>
>> Bug link at http://bugs.sun.com/view_bug.do?bug_id=8009778
>>
>> JBS bug link at https://jbs.oracle.com/bugs/browse/JDK-8009778
> Apologies again for not CC'ing hotspot-runtime-dev about the 8010722.
> issue.
>
> Thanks,
>    Thomas
>
>