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

[Erik Österlund]

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.com wrote:
>>>> 
>>>> 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 
>>>> 
>>> 
>> 
>