SA for ZGC

Yasumasa Suenaga yasuenag at gmail.com
Tue Jan 16 11:00:22 UTC 2018


> 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