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

Tue Apr 28 23:19:54 UTC 2020

On 4/28/20 4:30 PM, Erik Österlund wrote:
> Hi Coleen,
>
> Unfortunately I do need that change already now. Because making 
> compressed oops and compressed class pointers independent involves 
> changing the instanceOop base with compressed oop off and compressed 
> class pointers off. That changes the layout and offsets that are 
> hardcoded (primitive wrappers that wrap a primitive less than 4 bytes 
> now start in the klass gap).

Okay, I didn't see how it related from the diffs.  Now I get it. Your 
patch is good then.  Thanks for removing the hardcoded offsets next.

Coleen

>
> My proposed solution is to remove hardcoded layouts in this patch, and 
> remove hardcoded offsets in the next one. The alternative would be 
> trying to fix the hardcoded layouts only to remove them one patch 
> later. I tried to fix them but ran into issues; that is why I removed 
> them instead in this patch. I hope you think this is okay. The net 
> result will be the same one patch later.
>
> Thanks,
> /Erik
>
>> On 28 Apr 2020, at 21:56, coleen.phillimore at oracle.com wrote:
>>
>> 
>> 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
>>>>>>>
>>>>>>
>>>>>
>>>>
>>