RFR: 8024927: Nashorn performance regression with CompressedOops

[Coleen Phillimore]

Stefan, see below.

On 10/24/2013 9:20 AM, Stefan Karlsson wrote:
> On 10/24/13 3:01 PM, Coleen Phillimore wrote:
>>
>> Stefan,
>>
>> Thank you for the quick code review and testing.
>>
>> On 10/24/2013 4:45 AM, Stefan Karlsson wrote:
>>> On 10/24/13 6:03 AM, Coleen Phillimore wrote:
>>>> Summary: Allocate compressed class space at end of Java heap.  For 
>>>> small heap sizes, without CDS, reserve some space under 32G so 
>>>> compressed classes can have the same (more favorable) compression 
>>>> algorithm as oops.
>>>>
>>>> This change may have to be reverted when we implement extending 
>>>> compressed class spaces in the future, but it gets back the 
>>>> performance of these nashorn benchmarks, and seems to make sense 
>>>> for small heaps.
>>>>
>>>> open webrev at http://cr.openjdk.java.net/~coleenp/8024927/
>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8024927
>>>
>>> Hi Coleen,
>>>
>>> If I you run with a small heap, say -Xmx128m, you get shifted 
>>> compressed klass pointers. Is that intentional?
>>>
>>> $ java -Xmx128m -XX:+PrintCompressedOopsMode -version
>>>
>>> heap address: 0x00000000f8000000, size: 128 MB, zero based 
>>> Compressed Oops, 32-bits Oops
>>>
>>> Narrow klass base: 0x0000000000000000, Narrow klass shift: 3
>>> Compressed class space size: 1073741824 Address: 0x0000000100000000 
>>> Req Addr: 0x0000000100000000
>>>
>>
>> The top address 0x0000000100000000 + size must require the shift, 
>> since the base is zero (ie, the code did this math).
>
> Sure. But do we want the top address to be 0x0000000100000000 when we 
> probably could fit both heap and the compressed class space in the 
> lower 4GB? But let's ignore that for this change.
>
>>>
>>> Some comments:
>>>
>>> http://cr.openjdk.java.net/~coleenp/8024927/src/share/vm/memory/metaspace.cpp.frames.html 
>>>
>>>
>>> +  uint64_t klass_encoding_max = NarrowOopHeapMax << 
>>> LogKlassAlignmentInBytes;
>>>
>>> Shouldn't you be using this constant: ?
>>>  const uint64_t KlassEncodingMetaspaceMax = (uint64_t(max_juint) + 
>>> 1) << LogKlassAlignmentInBytes;
>>>
>>
>> Yes, it's the same constant as NarrowOopHeapMax so I made it common.  
>> I guess the name is confusing so I reverted that bit of the change 
>> and have a copy of the constant in the code.
>
> OK. I'll check the new webrev.
>
>>
>>>
>>> 2818   uint64_t klass_encoding_max = NarrowOopHeapMax << 
>>> LogKlassAlignmentInBytes;
>>> 2819   // If compressed class space fits in lower 32G, we don't need 
>>> a base.
>>> 2820   if (higher_address <= (address)klass_encoding_max) {
>>> 2821     Universe::set_narrow_klass_base(0);
>>> 2822     lower_base = 0; // effectively lower base is zero.
>>> 2823   } else {
>>> 2824     Universe::set_narrow_klass_base(lower_base);
>>> 2825   }
>>> 2826
>>> 2827   if ((uint64_t)(higher_address - lower_base) < 
>>> (uint64_t)max_juint) {
>>> 2828     Universe::set_narrow_klass_shift(0);
>>> 2829   } else {
>>> 2830     assert(!UseSharedSpaces, "Cannot shift with UseSharedSpaces");
>>> 2831 Universe::set_narrow_klass_shift(LogKlassAlignmentInBytes);
>>> 2832   }
>>>
>>> If you execute line 2822, will you hit the assert at line 2830 if 
>>> you run with CDS and set SharedBaseAddress.
>>>
>>> $ java -Xshare:dump -Xmx128m -XX:SharedBaseAddress=8g 
>>> -XX:+PrintCompressedOopsMode -XX:+VerifyBeforeGC -version
>>> ...
>>> $ java -Xshare:on -Xmx128m -XX:SharedBaseAddress=8g 
>>> -XX:+PrintCompressedOopsMode -XX:+VerifyBeforeGC -version
>>> ...
>>
>> I hadn't completely excluded UseSharedSpaces from this change, which 
>> I intended to do.  I fixed this now.
>>
>>>
>>> 2852   assert_is_ptr_aligned(requested_addr, _reserve_alignment);
>>> 2853   assert_is_ptr_aligned(cds_base, _reserve_alignment);
>>> 2854   assert_is_size_aligned(compressed_class_space_size(), 
>>> _reserve_alignment);
>>>
>>> Could you keep the alignment, or remove it?
>>
>> What do you mean?
>
> I mean that you messed up the indentation of _reserve_alignment. Maybe 
> a copy and past from the patch will show this:
>     assert_is_ptr_aligned(requested_addr,          _reserve_alignment);
>     assert_is_ptr_aligned(cds_base,                _reserve_alignment);
> -  assert_is_size_aligned(class_metaspace_size(), _reserve_alignment);
> +  assert_is_size_aligned(compressed_class_space_size(), _reserve_alignment);


Oh THAT alignment.  I removed it since it looked funny with so many spaces.

>
>>>
>>>
>>> -        allocate_metaspace_compressed_klass_ptrs((char 
>>> *)CompressedKlassPointersBase, 0);
>>> +        char* base = (char*)Universe::heap()->reserved_region().end();
>>> +        allocate_metaspace_compressed_klass_ptrs(base, 0);
>>>
>>> You need to make sure that 'base' address is aligned against 
>>> Metaspace::reserve_alignment().
>>>
>>
>> Fixed.
>>
>>>
>>> http://cr.openjdk.java.net/~coleenp/8024927/src/share/vm/memory/universe.cpp.udiff.html 
>>>
>>>
>>> +          if (UseCompressedClassPointers &&
>>> +              LogMinObjAlignmentInBytes == LogKlassAlignmentInBytes) {
>>> +            // For small heaps, save some space for compressed 
>>> class pointer
>>> +            // space with no base, only when ObjAlignmentInBytes is 
>>> 64 bits.
>>> +            uint64_t class_space = 
>>> align_size_up(CompressedClassSpaceSize, alignment);
>>> + assert(is_size_aligned((size_t)OopEncodingHeapMax-class_space,
>>> +                   alignment), "difference must be aligned too");
>>> +            uint64_t new_top = OopEncodingHeapMax-class_space;
>>> +
>>> +            if (total_size <= new_top) {
>>> +              base = (new_top - heap_size);
>>> +            } else {
>>> +              base = (OopEncodingHeapMax - heap_size);
>>> +            }
>>>
>>> I don't understand why you have this restriction:
>>>  LogMinObjAlignmentInBytes == LogKlassAlignmentInBytes
>>>
>>
>> Because if you use a bigger ObjAlignmentInBytes, you probably have a 
>> bigger heap and this won't work anyway and I didn't want to support 
>> and test it.
>
> OK. Could you change the check to compare LogMinObjAlignmentInBytes to 
> 3, or ObjAlignmentInBytes to 8, or OopEncodingHeapMax to 32GB? That 
> would be much more clear too me.

I don't like constants in code, which is why I picked the symbolic class 
alignment name for "3".   I'll add a comment.  How about that?

thanks,
Coleen

>
>> I am rerunning some tests on the fixes.  I'll send out another webrev 
>> shortly.
>>
>> Thanks so much for breaking my code!  Srsly.
>
> OK. I'll wait for the update.
>
> thanks,
> StefanK
>>
>> Coleen
>>
>>> thanks,
>>> StefanK
>>>
>>>>
>>>> Tested with ute vm.quick.testlist vm.mlvm.testlist on linux/x64 
>>>> solaris/x64 and windows/x64 (in progress).  Also running refworkload.
>>>>
>>>> Thanks,
>>>> Coleen
>>>
>>
>