SA for ZGC
Yasumasa Suenaga
yasuenag at gmail.com
Tue Jan 16 07:14:48 UTC 2018
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.
>> 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?
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.
> 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