RFR: 8024927: Nashorn performance regression with CompressedOops
Stefan Karlsson
stefan.karlsson at oracle.com
Fri Oct 25 01:25:29 PDT 2013
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?
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