RFR: 8223136: Move compressed oops functions to CompressedOops class
coleen.phillimore at oracle.com
coleen.phillimore at oracle.com
Tue May 7 18:27:57 UTC 2019
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