RFR: 8223136: Move compressed oops functions to CompressedOops class
Stefan Karlsson
stefan.karlsson at oracle.com
Tue May 7 18:02:15 UTC 2019
Thanks for reviewing!
Inlined:
On 2019-05-07 17:36, coleen.phillimore at oracle.com wrote:
>
>
> 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.
It moves the usage of Universe::heap() to the cpp file. So, unrelated to
the CompressedOops patch, but related to the universe.hpp include cleanups.
>
>> 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.
When splitting the big patch into smaller patches I didn't want to mess
around with the includes, and I deferred the removal of the include to
the last patch. This way there's less risk that these smaller changes
cause any compilation breakages.
>
>> https://cr.openjdk.java.net/~stefank/8223136/webrev.03/07.oopsHierarchy/
>>
>
> Seems unrelated but sure.
I added oopsHierarchy.hpp to one of the header files and didn't want
globals.hpp to spread through to other header files. To minimize this
change, I propose that we instead do the following:
diff --git a/src/hotspot/share/oops/oopsHierarchy.hpp
b/src/hotspot/share/oops/oopsHierarchy.hpp
--- a/src/hotspot/share/oops/oopsHierarchy.hpp
+++ b/src/hotspot/share/oops/oopsHierarchy.hpp
@@ -27,7 +27,6 @@
#include "metaprogramming/integralConstant.hpp"
#include "metaprogramming/primitiveConversions.hpp"
-#include "runtime/globals.hpp"
#include "utilities/globalDefinitions.hpp"
// OBJECT hierarchy
@@ -75,6 +74,8 @@
class PromotedObject;
class oopDesc;
+extern bool CheckUnhandledOops;
+
class oop {
oopDesc* _o;
> 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);
I talked to Coleen about this, and we agreed to leave this for now.
>
>>
>> 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.
Right. There might be further opportunities to split universe.hpp. One
common pattern that requires univese.hpp is
Universe::heap()->some_function(). All those places require includes of
both universe.hpp and collectedHeap.hpp. There might be a clean way to
change that, so that we only need one include ...
Thanks.
>
> 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