RFR: 8024927: Nashorn performance regression with CompressedOops

[Stefan Karlsson]

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);

>>
>>
>> -        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 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
>>
>