RFR: 8024927: Nashorn performance regression with CompressedOops
harold seigel
harold.seigel at oracle.com
Fri Oct 25 12:22:27 PDT 2013
Hi Coleen,
Your changes look good.
Thanks, Harold
On 10/25/2013 3:05 PM, Coleen Phillimore wrote:
>
> See http://cr.openjdk.java.net/~coleenp/8024927_3/
> <http://cr.openjdk.java.net/%7Ecoleenp/8024927_3/> with this change. I
> think it looks great - thanks for the suggestion.
>
> Coleen
>
> 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?
>> 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