SA for ZGC

Stefan Karlsson stefan.karlsson at oracle.com
Mon Jan 15 08:48:48 UTC 2018 

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