RFR: 8241825: Make compressed oops and compressed class pointers independent on x86_64 (closed)

[coleen.phillimore at oracle.com]

Hi Erik,

I was trying to figure out why this change is needed to decouple 
CompressedOops and CompressedKlassPointers, but it's not needed for the 
change, is it?

http://cr.openjdk.java.net/~eosterlund/8241825/webrev.01/src/hotspot/share/classfile/fieldLayoutBuilder.cpp.udiff.html

Can you make this change with your patch for 
https://bugs.openjdk.java.net/browse/JDK-8243996 which we really like?

Thanks,
Coleen


On 4/28/20 1:52 PM, Erik Österlund wrote:
> Hi Coleen,
>
> Oops, typo in the incremental URL. Here it is:
>
> http://cr.openjdk.java.net/~eosterlund/8241825/webrev.00..01/
>
> Thanks for the review.
>
> /Erik
>
>> On 28 Apr 2020, at 19:25, coleen.phillimore at oracle.com wrote:
>>
>>  Okay.  Your incremental is 4040 but looks good to me.
>> Thanks,
>> Coleen
>>
>> On 4/28/20 12:30 PM, Erik Österlund wrote:
>>> Hi Coleen,
>>>
>>> Thanks for the review.
>>>
>>> Here is a new webrev:
>>> http://cr.openjdk.java.net/~eosterlund/8241825/webrev.01/
>>>
>>> Incremental:
>>> http://cr.openjdk.java.net/~eosterlund/8241825/webrev.00_01/
>>>
>>> On 2020-04-28 16:15, coleen.phillimore at oracle.com wrote:
>>>> Sorry, this went to the wrong list.
>>>>
>>>> Coleen
>>>>
>>>> On 4/24/20 11:17 AM, coleen.phillimore at oracle.comwrote:
>>>>>
>>>>> Hi Erik, well the interpreter code isn't as bad as I thought it 
>>>>> would be.  We tied UseCompressedKlassPointers to UseCompressedOops 
>>>>> because we couldn't imagine why someone would care about 4 bytes 
>>>>> per object with all 8 byte pointers to oops.  But maybe you do care.
>>>>>
>>>>> http://cr.openjdk.java.net/~eosterlund/8241825/webrev.00/src/hotspot/cpu/x86/interp_masm_x86.cpp.udiff.html
>>>>>
>>>>>        if (UseBiasedLocking) {
>>>>> - biased_locking_enter(lock_reg, obj_reg, swap_reg, tmp_reg, 
>>>>> false, done, &slow_case);
>>>>> + Register rklass_decode_tmp = LP64_ONLY(rscratch1) NOT_LP64(noreg);
>>>>> + biased_locking_enter(lock_reg, obj_reg, swap_reg, tmp_reg, 
>>>>> rscratch1, false, done, &slow_case);
>>>>>        }
>>>>> You didn't use rklass_decode_tmp here.
>>>
>>> Nice catch. Fixed.
>>>
>>>>> http://cr.openjdk.java.net/~eosterlund/8241825/webrev.00/src/hotspot/cpu/x86/templateTable_x86.cpp.udiff.html
>>>>>
>>>>>
>>>>> + Register tmp_load_klass = LP64_ONLY(rscratch1) NOT_LP64(noreg);
>>>>>
>>>>> I wonder if you couldn't just pass rscratch1 into load_klass and 
>>>>> handle ignoring it inside the load_klass function, ie. fix the 
>>>>> asserts.  This gets repetitive in the templateTable and other 
>>>>> code.  Maybe people may have the opposite opinion though.
>>>
>>> Unfortunately, that won't compile in code that is shared between 32 
>>> bit and 64 bit. In 32 bit-only code I just pass in noreg, and in 
>>> 64-bit only code I just send in the temp. But for shared code, I 
>>> have to do the macro dance, because rscratch1 does not exist on 32 bit.
>>>
>>>>> This is quite nice that UseCompressedKlassPointers doesn't have to 
>>>>> reinit_heapbase.  The interpreter code looks good.
>>>
>>> Thanks!
>>>
>>>>> http://cr.openjdk.java.net/~eosterlund/8241825/webrev.00/src/hotspot/share/memory/metaspace.cpp.frames.html
>>>>>
>>>>> I don't know if base = HeapBaseMinAddress is right for the 
>>>>> compressed class space.  It's reserved but not committed as 1G.  
>>>>> Maybe it makes sense since the heap without UseCompressedOops 
>>>>> should be above the class space.
>>>
>>> Thomas had a similar comment. I think the summary is that yes it is 
>>> the right thing to use, but the naming is unfortunate.
>>>
>>> Hope I fixed it all.
>>>
>>> Thanks,
>>> /Erik
>>>
>>>>> Coleen
>>>>>
>>>>> On 4/24/20 4:25 AM, Erik Österlund wrote:
>>>>>> Hi,
>>>>>>
>>>>>> This is the closed version of the mostly open change. There are 
>>>>>> some @Contended tests in closed
>>>>>> that needed some love with my change to make compressed class 
>>>>>> pointers usable with compressed
>>>>>> oops disabled. In particular, these tests perform calculations to 
>>>>>> ensure that things are padded
>>>>>> as expected. Part of those calculations makes assumptions about 
>>>>>> how large the object header is,
>>>>>> presuming that the number is a constant that is the same for all 
>>>>>> objects, and is determinable
>>>>>> by checking the offset of the first field. However, with my 
>>>>>> changes, the first field will end
>>>>>> up either in the klass gap, or after, depending on the field 
>>>>>> size. I updated the tests to consider
>>>>>> different header sizes depending on the type of fields being 
>>>>>> used. It still isn't perfect, but
>>>>>> it is clever enough for the purposes of the tests; they pass with 
>>>>>> these changes.
>>>>>>
>>>>>> Bug:
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8241825
>>>>>>
>>>>>> Webrev:
>>>>>> http://wikifiles.se.oracle.com/dev/eosterlund/8241825/webrev.00/
>>>>>>
>>>>>> Thanks,
>>>>>> /Erik
>>>>>
>>>>
>>>
>>