RFR: 8223136: Move compressed oops functions to CompressedOops class

coleen.phillimore at oracle.com coleen.phillimore at oracle.com
Tue May 7 15:36:17 UTC 2019 


On 5/2/19 12:44 PM, Stefan Karlsson wrote:
> The big patch has been split up into smaller patches:
>  https://cr.openjdk.java.net/~stefank/8223136/webrev.03/01.compressed/

Not a small patch, but this looks great.  I really like the change to 
move compressed oops out of universe.
>  https://cr.openjdk.java.net/~stefank/8223136/webrev.03/02.verifyOption/

Good too.
>  https://cr.openjdk.java.net/~stefank/8223136/webrev.03/03.isGCActiveMark/ 
>

Ok.
>  https://cr.openjdk.java.net/~stefank/8223136/webrev.03/04.oopRecorder/

Ok.
>  https://cr.openjdk.java.net/~stefank/8223136/webrev.03/05.memAllocator/

Ok, seems unrelated though.

>  https://cr.openjdk.java.net/~stefank/8223136/webrev.03/06.oopFactory/

Not sure why you have this.  oopFactory.hpp still includes universe.hpp 
which I thought was the purpose of moving the function bodies to the cpp 
file.

>  https://cr.openjdk.java.net/~stefank/8223136/webrev.03/07.oopsHierarchy/

Seems unrelated but sure.   Can you make all of set_obj() in the cpp 
file?  For some reason gdb is stepping through this lately.

1201      oop mirror             = 
java_lang_reflect_Method::clazz(method_mirror);
(gdb) n
100      oopDesc* obj()  const volatile      { return _o; }
(gdb)
90      void raw_set_obj(const void* p)     { _o = (oopDesc*)p; }
(gdb)
97        if (CheckUnhandledOops) unregister_oop();
(gdb)
1202      int slot               = 
java_lang_reflect_Method::slot(method_mirror);

>
>  https://cr.openjdk.java.net/~stefank/8223136/webrev.03/08.fixIncludes/

Looks good.  universe.hpp is now not included by hpp files and 
redistributed to cpp files that need it.  Unsettling how many files need it.

Coleen
>
> All patches together:
>  https://cr.openjdk.java.net/~stefank/8223136/webrev.03/all/
>
> Thanks,
> StefanK
>
> On 2019-05-02 16:17, Stefan Karlsson wrote:
>> On 2019-05-02 16:12, coleen.phillimore at oracle.com wrote:
>>>
>>> Hi Stefan, This seems fine but I'd really like to have the #include 
>>> universe.hpp part of the patch broken out, as much as possible.
>>
>> OK. Thanks for the feedback. Let me work on that and get back to you 
>> with a few separate patches.
>>
>> Thanks,
>> StefanK
>>
>>>
>>> Thanks,
>>> Coleen
>>>
>>> On 5/2/19 8:44 AM, Stefan Karlsson wrote:
>>>> Hi all,
>>>>
>>>> Rebased on top of latest changes (jdk/jdk @ edd709e64ea1):
>>>>  http://cr.openjdk.java.net/~stefank/8223136/webrev.02/
>>>>
>>>> Thanks,
>>>> StefanK
>>>>
>>>>
>>>> On 2019-04-30 10:58, Stefan Karlsson wrote:
>>>>> Hi all,
>>>>>
>>>>> Please review this patch to move the rest of the compressed oops 
>>>>> functions and variables over to CompressedOops.
>>>>>
>>>>> http://cr.openjdk.java.net/~stefank/8223136/webrev.01
>>>>> https://bugs.openjdk.java.net/browse/JDK-8223136
>>>>>
>>>>> In 'JDK-8199946: Move load/store and encode/decode out of oopDesc' 
>>>>> the decoding and encoding of compressed oops were moved into the 
>>>>> namespace named CompressedOops.
>>>>>
>>>>> There infrastructure used by these functions were left in 
>>>>> Universe. This patch moves rest of the functions and variables 
>>>>> over to CompressedOops. I decided to convert the CompressedOops 
>>>>> namespace into a proper AllStatic class, to avoid any potential 
>>>>> problems with VMStructs.
>>>>>
>>>>> I also moved the corresponding functionality for compressed klass 
>>>>> pointers into a new class named CompressedKlassPointers.
>>>>>
>>>>> While doing this change I decided to cleanup the includes 
>>>>> regarding compressed oops, and universe.hpp, where most of the 
>>>>> code was moved from. Therefore, the patch is touching a large 
>>>>> number of files. A lot of the files contain updated header 
>>>>> includes and simple renames. If you want to fast-forward through 
>>>>> those, these are the main files where code related to compressed 
>>>>> oops actually moved:
>>>>>
>>>>>
>>>>> http://cr.openjdk.java.net/~stefank/8223136/webrev.01/src/hotspot/share/memory/universe.cpp.udiff.html 
>>>>>
>>>>>
>>>>> http://cr.openjdk.java.net/~stefank/8223136/webrev.01/src/hotspot/share/memory/universe.hpp.udiff.html 
>>>>>
>>>>>
>>>>> http://cr.openjdk.java.net/~stefank/8223136/webrev.01/src/hotspot/share/oops/compressedOops.hpp.html 
>>>>>
>>>>>
>>>>> http://cr.openjdk.java.net/~stefank/8223136/webrev.01/src/hotspot/share/oops/compressedOops.inline.hpp.udiff.html 
>>>>>
>>>>>
>>>>> http://cr.openjdk.java.net/~stefank/8223136/webrev.01/src/hotspot/share/oops/klass.cpp.udiff.html 
>>>>>
>>>>>
>>>>> http://cr.openjdk.java.net/~stefank/8223136/webrev.01/src/hotspot/share/oops/klass.hpp.udiff.html 
>>>>>
>>>>>
>>>>> http://cr.openjdk.java.net/~stefank/8223136/webrev.01/src/hotspot/share/oops/klass.inline.hpp.udiff.html 
>>>>>
>>>>>
>>>>> http://cr.openjdk.java.net/~stefank/8223136/webrev.01/src/hotspot/share/oops/oop.cpp.udiff.html 
>>>>>
>>>>>
>>>>> http://cr.openjdk.java.net/~stefank/8223136/webrev.01/src/hotspot/share/oops/oop.hpp.udiff.html 
>>>>>
>>>>>
>>>>> To get cleaner includes I did some minor restructuring to other 
>>>>> files. I can do these changes a small preparation patches, but 
>>>>> would like to get feedback on the overall patch first.
>>>>>
>>>>> Move flag checking out of hpp file, to stop propagation of 
>>>>> globals.hpp to all places where compressedOops.hpp is included:
>>>>>
>>>>>
>>>>> http://cr.openjdk.java.net/~stefank/8223136/webrev.01/src/hotspot/share/memory/oopFactory.cpp.udiff.html 
>>>>>
>>>>>
>>>>> http://cr.openjdk.java.net/~stefank/8223136/webrev.01/src/hotspot/share/memory/oopFactory.hpp.udiff.html 
>>>>>
>>>>>
>>>>> Extract VerifyOption out of universe.hpp:
>>>>>
>>>>>
>>>>> http://cr.openjdk.java.net/~stefank/8223136/webrev.01/src/hotspot/share/gc/shared/verifyOption.hpp.html 
>>>>>
>>>>>
>>>>> http://cr.openjdk.java.net/~stefank/8223136/webrev.01/src/hotspot/share/gc/shared/verifyOption.hpp.html 
>>>>>
>>>>>
>>>>> Moves a usage of CompressedOops to the cpp file:
>>>>>
>>>>>
>>>>> http://cr.openjdk.java.net/~stefank/8223136/webrev.01/src/hotspot/share/opto/matcher.cpp.udiff.html 
>>>>>
>>>>>
>>>>> http://cr.openjdk.java.net/~stefank/8223136/webrev.01/src/hotspot/share/opto/matcher.hpp.udiff.html 
>>>>>
>>>>>
>>>>> Move Universe usage out of hpp files
>>>>>
>>>>>
>>>>> http://cr.openjdk.java.net/~stefank/8223136/webrev.01/src/hotspot/share/gc/shared/memAllocator.hpp.udiff.html 
>>>>>
>>>>>
>>>>> http://cr.openjdk.java.net/~stefank/8223136/webrev.01/src/hotspot/share/gc/shared/memAllocator.cpp.udiff.html 
>>>>>
>>>>>
>>>>> http://cr.openjdk.java.net/~stefank/8223136/webrev.01/src/hotspot/share/gc/shared/isGCActiveMark.hpp.udiff.html 
>>>>>
>>>>> http://cr.openjdk.java.net/~stefank/8223136/webrev.01/src/hotspot/share/gc/shared/isGCActiveMark.cpp.html 
>>>>>
>>>>>
>>>>>
>>>>> http://cr.openjdk.java.net/~stefank/8223136/webrev.01/src/hotspot/share/memory/oopFactory.cpp.udiff.html 
>>>>>
>>>>>
>>>>> http://cr.openjdk.java.net/~stefank/8223136/webrev.01/src/hotspot/share/memory/oopFactory.hpp.udiff.html 
>>>>>
>>>>>
>>>>>
>>>>> http://cr.openjdk.java.net/~stefank/8223136/webrev.01/src/hotspot/share/code/oopRecorder.hpp.udiff.html 
>>>>>
>>>>>
>>>>> http://cr.openjdk.java.net/~stefank/8223136/webrev.01/src/hotspot/share/code/oopRecorder.inline.hpp.html 
>>>>>
>>>>>
>>>>> I've tried to update all platforms, but would appreciate 
>>>>> verification from platform maintainers that the changes don't 
>>>>> break the build.
>>>>>
>>>>> I've also left some of the old names in JVMCI, to not disturb 
>>>>> ongoing changes in that area.
>>>>>
>>>>> Thanks,
>>>>> StefanK
>>>
>>
>

More information about the hotspot-dev mailing list