SA for ZGC
Stefan Karlsson
stefan.karlsson at oracle.com
Tue Jan 16 09:26:10 UTC 2018
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