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

[Coleen Phillimore]

Hi Goetz,

I agree, don't add #2.   If you send me the complete patch for the test, 
I'll run it through JPRT.   Hopefully the test will "work" on all 
platforms, but if it doesn't you shouldn't add it after all.

Thanks,
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.
>