RFR: 8024927: Nashorn performance regression with CompressedOops

[Coleen Phillimore]

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

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

I am rerunning some tests on the fixes.  I'll send out another webrev 
shortly.

Thanks so much for breaking my code!  Srsly.

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
>