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

Wed Apr 29 17:15:02 UTC 2020

Looks good to me.
My comment about testing was related to the follow-up patch to remove hard coded
offsets, but Coleen told me we have good coverage about the different initialization
paths.

Thank you,

Fred

> On Apr 28, 2020, at 19:19, coleen.phillimore at oracle.com wrote:
> 
> 
> 
> 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
>>>>>>>> 
>>>>>>> 
>>>>>> 
>>>>> 
>>> 
> 