RFR(S): 8072434: 8064457: introduces performance regressions in 9-b47

[Coleen Phillimore]

Your test #1 doesn't work on all machines either.  I don't think we can 
add tests that assume memory allocation locations, so we shouldn't add 
your new test.  It's been verified manually.

Coleen

On 2/5/15, 10:00 AM, Lindenmaier, Goetz wrote:
>
> Hi Coleen,
>
> You are right, I missed to move that condition into the new code.
>
> I designed a test for zerobased startup with 30G I would like to add to
>
> the webrev as you proposed.  I had to use WhiteBox, which took a while
>
> to get right ...
>
> I thought of these two test cases.
>
> 1.) Setting 30G plain, and
>
> 2.) setting the flags used in the benchmark.  -Xmx30g -Xms30g 
> -Xmn26g-XX:+AggressiveOpts -XX:+UseNUMA
>
> 2.) causes two problems.  First it's slow as the debug build 
> initializes the heap which walks all
>
>       the memory.  Second it fails on machines that don't have 30G 
> main memory. So I have to
>
>      skip it.
>
> 1.)I'm not sure whether that will work with all OSes, the OS might not 
> return all
>
> of the theoretical available address space below 32G, even if 
> zerobased works
>
>         for medium size heaps.
>
> I will run 1.) with my tests tonight for a broader coverage, it worked 
> on linuxx86_64 and
>
> linuxppc64 fine, and breaks with the VM without the fix.
>
> Also, all our other tests worked fine with the patch from yesterday.
>
> Best regards,
>
>   Goetz.
>
> *From:*Coleen Phillimore [mailto:coleen.phillimore at oracle.com]
> *Sent:* Thursday, February 05, 2015 12:14 AM
> *To:* Lindenmaier, Goetz; hotspot-dev Source Developers
> *Subject:* Re: RFR(S): 8072434: 8064457: introduces performance 
> regressions in 9-b47
>
>
> Hi Goetz,
>
> I was trying to figure out how we used to decide whether to put the 
> compressed class space above the heap in preferred_heap_base().  The 
> code was:
>
>     const size_t total_size = heap_size + 
> heap_base_min_address_aligned;  // 30G + 2G
>
>     ...
>           // space so it can be decoded with no base.
>           if (UseCompressedClassPointers && !UseSharedSpaces &&
>               OopEncodingHeapMax <= 32*G) {
>
>             uint64_t class_space = 
> align_size_up(CompressedClassSpaceSize, alignment);   // 1G
> assert(is_size_aligned((size_t)OopEncodingHeapMax-class_space,
>                    alignment), "difference must be aligned too");
>             uint64_t new_top = OopEncodingHeapMax-class_space;    // 
> 32G - 1G
>
>             if (total_size <= new_top) {    // 32G <= 31G  false, 
> don't use the new_top.
>               heap_top = new_top;
>             }
>           }
>
>           // Align base to the adjusted top of the heap
>           base = heap_top - heap_size;
>
>
> This is also one instance where 32*G is much easier to understand in 
> the code than the symbolic name OopEncodingHeapMax.
>
> Anyway, your code fix looks correct.  You want it to be 
> KlassEncodingMetaspaceMax logically (I thought it should be 
> OopEncodingHeapMax) since allocating the class space above the heap 
> and getting zero based compressed oops, the class decoding algorithm 
> can only shift by 3 not 5.  The clause above it excludes the 
> ObjAlignmentInBytes = shifting 5 case anyway.
>
> Thank you for fixing this!
>
> Coleen
>
> On 2/4/15, 10:55 AM, Lindenmaier, Goetz wrote:
>
>     Hi,
>
>     There is a performance regression after my change 8064457.
>
>     30G heaps do not displace the CompressedClassSpace from the lower
>     memory
>
>     region and instead are allocated in upper regions.
>
>     This webrev should fix the Problem.
>
>     http://cr.openjdk.java.net/~goetz/webrevs/8072434-css/webrev.01/
>     <http://cr.openjdk.java.net/%7Egoetz/webrevs/8072434-css/webrev.01/>
>
>     I verified that it works now and ran runtime/CompressedOops jtreg
>     tests on linuxx86_64.
>
>     I'll get a comprehensive test run by tomorrow.
>
>     Best regards,
>
>       Goetz.
>