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

[Coleen Phillimore]

Webrev 01 is good.  You need another reviewer and I'll sponsor it.

http://cr.openjdk.java.net/~goetz/webrevs/8072434-css/webrev.01/

thanks,
Coleen

On 2/5/15, 5:22 PM, Coleen Phillimore wrote:
>
> On 2/5/15, 5:16 PM, Coleen Phillimore wrote:
>>
>> 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.
>
> Forgot to mention it didn't pass on windows x64 maybe because of ASLR.
> Coleen
>
>>
>> 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.
>>>
>>
>