<div class="__aliyun_email_body_block"><div  style="line-height:1.7;font-family:Tahoma,Arial,STHeiti,SimSun;font-size:14.0px;color:#000000;"><div  style="clear:both;">Vladimir, thanks for your help.</div><div  style="clear:both;"><br ></div><div  style="clear:both;">Kevin</div><div  style="clear:both;"><br ></div><blockquote  style="margin-right:.0px;margin-top:.0px;margin-bottom:.0px;font-family:Tahoma,Arial,STHeiti,SimSun;font-size:14.0px;color:#000000;"><div  style="clear:both;">------------------------------------------------------------------</div><div  style="clear:both;">From:Vladimir Kozlov <vladimir.kozlov@oracle.com></div><div  style="clear:both;">Send Time:2018年10月31日(星期三) 05:42</div><div  style="clear:both;">To:蒯微(麦庶) <kuaiwei.kw@alibaba-inc.com>; Tobias Hartmann <tobias.hartmann@oracle.com>; hotspot compiler <hotspot-compiler-dev@openjdk.java.net></div><div  style="clear:both;">Cc:李三红(三红) <sanhong.lsh@alibaba-inc.com></div><div  style="clear:both;">Subject:Re: 回复:[Patch] 8210853: C2 doesn't skip post barrier for new allocated objects</div><div  style="clear:both;"><br ></div>Pushed:<br ><br >http://hg.openjdk.java.net/jdk/jdk/rev/8d8702585652<br ><br >Vladimir<br ><br >On 10/29/18 7:44 AM, Kuai Wei wrote:<br >> Hi Vladimir,<br >> <br >>    I've re-run spec test on our test machine several times. In my test, I can get 2~3 percent improvement. So I think it <br >> may help to our system. It would be good for us if it could be merged into upstream JDK. Could you help us to push this <br >> change?<br >> <br >> Thanks,<br >> Kevin<br >> <br >>     ------------------------------------------------------------------<br >>     From:Vladimir Kozlov <vladimir.kozlov@oracle.com><br >>     Send Time:2018年10月25日(星期四) 01:04<br >>     To:蒯微(麦庶) <kuaiwei.kw@alibaba-inc.com>; Tobias Hartmann <tobias.hartmann@oracle.com>; hotspot compiler<br >>     <hotspot-compiler-dev@openjdk.java.net><br >>     Cc:李三红(三红) <sanhong.lsh@alibaba-inc.com><br >>     Subject:Re: 回复:[Patch] 8210853: C2 doesn't skip post barrier for new allocated objects<br >> <br >>     jbb2015 finished and there is no significant difference.<br >> <br >>     To test changes I added diagnostic flag to turn changes on and off:<br >> <br >>     -XX:+UnlockDiagnosticVMOptions -XX:+UseNewCode<br >>     -XX:+UnlockDiagnosticVMOptions -XX:-UseNewCode<br >> <br >>     Please, try it in your setup.  Based on all this data I agree to push changes but please test this latest changes with<br >>     on/off new code to verify that you still see improvement and you still *want* these changes (the last version of it).<br >> <br >>     Thanks,<br >>     Vladimir<br >> <br >>     diff -r c9459e2f7bc8 src/hotspot/share/opto/graphKit.cpp<br >>     --- a/src/hotspot/share/opto/graphKit.cpp Tue Oct 23 17:01:48 2018 -0400<br >>     +++ b/src/hotspot/share/opto/graphKit.cpp Wed Oct 24 08:52:48 2018 -0700<br >>     @@ -2116,8 +2116,17 @@<br >>        // We use this to determine if an object is so "fresh" that<br >>        // it does not require card marks.<br >>        Node* GraphKit::just_allocated_object(Node* current_control) {<br >>     -  if (C->recent_alloc_ctl() == current_control)<br >>     -    return C->recent_alloc_obj();<br >>     +  Node* ctrl = current_control;<br >>     +  // Object::<init> is invoked after allocation, most of invoke nodes<br >>     +  // will be reduced, but a region node is kept in parse time, we check<br >>     +  // the pattern and skip the region node if it degraded to a copy.<br >>     +  if (UseNewCode && ctrl != NULL && ctrl->is_Region() && ctrl->req() == 2 &&<br >>     +      ctrl->as_Region()->is_copy()) {<br >>     +    ctrl = ctrl->as_Region()->is_copy();<br >>     +  }<br >>     +  if (C->recent_alloc_ctl() == ctrl) {<br >>     +   return C->recent_alloc_obj();<br >>     +  }<br >>          return NULL;<br >>        }<br >> <br >> <br >> <br >>     On 10/24/18 8:59 AM, Vladimir Kozlov wrote:<br >>      > I ran jvm2008, jbb2005, jbb2015 (which is still running) and javac tool performance tests.<br >>      > I don't see any effects in jvm2008 and jbb2005. I see slight improvement in javac performance.<br >>      > Lets wait when jbb2015 finish it runs.<br >>      > We also have very unstable jbb2015 results and use mean value from several runs.<br >>      ><br >>      > Vladimir<br >>      ><br >>      > On 10/23/18 7:14 PM, Kuai Wei wrote:<br >>      >><br >>      >> I tested several times and the result is similar. I can get overall performance improvement and some test suite<br >>      >> degradation.<br >>      >> It looks the small jOPS test will cause more unstable score. Can you share some experience to to run spec with stable<br >>      >> result?<br >>      >><br >>      >> Thanks,<br >>      >> Kevin<br >>      >><br >>      >>     ------------------------------------------------------------------<br >>      >>     From:Vladimir Kozlov <vladimir.kozlov@oracle.com><br >>      >>     Send Time:2018年10月24日(星期三) 06:57<br >>      >>     To:蒯微(麦庶) <kuaiwei.kw@alibaba-inc.com>; Tobias Hartmann <tobias.hartmann@oracle.com>; hotspot compiler<br >>      >>     <hotspot-compiler-dev@openjdk.java.net><br >>      >>     Cc:李三红(三红) <sanhong.lsh@alibaba-inc.com><br >>      >>     Subject:Re: 回复:[Patch] 8210853: C2 doesn't skip post barrier for new allocated objects<br >>      >><br >>      >>     Did you run it only one? It is known that jbb2015 produce unstable results. I would suggest to rerun test with<br >>      >>     regression several times.<br >>      >><br >>      >>     My regression testing finished clean. I will submit performance testing too.<br >>      >><br >>      >>     Regards,<br >>      >>     Vladimir<br >>      >><br >>      >>     On 10/19/18 12:56 AM, Kuai Wei wrote:<br >>      >>      > Hi Vladimir,<br >>      >>      ><br >>      >>      >    Thanks for testing the change. It's good to me to move input check from assert to condition.<br >>      >>      > I also tested with specjbb2015. The result is<br >>      >>      >                                                          enable         disable<br >>      >>      > jbb2015.result.metric.max-jOPS           73234          71531      2.38%<br >>      >>      > jbb2015.result.metric.critical-jOPS        31256          30293      3.18%<br >>      >>      > jbb2015.result.SLA-10000-jOPS           16605          14902      11.43%<br >>      >>      > jbb2015.result.SLA-25000-jOPS            21715          23503      -7.61%<br >>      >>      > jbb2015.result.SLA-50000-jOPS            38076          34630       9.95%<br >>      >>      > jbb2015.result.SLA-75000-jOPS            43004          43145      -0.33%<br >>      >>      > jbb2015.result.SLA-100000-jOPS          50525          48751       3.64%<br >>      >>      ><br >>      >>      >   The jvm option I used is "-XX:+UseG1GC<br >>      >><br >>      >> > -XX:+UnlockExperimentalVMOptions -Xmx100g -Xms100g -XX:MaxGCPauseMillis=300 -XX:G1NewSizePercent=8 -XX:G1MaxNewSizePercent=50"<br >>      >><br >>      >><br >>      >> > It can improve overall performance. And there's one test suite with performance degradation. I'm not sure why it happen.<br >>      >><br >>      >>      ><br >>      >>      > Thanks,<br >>      >>      > Kevin<br >>      >>      ><br >>      >>      >     ------------------------------------------------------------------<br >>      >>      >     From:Vladimir Kozlov <vladimir.kozlov@oracle.com><br >>      >>      >     Send Time:2018年10月17日(星期三) 05:16<br >>      >>      >     To:蒯微(麦庶) <kuaiwei.kw@alibaba-<br >>     inc.com>; Tobias Hartmann <tobias.hartmann@oracle.com>; hotspot compiler<br >>      >>      >     <hotspot-compiler-dev@openjdk.java.net><br >>      >>      >     Cc:李三红(三红) <sanhong.lsh@alibaba-inc.com><br >>      >>      >     Subject:Re: 回复:[Patch] 8210853: C2 doesn't skip post barrier for new allocated objects<br >>      >>      ><br >>      >>      >     On 10/16/18 4:56 AM, Kuai Wei wrote:<br >>      >>      >      > Hi Vladimir,<br >>      >>      >      ><br >>      >>      >      >    About the copy region, I made this fix to check if region node is copy. In my test case, the<br >>      >>      >      > region is degraded to copy and<br >>      >>      >      > post write barrier is removed. I haven't checked the detail of the transform. I think there's an<br >>      >>      >      > ideal phase between inlining and<br >>      >>      >      > parsing store oop. So the phi node is removed and region node is degraded to copy.<br >>      >>      >      ><br >>      >>      >      >    We tested the fix in several online trade systems. We can check cpu usage difference between<br >>      >>      >      > machines with same load. I will<br >>      >>      >      > test with some benchmark tests and give the score.<br >>      >>      ><br >>      >>      >     Sounds good. Lets see if it helps in your production system.<br >>      >>      >     I will submit our internal testing with your latest fix with small change. I moved req() == 2 check<br >>      >>      >     from assert to condition to narrow cases (copy Region may have > 2 inputs with only 1 not-null input):<br >>      >>      ><br >>      >>      >     http://cr.openjdk.java.net/~kvn/8210853/webrev.00/<br >>      >>      ><br >>      >>      >     Vladimir<br >>      >>      ><br >>      >>      >      ><br >>      >>      >      > Thanks,<br >>      >>      >      > Kevin<br >>      >>      >      ><br >>      >>      >      >     ------------------------------------------------------------------<br >>      >>      >      >     From:Vladimir Kozlov <vladimir.kozlov@oracle.com><br >>      >>      >      >     Send Time:2018年10月16日(星期二) 08:45<br >>      >>      >      >     To:蒯微(麦庶) <kuaiwei.kw@alibaba-inc.com>; Tobias Hartmann <tobias.hartmann@oracle.com>;<br >>      >>      >      >     hotspot compiler <hotspot-compiler-dev@openjdk.java.net><br >>      >>      >      >     Cc:李三红(三红) <sanhong.lsh@alibaba-inc.com><br >>      >>      >      >     Subject:Re: 回复:[Patch] 8210853: C2 doesn't skip post barrier for new allocated objects<br >>      >>      >      ><br >>      >>      >      >     Hi Kevin,<br >>      >>      >      ><br >>      >>      >      >     On 10/4/18 8:18 AM, Kuai Wei wrote:<br >>      >>      >      >      > Hi Vladimir,<br >>      >>      >      >      ><br >>      >><br >>      >> >      >      >    I'm not sure about the phi node here. Is it used for merging allocation in fast path and slow path?<br >>      >>      >      >      > In parsing phase, the allocation node is not expanded, so there's no this phi node. If my understand<br >>      >>      >      >      > is wrong, please correct me. So far as I know, the region node is created while inlining initialize<br >>      >>      >      >      > method<br >>      >>      >      >      > of super type. And phi node is not necessary. But the region node is always created.<br >>      >>      >      ><br >>      >>      >      >     It is not related to slow and fast path of allocation. It is general rule to have Phi nodes when new<br >>      >>      >      ><br >>      >>      >      >     Region node is created. Like here:<br >>      >>      >      ><br >>      >>      >      >     http://hg.openjdk.java.net/jdk/jdk/file/04d4f1e4aff2/src/hotspot/share/opto/parse1.cpp#l772<br >>      >>      >      ><br >>      >>      >      >      ><br >>      >><br >>      >> >      >      >    I understand your concern. But write barrier is added in parse phase, we must check it in parse time<br >>      >>      >      >      > to do this optimization. If the region node is a copy, could we assume it will not have additional<br >>      >>      >      >      > input?<br >>      >>      >      ><br >>      >>      >      >     Yes, I think it is true. Copy Region never goes back to normal (merge) Region.<br >>      >>      >      ><br >>      >>      >      >      > I checked region node, a copy region node is created when only one input edge and compiler can not<br >>      >>      >      >      > reshape, the region node will be degraded to a copy. I think it will be dead in future phase.<br >>      >>      >      ><br >>      >>      >      >     Yes. But does your case have a 'copy' Region? I originally thought that you have normal Region with<br >>      >>      >      >     just one input.<br >>      >>      >      ><br >>      >>      >      >      ><br >>      >><br >>      >> >      >      >    In our trade system, this optimization could improve 5%+ performance. It's a big improvement and we<br >>      >>      >      >      > don't want to lose it. If there's better solution, we are pleasure to implement it.<br >>      >>      >      ><br >>      >>      >      >     Yes, I agree that we should fix it.<br >>      >>      >      ><br >>      >>      >      >     What testing did you perform?<br >>      >>      >      ><br >>      >>      >      >     Thanks,<br >>      >>      >      >     Vladimir<br >>      >>      >      ><br >>      >>      >      >      ><br >>      >>      >      >      > Thanks,<br >>      >>      >      >      > Kevin<br >>      >>      >      >      ><br >>      >>      >      >      >     ------------------------------------------------------------------<br >>      >>      >      >      >     From:Vladimir Kozlov <vladimir.kozlov@oracle.com><br >>      >>      >      >      >     Send Time:2018年10月4日(星期四) 06:50<br >>      >>      >      >      >     To:蒯微(麦庶) <kuaiwei.kw@alibaba-inc.com>; Tobias Hartmann <tobias.hartmann@oracle.com>;<br >>      >>      >      >      >     hotspot compiler <hotspot-compiler-dev@openjdk.java.net><br >>      >>      >      >      >     Cc:李三红(三红) <sanhong.lsh@alibaba-inc.com><br >>      >>      >      >      >     Subject:Re: 回复:[Patch] 8210853: C2 doesn't skip post barrier for new allocated objects<br >>      >>      >      >      ><br >>      >>      >      >      >     Sorry, I mean<br >>      >>      >      >      ><br >>      >>      >      >      >     "should exist Phi *associated* with allocation"<br >>      >>      >      >      ><br >>      >>      >      >      >     On 10/3/18 3:48 PM, Vladimir Kozlov wrote:<br >>      >>      >      >      >      > Hi Kevin,<br >>      >>      >      >      >      ><br >>      >><br >>      >> >      >      >      > You correctly pointed in your analysis that if base of address is pointing to allocation there<br >>      >><br >>      >> >      >      >      > should not be other control path to this store. But it also means that before there should exist Phi<br >>      >><br >>      >><br >>      >> >      >      >      > not associated with allocation when Region node was added. If Phi node was removed at some time<br >>      >>      >      >      >      > Region node should be removed too (or not add it in first place). Please, look on that.<br >>      >>      >      >      >      ><br >>      >><br >>      >> >      >      >      > I share Tobias's concern about skipping Region node in Parse phase when IR graph is still constructed.<br >>      >><br >>      >>      >      >      >      ><br >>      >>      >      >      >      > Thanks,<br >>      >>      >      >      >      > Vladimir<br >>      >>      >      >      >      ><br >>      >>      >      >      >      > On 10/3/18 8:08 AM, Kuai Wei wrote:<br >>      >>      >      >      >      >> Hi Tobias,<br >>      >>      >      >      >      >><br >>      >>      >      >      >      >>   I made the change to check with RegionNode::is_copy, could you check the new patch?<br >>      >>      >      >      >      >><br >>      >>      >      >      >      >> diff --git a/src/hotspot/share/opto/graphKit.cpp b/src/hotspot/share/opto/graphKit.cpp<br >>      >>      >      >      >      >> index 068141f..884a76c 100644<br >>      >>      >      >      >      >> --- a/src/hotspot/share/opto/graphKit.cpp<br >>      >>      >      >      >      >> +++ b/src/hotspot/share/opto/graphKit.cpp<br >>      >>      >      >      >      >> @@ -2126,7 +2126,15 @@ void GraphKit::uncommon_trap(int trap_request,<br >>      >>      >      >      >      >>   // We use this to determine if an object is so "fresh" that<br >>      >>      >      >      >      >>   // it does not require card marks.<br >>      >>      >      >      >      >>   Node* GraphKit::just_allocated_object(Node* current_control) {<br >>      >>      >      >      >      >> -  if (C->recent_alloc_ctl() == current_control)<br >>      >>      >      >      >      >> +  Node * ctrl = current_control;<br >>      >>      >      >      >      >> +  // Object::<init> is invoked after allocation, most of invoke nodes<br >>      >>      >      >      >      >> +  // will be reduced, but a region node is kept in parse time, we check<br >>      >>      >      >      >      >> +  // the pattern and skip the region node<br >>      >>      >      >      >      >> +  if (ctrl != NULL && ctrl->is_Region() && ctrl->as_Region()->is_copy()) {<br >>      >>      >      >      >      >> +    assert(ctrl->req() == 2, "copy region has only 2 inputs");<br >>      >>      >      >      >      >> +    ctrl = ctrl->as_Region()->is_copy();<br >>      >>      >      >      >      >> +  }<br >>      >>      >      >      >      >> +  if (C->recent_alloc_ctl() == ctrl)<br >>      >>      >      >      >      >>       return C->recent_alloc_obj();<br >>      >>      >      >      >      >>     return NULL;<br >>      >>      >      >      >      >>   }<br >>      >>      >      >      >      >> Thanks,<br >>      >>      >      >      >      >> Kevin<br >>      >>      >      >      >      >><br >>      >>      >      >      >      >>     ------------------------------------------------------------------<br >>      >>      >      >      >      >>     From:蒯微(麦庶) <kuaiwei.kw@alibaba-inc.com><br >>      >>      >      >      >      >>     Send Time:2018年9月25日(星期二) 21:50<br >>      >>      >      >      >      >>     To:Tobias Hartmann <tobias.hartmann@oracle.com>; hotspot compiler<br >>      >>      >      >      >      >>     <hotspot-compiler-dev@openjdk.java.net><br >>      >>      >      >      >      >>     Cc:李三红(三红) <sanhong.lsh@alibaba-inc.com><br >>      >>      >      >      >      >>     Subject:回复:回复:<br >>      >>      >      >     [Patch] 8210853: C2 doesn't skip post barrier for new allocated objects<br >>      >>      >      >      >      >><br >>      >>      >      >      >      >>     Hi Tobias,<br >>      >>      >      >      >      >><br >>      >><br >>      >> >      >      >      >>        Thanks for your comments. I will check RegionNode::is_copy to see if it can be used to detect<br >>      >><br >>      >>      >      >      >      >>     unnecessary region node. I will send new review after testing.<br >>      >>      >      >      >      >><br >>      >>      >      >      >      >>     Best Regards,<br >>      >>      >      >      >      >>     Kevin<br >>      >>      >      >      >      >><br >>      >>      >      >      >      >>     ------------------------------------------------------------------<br >>      >>      >      >      >      >>     发件人:Tobias Hartmann <tobias.hartmann@oracle.com><br >>      >>      >      >      >      >>     发送时间:2018年9月24日(星期一) 21:34<br >>      >>      >      >      >      >>     收件人:蒯微(麦庶) <kuaiwei.kw@alibaba-inc.com>; hotspot compiler<br >>      >>      >      >      >      >>     <hotspot-compiler-dev@openjdk.java.net><br >>      >>      >      >      >      >>     抄 送:李三红(三红) <sanhong.lsh@alibaba-inc.com><br >>      >>      >      >      >      >>     主 题:Re: 回复:<br >>      >>      >      >     [Patch] 8210853: C2 doesn't skip post barrier for new allocated objects<br >>      >>      >      >      >      >><br >>      >>      >      >      >      >>     Hi Kevin,<br >>      >>      >      >      >      >><br >>      >>      >      >      >      >>     On 24.09.2018 08:06, Kuai Wei wrote:<br >>      >>      >      >      >      >><br >>      >><br >>      >> >      >      >      >> >   Thanks for your suggestion. I think your point is the region node may have new path in later parse<br >>      >><br >>      >>      >      >      >      >><br >>      >>      >      >      >      >>      > phase, so we can not make sure the region node will be optimized.<br >>      >>      >      >      >      >><br >>      >>      >      >      >      >><br >>      >><br >>      >> >      >      >      >> Yes, my point is that a new path to the region might be added after your optimization and that path<br >>      >><br >>      >>      >      >      >      >>     might contain stores to the newly allocated object.<br >>      >>      >      >      >      >><br >>      >><br >>      >> >      >      >      >>      >   It's a good question and I checked it. Now I think it may not cause trouble. In post barrier<br >>      >><br >>      >><br >>      >> >      >      >      >>      > reduce, the oop store use allocation node as base pointer. The data graph guarantee control of<br >>      >><br >>      >>      >      >      >      >><br >>      >><br >>      >> >      >      >      >> > allocation node should dominate control of store. If allocation node is in pred of region node and<br >>      >><br >>      >>      >      >      >      >><br >>      >><br >>      >> >      >      >      >> > there's a new path into region, the graph is bad because we can reach store without allocation.<br >>      >>      >      >      >      >><br >>      >><br >>      >> >      >      >      >>     Yes but the new path might be a backedge from a loop that is dominated by the allocation.<br >>      >>      >      >      >      >><br >>      >>      >      >      >      >><br >>      >><br >>      >> >      >      >      >> > If allocation node is in a domination ancestor, the graph shape is a little complicated, so we can not<br >>      >><br >>      >>      >      >      >      >><br >>      >>      >      >      >      >>      > reach control of allocation by skipping one region.<br >>      >>      >      >      >      >><br >>      >>      >      >      >      >><br >>      >><br >>      >> >      >      >      >> Right, that's basically the implicit assumption of your patch. I'm not sure if it always holds. But<br >>      >><br >>      >>      >      >      >      >>     I think you should at least use RegionNode::is_copy().<br >>      >>      >      >      >      >><br >>      >>      >      >      >      >>     Let's see what other reviewers think.<br >>      >>      >      >      >      >><br >>      >>      >      >      >      >><br >>      >><br >>      >> >      >      >      >> >   The better solution is we can know the region node is created in exit_map and we will not change<br >>      >><br >>      >>      >      >      >      >>      > it in later. Is there any way to know it in compile time?<br >>      >>      >      >      >      >><br >>      >>      >      >      >      >><br >>      >><br >>      >> >      >      >      >> The region node is created in Parse::build_exits(). I don't think there is a way to keep track of this.<br >>      >><br >>      >>      >      >      >      >><br >>      >>      >      >      >      >><br >>      >>      >      >      >      >>     Thanks,<br >>      >>      >      >      >      >>     Tobias<br >>      >>      >      >      >      >><br >>      >>      >      >      ><br >>      >>      >      ><br >>      >>      >      ><br >>      >>      ><br >>      >>      ><br >> </blockquote></div></div>