SA for ZGC
Stefan Karlsson
stefan.karlsson at oracle.com
Wed Jan 17 12:57:55 UTC 2018
Hi Yasumasa,
On 2018-01-17 03:51, Yasumasa Suenaga wrote:
> Hi Stefan,
>
> I uploaded new webrev:
> http://cr.openjdk.java.net/~ysuenaga/z/sa-universe/webrev.03/
I created a delta diff for your patch:
http://cr.openjdk.java.net/~stefank/zgc/zSALoadBarriers/yasumasa_webrev.03_diff
>
> I removed dependencies to Linux specific code in it.
Thanks!
I've made some unifications, cleanups, and fixes to the patch:
http://cr.openjdk.java.net/~stefank/zgc/zSALoadBarriers/webrev.04.delta
http://cr.openjdk.java.net/~stefank/zgc/zSALoadBarriers/webrev.04
Notably:
1) Got rid of the duplicated calculations of constants and instead read
the values from the database.
2) There's bug in your code:
http://cr.openjdk.java.net/~stefank/zgc/zSALoadBarriers/yasumasa_webrev.03_diff/src/hotspot/share/runtime/vmStructs.cpp.udiff.html
+ VM_LONG_CONSTANTS_Z(GENERATE_VM_INT_CONSTANT_ENTRY,
+ GENERATE_VM_INT_CONSTANT_WITH_VALUE_ENTRY)
Should be using the _LONG_ versions.
However, that doesn't solve the bug, because there's another bug in the
SA agent that truncates longs to ints. I created a bug report for that:
https://bugs.openjdk.java.net/browse/JDK-8195613
3) There's a bug in the proposed ZAddress.address. If value is 0, then
the orWithMask will throw an NPE. I've changed the code to manipulate
the value, and as a last step convert it with ZOop.to_address.
Thanks,
StefanK
>
>
> 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