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

[coleen.phillimore at oracle.com]

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
>>>
>>
>