RFR: 8199946: Move load/store and encode/decode out of oopDesc

[Stefan Karlsson]

Hi Thomas,

On 2018-03-23 16:50, Thomas Schatzl wrote:
> Hi,
> 
>    looks good with minor caveats:
> 
> There is another at
> psScavenge.inline.hpp
>   111       RawAccess<>::oop_store(p, new_obj);

I'll fix.

> 
> and
> metaspaceShared.cpp
> 1940       RootAccess<IN_ARCHIVE_ROOT>::oop_store(p,
> CompressedOops::decode(o));
> 
> has been a decode_heap_oop_not_null()


I'll fix.

> 
> ------
> 
> Depending on whether you think this fits this change, it would be nice
> to fix some code in the following locations. Otherwise I can do a
> separate change.
> 
> g1OopClosures.inline.hpp:
>   244     RawAccess<>::oop_store(p, forwardee);
> 
> This could be a RawAccess<OOP_NOT_NULL>::oop_store()
> 
> g1ParScanThreadState.inline.hpp:
>    49     RawAccess<>::oop_store(p, obj);
> 
> Same here.

I had changes to do this, but I decided to revert those changes for this 
patch. A new RFE for this would be good.

Thanks,
StefanK

> 
> Thanks,
>    Thomas
> 
> 
> 
> On Fri, 2018-03-23 at 13:27 +0100, Stefan Karlsson wrote:
>> On 2018-03-23 13:28, Erik Österlund wrote:
>>> Hi Stefan,
>>>
>>> Looks good. There is one remaining OOP_NOT_NULL in
>>> psScavenge.inline.hpp, but I do not need another webrev.
>>
>>
>> Fixed. Thanks for the review!
>>
>> StefanK
>>
>>>
>>> Thanks,
>>> /Erik
>>>
>>> On 2018-03-22 20:28, Stefan Karlsson wrote:
>>>> Erik found two places where I didn't use OOP_NOT_NULL, where we
>>>> previously used _not_null functions.
>>>>
>>>> diff --git a/src/hotspot/share/gc/cms/parNewGeneration.cpp
>>>> b/src/hotspot/share/gc/cms/parNewGeneration.cpp
>>>> --- a/src/hotspot/share/gc/cms/parNewGeneration.cpp
>>>> +++ b/src/hotspot/share/gc/cms/parNewGeneration.cpp
>>>> @@ -734,7 +734,7 @@
>>>>         oop new_obj = obj->is_forwarded()
>>>>                         ? obj->forwardee()
>>>>                         :
>>>> _g->DefNewGeneration::copy_to_survivor_space(obj);
>>>> -      RawAccess<>::oop_store(p, new_obj);
>>>> +      RawAccess<OOP_NOT_NULL>::oop_store(p, new_obj);
>>>>       }
>>>>       if (_gc_barrier) {
>>>>         // If p points to a younger generation, mark the card.
>>>> diff --git a/src/hotspot/share/gc/cms/parOopClosures.inline.hpp
>>>> b/src/hotspot/share/gc/cms/parOopClosures.inline.hpp
>>>> --- a/src/hotspot/share/gc/cms/parOopClosures.inline.hpp
>>>> +++ b/src/hotspot/share/gc/cms/parOopClosures.inline.hpp
>>>> @@ -52,7 +52,7 @@
>>>>         new_obj =
>>>> ((ParNewGeneration*)_g)->copy_to_survivor_space(_par_scan_state,
>>>> obj, obj_sz, m);
>>>>       }
>>>> -    RawAccess<>::oop_store(p, new_obj);
>>>> +    RawAccess<OOP_NOT_NULL>::oop_store(p, new_obj);
>>>>     }
>>>>   }
>>>>
>>>> StefanK
>>>>
>>>> On 2018-03-22 17:01, Stefan Karlsson wrote:
>>>>> Hi,
>>>>>
>>>>> This patch needs Erik's change to the LoadProxies:
>>>>> http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2018-Marc
>>>>> h/021504.html
>>>>>
>>>>>
>>>>> to build on fastdebug.
>>>>>
>>>>> Here's a rebased patch:
>>>>> http://cr.openjdk.java.net/~stefank/8199946/webrev.02/
>>>>>
>>>>> Thanks,
>>>>> StefanK
>>>>>
>>>>> On 2018-03-21 18:27, Stefan Karlsson wrote:
>>>>>> Hi all,
>>>>>>
>>>>>> Please review this patch to get rid of the
>>>>>> oopDesc::load/store
>>>>>> functions and to move the oopDesc::encode/decode functions to
>>>>>> a new
>>>>>> CompressedOops subsystem.
>>>>>>
>>>>>> http://cr.openjdk.java.net/~stefank/8199946/webrev.01
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8199946
>>>>>>
>>>>>> When the Access API was introduced many of the usages of
>>>>>> oopDesc::load_decode_heap_oop, and friends, were replaced by
>>>>>> calls
>>>>>> to the Access API. However, there are still some usages of
>>>>>> these
>>>>>> functions, most notably in the GC code.
>>>>>>
>>>>>> This patch is two-fold:
>>>>>>
>>>>>> 1) It replaces the oopDesc load and store calls with
>>>>>> RawAccess
>>>>>> equivalents.
>>>>>>
>>>>>> 2) It moves the oopDesc encode and decode functions to a
>>>>>> new,
>>>>>> separate, subsystem called CompressedOops. A future patch
>>>>>> could even
>>>>>> move all the Universe::_narrow_oop variables over to
>>>>>> CompressedOops.
>>>>>>
>>>>>> The second part has the nice property that it breaks up a
>>>>>> circular
>>>>>> dependency between oop.inline.hpp and access.inline.hpp.
>>>>>> After the
>>>>>> change we have:
>>>>>>
>>>>>> oop.inline.hpp includes:
>>>>>>     access.inline.hpp
>>>>>>     compressedOops.inline.hpp
>>>>>>
>>>>>> access.inline.hpp includes:
>>>>>>     compressedOops.inline.hpp
>>>>>>
>>>>>> Thanks,
>>>>>> StefanK
>>>>
>>>>
>