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

[Vladimir Kozlov]

Reviewed. Good.

Thanks,
Vladimir

On 2/5/15 3:12 PM, Coleen Phillimore wrote:
>
> 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.
>>>>
>>>
>>
>