RFR: 8024927: Nashorn performance regression with CompressedOops
Coleen Phillimore
coleen.phillimore at oracle.com
Fri Oct 25 07:20:42 PDT 2013
On 10/25/2013 4:25 AM, Stefan Karlsson wrote:
>
> On 2013-10-25 03:13, Coleen Phillimore wrote:
>> Summary: Allocate compressed class space at end of Java heap. For
>> small heap sizes, without CDS, save some space so compressed classes
>> can have the same favorable compression as oops
>>
>> I've updated the code to resolve the issues that Stefan found and
>> implemented suggestions from Stefan and Goetz. I did additional
>> testing and added a test. Please review again.
>>
>> http://cr.openjdk.java.net/~coleenp/8024927_2/
>
> You set up the narrow_klass_max as 4GB (32 bits):
> 2817 const uint64_t narrow_klass_max = (uint64_t(max_juint) + 1);
>
> Are the following usages of (uint64_t)max_juint wrong?
Yes, they should all be +1. I've introduced constant
UnscaledClassSpaceMax to match the similar constant with the same value
in universe.cpp (UnscaledOopHeapMax - name suggested by Goetz) and
replaced the uses.
Thanks,
Coleen
> 2827 if ((uint64_t)(higher_address - lower_base) <
> (uint64_t)max_juint) {
>
> 2843 return ((uint64_t)(higher_address - lower_base) <
> (uint64_t)max_juint);
>
> 3007 if (cds_total + compressed_class_space_size() >
> (uint64_t)max_juint) {
>
> Should they be changed to:
> 2827 if ((uint64_t)(higher_address - lower_base) <=
> ((uint64_t)max_juint + 1)) {
>
> 2843 return ((uint64_t)(higher_address - lower_base) <=
> ((uint64_t)max_juint + 1));
>
> 3007 if (cds_total + compressed_class_space_size() >
> ((uint64_t)max_juint + 1)) {
>
> Or rather:
> 2827 if ((uint64_t)(higher_address - lower_base) <= narrow_klass_max) {
>
> 2843 return ((uint64_t)(higher_address - lower_base) <=
> narrow_klass_max);
>
> 3007 if (cds_total + compressed_class_space_size() >
> narrow_klass_max) {
>
> thanks,
> StefanK
>
>>
>> Thanks,
>> Coleen
>>
>> 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);
>>>
>>>>>
>>>>>
>>>>> - 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
>>>>>
>>>>
>>>
>>
>
More information about the hotspot-dev
mailing list