RFR: 8024927: Nashorn performance regression with CompressedOops
Coleen Phillimore
coleen.phillimore at oracle.com
Fri Oct 25 12:28:30 PDT 2013
On 10/25/2013 3:22 PM, harold seigel wrote:
> Hi Coleen,
>
> Your changes look good.
Thanks Harold, and thanks for the offline help.
Coleen
>
> 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