RFR: 8223136: Move compressed oops functions to CompressedOops class
Stefan Karlsson
stefan.karlsson at oracle.com
Tue May 7 19:28:05 UTC 2019
These are the latest patches (on jdk/jdk @ 61049e91eae5):
http://cr.openjdk.java.net/~stefank/8223136/webrev.04/01.compressed/
http://cr.openjdk.java.net/~stefank/8223136/webrev.04/02.verifyOption/
http://cr.openjdk.java.net/~stefank/8223136/webrev.04/03.isGCActiveMark/
http://cr.openjdk.java.net/~stefank/8223136/webrev.04/04.oopRecorder/
http://cr.openjdk.java.net/~stefank/8223136/webrev.04/05.memAllocator/
http://cr.openjdk.java.net/~stefank/8223136/webrev.04/06.oopFactory/
http://cr.openjdk.java.net/~stefank/8223136/webrev.04/07.fixIncludes/
http://cr.openjdk.java.net/~stefank/8223136/webrev.04/all/
I folded the oopsHierarchy change below into the fixIncludes patch. I
also found that I hadn't updated all ppc files, so that's fixed as well.
This compiles locally on Linux x64. I'll also test that it still works
on the platforms we build on.
Thanks,
StefanK
On 2019-05-07 20:27, coleen.phillimore at oracle.com wrote:
>
>
> On 5/7/19 2:02 PM, Stefan Karlsson wrote:
>> 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.
>
> Ok, I see.
>>
>>>
>>>> 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;
>>
>
> Not ideal, but ok. What you had might not hurt performance of
> fastdebug, but you'd have to measure it. This can be done some other
> time.
>>
>>> 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 ...
>
> I agree.
>
> thanks for doing this cleanup.
> Coleen
>>
>> 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