SA for ZGC
Yasumasa Suenaga
yasuenag at gmail.com
Wed Jan 17 02:51:30 UTC 2018
Hi Stefan,
I uploaded new webrev:
http://cr.openjdk.java.net/~ysuenaga/z/sa-universe/webrev.03/
I removed dependencies to Linux specific code in it.
Thanks,
Yasumasa
2018-01-16 20:00 GMT+09:00 Yasumasa Suenaga <yasuenag at gmail.com>:
>> I checked this patch with -XX:+ZUnmapBadViews on my Linux x64, it works fine.
>
> Sorry, I forgot to run GC.run jcmd. I had a error.
> I will look it later.
>
>
> Yasumasa
>
>
> 2018-01-16 19:57 GMT+09:00 Yasumasa Suenaga <yasuenag at gmail.com>:
>> Hi Stefan,
>>
>> I've restored your change and use VM.getOS() and getCPU() in new webrev:
>> http://cr.openjdk.java.net/~ysuenaga/z/sa-universe/webrev.02/
>>
>> I checked this patch with -XX:+ZUnmapBadViews on my Linux x64, it works fine.
>>
>>
>> Thanks,
>>
>> Yasumasa
>>
>>
>>
>>
>> 2018-01-16 18:26 GMT+09:00 Stefan Karlsson <stefan.karlsson at oracle.com>:
>>> Hi Yasumas,
>>>
>>> On 2018-01-16 08:14, Yasumasa Suenaga wrote:
>>>>
>>>> Hi Stefan,
>>>>
>>>>>> Stefan's patch seems to have resolved JDK-8194737.
>>>>>> Should we resolve this JDK-8194737 in upstream repo and merge it to zgc
>>>>>> at
>>>>>> first?
>>>>>
>>>>>
>>>>>
>>>>> I'll let Jini handle that bug.
>>>>
>>>>
>>>> Thanks!
>>>> I watch this bug.
>>>>
>>>>
>>>>> On SPARC we add a heap base when converting from "offsets" to "objects".
>>>>> See:
>>>>>
>>>>> http://hg.openjdk.java.net/zgc/zgc/file/89d447cff6c5/src/hotspot/os_cpu/solaris_sparc/zAddress_solaris_sparc.inline.hpp
>>>>
>>>>
>>>> I think we can use getOS() and getCPU() in VM class:
>>>>
>>>> http://hg.openjdk.java.net/zgc/zgc/file/89d447cff6c5/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/runtime/VM.java#l511
>>>>
>>>>
>>>>> My first version of of ZCollectedHeap.oop_load_at passed an OopHandle to
>>>>> ZBarrier.weak_barrier, but I changed that, so my implementation of
>>>>> ZOop.to_address isn't needed there. However, it is still needed for
>>>>> ZCollectedHeap.oopAddressDescription:
>>>>>
>>>>> public String oopAddressDescription(OopHandle handle) {
>>>>> Address origOop = ZOop.to_address(handle);
>>>>> Address loadBarrieredOop = ZBarrier.weak_barrier(origOop);
>>>>>
>>>>> We either need to restore the previous implementation, or we need to fix
>>>>> oopAddressDescription.
>>>>
>>>>
>>>> OK, I will keep your original ZOop.java and related implementation.
>>>
>>>
>>> OK. We can revisit this later if we do decide to clean this up.
>>>
>>>>
>>>>
>>>>>> I added these values to constant values in VMStructs.
>>>>>> Thus I removed ZGlobalsForVMStructs.
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> This is a bug, and causes my test to fail.
>>>>>
>>>>> The constants values in VMStructs are for constants in the C++ code. Some
>>>>> of
>>>>> the values you changed are initialized during the VM initializations, and
>>>>> some are changed throughout the run of the VM.
>>>>
>>>>
>>>> I guess we can use HotSpotVMAddress as JVMCI:
>>>>
>>>> http://hg.openjdk.java.net/zgc/zgc/file/89d447cff6c5/src/hotspot/share/jvmci/vmStructs_jvmci.cpp#l937
>>>>
>>>> However this symbol is not exported.
>>>> We need to add `gHotSpotVMAddress` to vmStructs.hpp .
>>>>
>>>> BTW, do ZGC team have a plan to refactor zGlobals.hpp?
>>>
>>>
>>>
>>> Currently, we don't have any plans to do that.
>>>
>>>> AFAICS HotSpot does not have global variables (not `const` value).
>>>> IMHO we can refactor zGlobals to the C++ class which extends AllStatic.
>>>> We can use VMStructs directly if we do so.
>>>
>>>
>>> HotSpot actually does have global variables. You'll find some of them in
>>> globalDefinitions.hpp. For example: heapOopSize, MinObjAlignment, and
>>> OopEncodingHeapMax.
>>>
>>> The SA seems to duplicate the calculations of these values instead of
>>> reading the actual values from the process/core.
>>>
>>> I'm a bit reluctant to change the ZGC code style just to adapt to the SA
>>> agent. Currently, I think it's fine to have an extra layer in vmStructs_z
>>> until the SA agent gets support for global variables.
>>>
>>> Thanks,
>>> StefanK
>>>
>>>
>>>>
>>>>
>>>>> Try to run with -XX:+ZUnmapBadViews. You will find any stray pointers
>>>>> faster
>>>>> with that flag.
>>>>
>>>>
>>>> Thanks!
>>>> After all concerns in above is resolved, I will test it.
>>>>
>>>>
>>>> Yasumasa
>>>>
>>>>
>>>> 2018-01-15 17:48 GMT+09:00 Stefan Karlsson <stefan.karlsson at oracle.com>:
>>>>>
>>>>> Hi Yasumasa,
>>>>>
>>>>> On 2018-01-13 04:39, Yasumasa Suenaga wrote:
>>>>>>
>>>>>>
>>>>>> Hi Stefan, Per,
>>>>>>
>>>>>> Thank you for your comments and patches!
>>>>>> I uploaded new webrev:
>>>>>>
>>>>>> all:
>>>>>> http://cr.openjdk.java.net/~ysuenaga/z/sa-universe/webrev.01/all/
>>>>>>
>>>>>> diff from Stefan's patch:
>>>>>> http://cr.openjdk.java.net/~ysuenaga/z/sa-universe/webrev.01/diff/
>>>>>>
>>>>>>
>>>>>> Stefan's patch seems to have resolved JDK-8194737.
>>>>>> Should we resolve this JDK-8194737 in upstream repo and merge it to zgc
>>>>>> at
>>>>>> first?
>>>>>
>>>>>
>>>>>
>>>>> I'll let Jini handle that bug.
>>>>>
>>>>>>
>>>>>>
>>>>>>> Just one comment, in ZPage.java:
>>>>>>>
>>>>>>> 91 // There's no relocate operation in the SA. Simply use
>>>>>>> the
>>>>>>> from address and hope for the best.
>>>>>>> 92 return from;
>>>>>>>
>>>>>>> we should return ZAddress.good(from) here. And then we can remove the
>>>>>>> last part of the comment. Maybe say something about in-place forwarding
>>>>>>> instead.
>>>>>>
>>>>>>
>>>>>>
>>>>>> I've fixed.
>>>>>>
>>>>>>
>>>>>>>> 1) Some of the constants are Linux-specific and there's no platform
>>>>>>>> abstraction in place.
>>>>>>
>>>>>>
>>>>>>
>>>>>> I added some constant values to vmStructs_z.hpp . So we can remove
>>>>>> Linux-specific values from SA Java sources.
>>>>>
>>>>>
>>>>>
>>>>> OK. Sounds like the right thing to do for true constants. More about this
>>>>> later.
>>>>>
>>>>>> But I've not remove address() from ZAddress.java because I'm not sure
>>>>>> why
>>>>>> this method is needed.
>>>>>
>>>>>
>>>>>
>>>>> On SPARC we add a heap base when converting from "offsets" to "objects".
>>>>> See:
>>>>>
>>>>> http://hg.openjdk.java.net/zgc/zgc/file/89d447cff6c5/src/hotspot/os_cpu/solaris_sparc/zAddress_solaris_sparc.inline.hpp
>>>>>
>>>>>
>>>>>>
>>>>>>
>>>>>>>> 2) ZOop.to_address subverts the type system so that we can get from
>>>>>>>> OopHandles and longs to Addresses. This seems to be frowned upon if
>>>>>>>> you read
>>>>>>>> the SA code. Maybe there are more correct ways to implement this?
>>>>>>
>>>>>>
>>>>>>
>>>>>> I changed ZOop#to_address() which have one argument which type is
>>>>>> Address.
>>>>>> I guess this change is type-safe.
>>>>>
>>>>>
>>>>>
>>>>> My version of ZOop.to_address converted a OopHandle into an non-OopHandle
>>>>> Address. Without it you get an UnsupportedOperationException when you try
>>>>> to
>>>>> do bit-manipulation on the OopHandle. From LinuxOopHandle:
>>>>> public Address addOffsetTo (long offset) throws
>>>>> UnsupportedOperationException {
>>>>> throw new UnsupportedOperationException("addOffsetTo not applicable
>>>>> to
>>>>> OopHandles (interior object pointers not allowed)");
>>>>> }
>>>>>
>>>>>
>>>>> My first version of of ZCollectedHeap.oop_load_at passed an OopHandle to
>>>>> ZBarrier.weak_barrier, but I changed that, so my implementation of
>>>>> ZOop.to_address isn't needed there. However, it is still needed for
>>>>> ZCollectedHeap.oopAddressDescription:
>>>>>
>>>>> public String oopAddressDescription(OopHandle handle) {
>>>>> Address origOop = ZOop.to_address(handle);
>>>>> Address loadBarrieredOop = ZBarrier.weak_barrier(origOop);
>>>>>
>>>>> We either need to restore the previous implementation, or we need to fix
>>>>> oopAddressDescription.
>>>>>
>>>>> My version of ZOop.to_address performed bit manipulations just to ensure
>>>>> that we never got a all-zero value. You have left the
>>>>> andWithMask(REMOVE_MSB_MASK), which isn't necessary when you didn't add
>>>>> the
>>>>> bit in the first place.
>>>>>
>>>>>
>>>>>>
>>>>>>
>>>>>>>> 4) I see that you used VMObjectFactory.newObject, I skipped that part,
>>>>>>>> but we can think about unifying our patches.
>>>>>>
>>>>>>
>>>>>>
>>>>>> I think the classes which extends VMObject should be instantiated via
>>>>>> VMObjectFactory#newObject() because other SA codes seems to be so.
>>>>>> I changed to use this factory method as possible.
>>>>>
>>>>>
>>>>>
>>>>> OK.
>>>>>
>>>>>>
>>>>>>
>>>>>>>> 5) I had to invent a way to read global variables from HotSpot. See
>>>>>>>> the
>>>>>>>> usage of ZGlobalsForVMStructs. Maybe there's a more correct way to do
>>>>>>>> this?
>>>>>>
>>>>>>
>>>>>>
>>>>>> I added these values to constant values in VMStructs.
>>>>>> Thus I removed ZGlobalsForVMStructs.
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> This is a bug, and causes my test to fail.
>>>>>
>>>>> The constants values in VMStructs are for constants in the C++ code. Some
>>>>> of
>>>>> the values you changed are initialized during the VM initializations, and
>>>>> some are changed throughout the run of the VM.
>>>>>
>>>>>>
>>>>>>
>>>>>>>> I stress tested the thread dumping part that threw exceptions for me
>>>>>>>> earlier by running SPECjbb2005:
>>>>>>>> $ java -XX:+UseZGC -Xlog:gc -cp jbb.jar:check.jar spec.jbb.JBBmain
>>>>>>>>
>>>>>>>> and then continuously dumping the threads:
>>>>>>>> $ while true; do echo -e "verbose true\nthreads" | jhsdb -J-ea clhsdb
>>>>>>>> --pid=`jps | grep JBBmain | awk '{print $1}'`; done
>>>>>>
>>>>>>
>>>>>>
>>>>>> Sorry, I do not have SPECjbb.
>>>>>> Can you share the result?
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> You can use any program that starts a number of threads an continuously
>>>>> run
>>>>> GCs.
>>>>>
>>>>>>
>>>>>> I tested `universe` and `jstack` and `jstack -v` on CLHSDB with simple
>>>>>> Java app.
>>>>>> This change works fine on my Linux x64 box.
>>>>>
>>>>>
>>>>>
>>>>> Try to run with -XX:+ZUnmapBadViews. You will find any stray pointers
>>>>> faster
>>>>> with that flag.
>>>>>
>>>>> This reproduces the problem:
>>>>>
>>>>> public class HelloSleep {
>>>>> public static void main(String... args) throws Exception {
>>>>> Thread.sleep(100000000000000L);
>>>>> }
>>>>> }
>>>>>
>>>>> DIR=~/hg/z/build/release/jdk
>>>>> $DIR/bin/java -XX:+ZUnmapBadViews -XX:+UseZGC HelloSleep &
>>>>> $DIR/bin/jcmd $! GC.run
>>>>> echo -e "verbose true\nthreads" | $DIR/bin/jhsdb -J-ea hsdb --pid=$
>>>>>
>>>>> Thanks,
>>>>> StefanK
>>>>>
>>>>>
>>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> Yasumasa
>>>>>>
>>>>>>
>>>>>> On 2018/01/11 21:40, Per Liden wrote:
>>>>>>>
>>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>> On 01/10/2018 05:55 PM, Stefan Karlsson wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> Hi Yasumasa,
>>>>>>>>
>>>>>>>> Here's my first stab at adding the ZGC load barries to the SA:
>>>>>>>>
>>>>>>>> http://cr.openjdk.java.net/~stefank/zgc/zSALoadBarriers/webrev.01/
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Just one comment, in ZPage.java:
>>>>>>>
>>>>>>> 91 // There's no relocate operation in the SA. Simply use
>>>>>>> the
>>>>>>> from address and hope for the best.
>>>>>>> 92 return from;
>>>>>>>
>>>>>>> we should return ZAddress.good(from) here. And then we can remove the
>>>>>>> last part of the comment. Maybe say something about in-place forwarding
>>>>>>> instead.
>>>>>>>
>>>>>>> cheers,
>>>>>>> Per
>>>>>>>
>>>>>>>>
>>>>>>>> I find it unfortunate that we have to duplicate the C++ code into
>>>>>>>> Java,
>>>>>>>> to get this to work. But it is what it is.
>>>>>>>>
>>>>>>>> Some comments about the code:
>>>>>>>>
>>>>>>>> 1) Some of the constants are Linux-specific and there's no platform
>>>>>>>> abstraction in place.
>>>>>>>>
>>>>>>>> 2) ZOop.to_address subverts the type system so that we can get from
>>>>>>>> OopHandles and longs to Addresses. This seems to be frowned upon if
>>>>>>>> you read
>>>>>>>> the SA code. Maybe there are more correct ways to implement this?
>>>>>>>>
>>>>>>>> 3) I use the ZGC weak barrier, since the SA agent only needs to be
>>>>>>>> able
>>>>>>>> to access the Objects, not mark them. Which is what the weak barriers
>>>>>>>> provide.
>>>>>>>>
>>>>>>>> 4) I see that you used VMObjectFactory.newObject, I skipped that part,
>>>>>>>> but we can think about unifying our patches.
>>>>>>>>
>>>>>>>> 5) I had to invent a way to read global variables from HotSpot. See
>>>>>>>> the
>>>>>>>> usage of ZGlobalsForVMStructs. Maybe there's a more correct way to do
>>>>>>>> this?
>>>>>>>>
>>>>>>>> I stress tested the thread dumping part that threw exceptions for me
>>>>>>>> earlier by running SPECjbb2005:
>>>>>>>> $ java -XX:+UseZGC -Xlog:gc -cp jbb.jar:check.jar spec.jbb.JBBmain
>>>>>>>>
>>>>>>>> and then continuously dumping the threads:
>>>>>>>> $ while true; do echo -e "verbose true\nthreads" | jhsdb -J-ea clhsdb
>>>>>>>> --pid=`jps | grep JBBmain | awk '{print $1}'`; done
>>>>>>>>
>>>>>>>>
>>>>>>>> Cheers,
>>>>>>>> StefanK
>>>>>>>>
>>>>>>>> On 2018-01-09 13:59, Yasumasa Suenaga wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Hi Stefan,
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> Let me try to create an initial prototype for load barriers in the
>>>>>>>>>> SA
>>>>>>>>>> case and see where this takes us.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Sure!
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> FYI i also found that compressed oops are not fully supported in the
>>>>>>>>>> SA:
>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8194737
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Thanks, I didn't know that.
>>>>>>>>> I hope this issue will be fixed soon :-)
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Yasumasa
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 2018/01/09 18:36, Stefan Karlsson wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Hi Yasumasa,
>>>>>>>>>>
>>>>>>>>>> Thanks for your contribution!
>>>>>>>>>>
>>>>>>>>>> I tested your patch together with jhsdb hsdb and immediately hit
>>>>>>>>>> problems because of the lack of load barriers in the SA code. Let me
>>>>>>>>>> try to
>>>>>>>>>> create an initial prototype for load barriers in the SA case and see
>>>>>>>>>> where
>>>>>>>>>> this takes us.
>>>>>>>>>>
>>>>>>>>>> FYI i also found that compressed oops are not fully supported in the
>>>>>>>>>> SA:
>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8194737
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>> StefanK
>>>>>>>>>>
>>>>>>>>>> On 2017-12-21 23:38, Yasumasa Suenaga wrote:
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Thanks Per!
>>>>>>>>>>>
>>>>>>>>>>> I'm waiting for comments and sponsorship.
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Yasumasa
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On 2017/12/22 4:33, Per Liden wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> Hi Yasumasa,
>>>>>>>>>>>>
>>>>>>>>>>>> On 2017-12-21 09:40, Yasumasa Suenaga wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> Hi all,
>>>>>>>>>>>>>
>>>>>>>>>>>>> I checked SA implementation for ZGC, but it is not yet available.
>>>>>>>>>>>>> For example, we can see WrongTypeException when CLHSDB `universe`
>>>>>>>>>>>>> command is executed.
>>>>>>>>>>>>>
>>>>>>>>>>>>> As the first step, I propose to implement ZCollectedHeap and
>>>>>>>>>>>>> related classes for
>>>>>>>>>>>>> SA as this webrev:
>>>>>>>>>>>>>
>>>>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/z/sa-universe/
>>>>>>>>>>>>>
>>>>>>>>>>>>> After applying this patch, we can use `universe` command on
>>>>>>>>>>>>> CLHSDB.
>>>>>>>>>>>>> (I followed `VM.info` jcmd output format)
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> Thanks! Most of us working on ZGC are currently on vacation, but
>>>>>>>>>>>> we'll have a closer look at the patch after the holidays.
>>>>>>>>>>>>
>>>>>>>>>>>> cheers,
>>>>>>>>>>>> Per
>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> Of course, it is not all. We need to work more for it.
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>
>>>>>>>>>>>>> Yasumasa
>>>>>>>>>>>>>
>>>>>
>>>
More information about the zgc-dev
mailing list