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

[Erik Österlund]

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