My only concern at this point is if Claes and I get hit by a bus, no one will be able to figure this out. I recommend that next release that we switch to character based hash. This means some minor complexity in the C code but I think the java code will be much simpler (and the C not so bad.)
> On Mar 1, 2017, at 7:42 PM, Mandy Chung <mandy.ch...@oracle.com> wrote: > > >> On Feb 27, 2017, at 6:55 PM, Claes Redestad <claes.redes...@oracle.com> >> wrote: >> >> Hi, >> >> thanks Mandy and Jim for reviewing! >> >> However, I've found enough evidence now that we should this >> one step further and eliminating the allocation in >> BasicImageReader::findLocation(String, String), which completely >> gets rid of the regressions we're seeing: >> >> http://cr.openjdk.java.net/~redestad/8175561/jdk.02/ >> http://cr.openjdk.java.net/~redestad/8175561/jdk.01.02.diff/ >> > > This looks correct but we need Jim to confirm the bug you spotted > ImageLocationWriter. > > 59 public static int hashCode(String string) { > > and all of other hashCode methods. > > Nit: "String name" should work as it matches the parameter in the caller > method. Or `s` might be better than “String string”. > > 153 static boolean verify(String module, String name, > 154 final long[] attributes, final ImageStrings strings) { > 67 private static boolean verifyName(String name, int index, final int > length, > 168 final long[] attributes, final ImageStrings strings) { > > Nit: some final and some non-final parameters and better to be consistent. > Any reason why you mark it “final”? > > No need for a new webrev. > > Thanks > Mandy
