Looks good. Thanks, /Staffan
On 23 aug 2013, at 23:49, Jiangli Zhou <jiangli.z...@oracle.com> wrote: > Hi, > > Please review the fix for 8023477: > > http://cr.openjdk.java.net/~jiangli/8023477/webrev.00/ > > Both tests reported by the bug failed due to the same problem. They both are > passing now. > > The original memory reduction change for 8021948 turned out to be a little > trickier than I expected. > > Thanks, > Jiangli > > > On 08/23/2013 11:00 AM, Jiangli Zhou wrote: >> I found the issue in the SA code. For class without generic signature, the >> InstanceKlass::_generic_signature_index is 0. Need to check for this case in >> the SA code. I'm working on the fix and doing testing. >> >> Thanks, >> Jiangli >> >> On 08/23/2013 08:21 AM, Jiangli Zhou wrote: >>> Hi Staffan, >>> >>> Thanks for the info. Will look into that one. >>> >>> Jiangli >>> >>> On 08/23/2013 05:27 AM, Staffan Larsen wrote: >>>> Unfortunately there are two more tests that started failing after the >>>> initial change. See: JDK-8023477. >>>> >>>> /Staffan >>>> >>>> On 23 aug 2013, at 01:16, Jiangli Zhou <jiangli.z...@oracle.com> wrote: >>>> >>>>> Hi Coleen, >>>>> >>>>> Yes, that's the case. Thanks for the review. I'll push this to hotspot-rt. >>>>> >>>>> Thanks, >>>>> Jiangli >>>>> >>>>> On 08/22/2013 03:34 PM, Coleen Phillimore wrote: >>>>>> Hi, >>>>>> Is it the case that the old index isn't in the index map because it >>>>>> didn't change? If so, this looks correct. >>>>>> Thanks, >>>>>> Coleen >>>>>> >>>>>> >>>>>> On 08/22/2013 06:15 PM, Jiangli Zhou wrote: >>>>>>> On 08/22/2013 03:10 PM, serguei.spit...@oracle.com wrote: >>>>>>>> Hi Jiangli, >>>>>>>> >>>>>>>> The fix is good and safe. >>>>>>>> I'm happy you fixed another case as well! >>>>>>>> Let's consider current bug as a clean-up issue so that we do not need >>>>>>>> to file a separate bug. :) >>>>>>> Ok. >>>>>>> >>>>>>> Thanks, >>>>>>> Jiangli >>>>>>> >>>>>>>> Thanks, >>>>>>>> Serguei >>>>>>>> >>>>>>>> >>>>>>>> On 8/22/13 2:50 PM, Jiangli Zhou wrote: >>>>>>>>> Hi Serguei, >>>>>>>>> >>>>>>>>> I've also made change to the case that you discovered. Please let me >>>>>>>>> know if you think a separate bug should be filed to track it instead. >>>>>>>>> >>>>>>>>> http://cr.openjdk.java.net/~jiangli/8023547/webrev.01/ >>>>>>>>> >>>>>>>>> Thanks, >>>>>>>>> Jiangli >>>>>>>>> >>>>>>>>> On 08/22/2013 02:24 PM, Jiangli Zhou wrote: >>>>>>>>>> Hi Serguei, >>>>>>>>>> >>>>>>>>>> Thank you very much for the review and confirmation with the test. >>>>>>>>>> >>>>>>>>>> Jiangli >>>>>>>>>> >>>>>>>>>> On 08/22/2013 02:18 PM, serguei.spit...@oracle.com wrote: >>>>>>>>>>> Hi Jiangli, >>>>>>>>>>> >>>>>>>>>>> Thank you for the quick fix which looks fine to me. >>>>>>>>>>> I confirm the test is passed with it. >>>>>>>>>>> >>>>>>>>>>> Staffan, thank you for the regression isolation. >>>>>>>>>>> I've noticed the following fragment in this file which seems has a >>>>>>>>>>> similar issue: >>>>>>>>>>> >>>>>>>>>>> // We also need to rewrite the parameter name indexes, if there is >>>>>>>>>>> // method parameter data present >>>>>>>>>>> if(method->has_method_parameters()) { >>>>>>>>>>> const int len = method->method_parameters_length(); >>>>>>>>>>> MethodParametersElement* elem = >>>>>>>>>>> method->method_parameters_start(); >>>>>>>>>>> >>>>>>>>>>> for (int i = 0; i < len; i++) { >>>>>>>>>>> const u2 cp_index = elem[i].name_cp_index; >>>>>>>>>>> elem[i].name_cp_index = find_new_index(cp_index); >>>>>>>>>>> } >>>>>>>>>>> } >>>>>>>>>>> } // end rewrite_cp_refs_in_method() >>>>>>>>>>> >>>>>>>>>>> The result of the find_new_index() above is not checked for 0. >>>>>>>>>>> >>>>>>>>>>> Thanks, >>>>>>>>>>> Serguei >>>>>>>>>>> >>>>>>>>>>> On 8/22/13 12:38 PM, Jiangli Zhou wrote: >>>>>>>>>>>> Hi Staffan, Serguei and others, >>>>>>>>>>>> >>>>>>>>>>>> Here is the webrev for the 8023547 fix: >>>>>>>>>>>> >>>>>>>>>>>> http://cr.openjdk.java.net/~jiangli/8023547/webrev.00/ >>>>>>>>>>>> >>>>>>>>>>>> Thanks! >>>>>>>>>>>> >>>>>>>>>>>> Jiangli >>> >> >
