Hi Claes, Did you considere to use return c.asSubclass(BoundMethodHandle.class); instead of return (Class<? extends BoundMethodHandle>)c; to avoid the @SupressWarnings, like in generateConcreteBMHClass ?
cheers, Rémi ----- Mail original ----- > De: "Claes Redestad" <claes.redes...@oracle.com> > À: "Peter Levart" <peter.lev...@gmail.com>, "Mandy Chung" > <mandy.ch...@oracle.com> > Cc: "jigsaw-dev" <jigsaw-...@openjdk.java.net>, "core-libs-dev" > <core-libs-dev@openjdk.java.net> > Envoyé: Mercredi 30 Mars 2016 12:53:08 > Objet: Re: RFR: 8152641: Plugin to generate BMH$Species classes ahead-of-time > > Hi, > > I think Class.forName(Module, String) seemed like a nice > efficiency/simplicity compromise, but there is some usage of > lambdas/method-refs in the Module lookup code, especially for exploded > modules (which get exercised during JDK build). Depending on a lambda > from code in java.lang.invoke means we fail to bootstrap. > > But hey, we're living in an encapsulated world now, and this is in > java.base, so why not go directly to the BootLoader: > > http://cr.openjdk.java.net/~redestad/8152641/webrev.03/ > > Since this is what's called from Class.forName(Module, String), the > overhead should be even smaller than in your classForNameInModule test. > > If I call this final, will you say "Reviewed"? :-) > > /Claes > > PS: The default list of types is generated in a few adhoc tests not part > of this patch. I'm considering proposing add support for generating this > list at build time. Maybe a JEP called "Build system support for > profile-guided optimizations", which could also handle > https://bugs.openjdk.java.net/browse/JDK-8150044 > > On 2016-03-30 09:53, Peter Levart wrote: > > Hi Claes, > > > > Sorry, I found a flaw in the benchmark (the regex pattern to split > > comma-separated string into substrings was wrong). What the benchmark > > did was compare the overheads of a single lookup of a longer class > > name containing commas. Here's the corrected result of overheads of 5 > > unsuccessful lookups: > > > > Benchmark (generate) (lookup) Mode Cnt > > Score Error Units > > ClassForNameBench.classForName LL,LLL,LLLL,LLLLL,LLLLLL > > LLI,LLLI,LLLLI,LLLLLI,LLLLLLI avgt 10 29627.141 ± 567.427 ns/op > > ClassForNameBench.classForNameInModule LL,LLL,LLLL,LLLLL,LLLLLL > > LLI,LLLI,LLLLI,LLLLLI,LLLLLLI avgt 10 1073.256 ± 23.794 ns/op > > ClassForNameBench.hashSetContains LL,LLL,LLLL,LLLLL,LLLLLL > > LLI,LLLI,LLLLI,LLLLLI,LLLLLLI avgt 10 33.022 ± 0.066 ns/op > > ClassForNameBench.switchStatement LL,LLL,LLLL,LLLLL,LLLLLL > > LLI,LLLI,LLLLI,LLLLLI,LLLLLLI avgt 10 38.498 ± 5.842 ns/op > > > > ...overheads are a little bigger (x5 approx.). > > > > > > Here's the corrected benchmark: > > > > > > package jdk.test; > > > > import org.openjdk.jmh.annotations.*; > > import org.openjdk.jmh.infra.Blackhole; > > > > import java.io.Serializable; > > import java.lang.invoke.MethodHandle; > > import java.util.Iterator; > > import java.util.Set; > > import java.util.concurrent.TimeUnit; > > import java.util.stream.Collectors; > > import java.util.stream.Stream; > > > > @BenchmarkMode(Mode.AverageTime) > > @Fork(value = 1, warmups = 0) > > @Warmup(iterations = 10) > > @Measurement(iterations = 10) > > @OutputTimeUnit(TimeUnit.NANOSECONDS) > > @State(Scope.Thread) > > public class ClassForNameBench { > > > > static final String BMH = "java/lang/invoke/BoundMethodHandle"; > > static final String SPECIES_PREFIX_NAME = "Species_"; > > static final String SPECIES_PREFIX_PATH = BMH + "$" + > > SPECIES_PREFIX_NAME; > > > > @Param({"LL,LLL,LLLL,LLLLL,LLLLLL"}) > > public String generate; > > > > @Param({"LLI,LLLI,LLLLI,LLLLLI,LLLLLLI"}) > > public String lookup; > > > > private String[] generatedTypes; > > private String[] lookedUpTypes; > > private Set<String> generatedNames; > > private String[] lookedUpNames; > > > > @Setup(Level.Trial) > > public void setup() { > > generatedTypes = generate.trim().split("\\s*,\\s*"); > > lookedUpTypes = lookup.trim().split("\\s*,\\s*"); > > generatedNames = Stream.of(generatedTypes) > > .map(types -> SPECIES_PREFIX_PATH + > > shortenSignature(types)) > > .collect(Collectors.toSet()); > > lookedUpNames = Stream.of(lookedUpTypes) > > .map(types -> SPECIES_PREFIX_PATH + > > shortenSignature(types)) > > .toArray(String[]::new); > > } > > > > @Benchmark > > public void classForName(Blackhole bh) { > > for (String name : lookedUpNames) { > > try { > > bh.consume(Class.forName(name, false, > > MethodHandle.class.getClassLoader())); > > } catch (ClassNotFoundException e) { > > bh.consume(e); > > } > > } > > } > > > > @Benchmark > > public void classForNameInModule(Blackhole bh) { > > for (String name : lookedUpNames) { > > bh.consume(Class.forName(MethodHandle.class.getModule(), name)); > > } > > } > > > > @Benchmark > > public void hashSetContains(Blackhole bh) { > > for (String name : lookedUpNames) { > > bh.consume(generatedNames.contains(name)); > > } > > } > > > > @Benchmark > > public void switchStatement(Blackhole bh) { > > for (String types : lookedUpTypes) { > > bh.consume(getBMHSwitch(types)); > > } > > } > > > > static Class<?> getBMHSwitch(String types) { > > // should be in sync with @Param generate above... > > switch (types) { > > case "LL": return Object.class; > > case "LLL": return Serializable.class; > > case "LLLL": return Iterator.class; > > case "LLLLL": return Throwable.class; > > case "LLLLLL": return String.class; > > default: return null; > > } > > } > > > > // copied from non-public LambdaForm > > static String shortenSignature(String signature) { > > // Hack to make signatures more readable when they show up in > > method names. > > final int NO_CHAR = -1, MIN_RUN = 3; > > int c0, c1 = NO_CHAR, c1reps = 0; > > StringBuilder buf = null; > > int len = signature.length(); > > if (len < MIN_RUN) return signature; > > for (int i = 0; i <= len; i++) { > > // shift in the next char: > > c0 = c1; c1 = (i == len ? NO_CHAR : signature.charAt(i)); > > if (c1 == c0) { ++c1reps; continue; } > > // shift in the next count: > > int c0reps = c1reps; c1reps = 1; > > // end of a character run > > if (c0reps < MIN_RUN) { > > if (buf != null) { > > while (--c0reps >= 0) > > buf.append((char) c0); > > } > > continue; > > } > > // found three or more in a row > > if (buf == null) > > buf = new StringBuilder().append(signature, 0, i - > > c0reps); > > buf.append((char) c0).append(c0reps); > > } > > return (buf == null) ? signature : buf.toString(); > > } > > } > > > > > > Regards, Peter > > > > > > > > On 03/30/2016 09:40 AM, Peter Levart wrote: > >> Hi Claes, > >> > >> On 03/30/2016 01:03 AM, Claes Redestad wrote: > >>> Hi Peter, Mandy, > >>> > >>> On 2016-03-26 12:47, Peter Levart wrote: > >>>> > >>>> Comparing this with proposed code from webrev: > >>>> > >>>> 493 try { > >>>> 494 return (Class<? extends > >>>> BoundMethodHandle>) > >>>> 495 Class.forName("java.lang.invoke.BoundMethodHandle$Species_" + > >>>> LambdaForm.shortenSignature(types)); > >>>> 496 } catch (ClassNotFoundException cnf) { > >>>> 497 // Not pregenerated, generate the > >>>> class > >>>> 498 return > >>>> generateConcreteBMHClass(types); > >>>> 499 } > >>>> > >>>> > >>>> ...note that the function passed to CLASS_CACHE.computeIfAbsent is > >>>> executed just once per distinct 'types' argument. Even if you put > >>>> the generated switch between a call to getConcreteBMHClass and > >>>> CLASS_CACHE.computeIfAbsent, getConcreteBMHClass(String types) is > >>>> executed just once per distinct 'types' argument (except in rare > >>>> occasions when VM can not initialize the loaded class). > >>>> > >>>> In this respect a successful Class.forName() is not any worse than > >>>> static resolving. It's just that unsuccessful Class.forName() > >>>> represents some overhead for classes that are not pre-generated. So > >>>> an alternative way to get rid of that overhead is to have a HashSet > >>>> of 'types' strings for pre-generated classes at hand in order to > >>>> decide whether to call Class.forName or generateConcreteBMHClass. > >>>> > >>>> What's easier to support is another question. > >>>> > >>>> Regards, Peter > >>> > >>> to have something to compare with I built a version which, like you > >>> suggest, > >>> generates a HashSet<String> with the set of generated classes here: > >>> > >>> http://cr.openjdk.java.net/~redestad/8152641/webrev.02/ > >>> > >>> This adds a fair bit of complexity to the plugin and requires we add > >>> a nested > >>> class in BoundMethodHandle that we can replace. Using the anonymous > >>> compute Function for this seems like the best choice for this. > >> > >> ...what I had in mind as alternative to a pregenerated class with a > >> switch was a simple textual resource file, generated by plugin, > >> read-in by BMH into a HashSet. No special-purpose class generation > >> and much less complexity for the plugin. > >> > >>> > >>> I've not been able to measure any statistical difference in real > >>> startup terms, > >>> though, and not figured out a smart way to benchmark the overhead of > >>> the > >>> CNFE in relation to the class generation (my guess it adds a > >>> fraction to the > >>> total cost) and since this adds ever so little footprint and an > >>> extra lookup to the > >>> fast path it would seem that this is the wrong trade-off to do here. > >> > >> Yes, perhaps it would be best to just use Class.forName(module, > >> className) instead. I have created a little benchmark to compare > >> overheads (just overheads) of unsuccessful lookups for pregenerated > >> classes (a situation where a BMH class is requested that has not been > >> pregenerated) and here's the result for overhead of 5 unsuccessfull > >> lookups: > >> > >> Benchmark (generate) (lookup) Mode Cnt > >> Score Error Units > >> ClassForNameBench.classForName LL,LLL,LLLL,LLLLL,LLLLLL > >> LLI,LLLI,LLLLI,LLLLLI,LLLLLLI avgt 10 6800.878 ± 421.424 ns/op > >> ClassForNameBench.classForNameInModule LL,LLL,LLLL,LLLLL,LLLLLL > >> LLI,LLLI,LLLLI,LLLLLI,LLLLLLI avgt 10 209.574 ± 2.114 ns/op > >> ClassForNameBench.hashSetContains LL,LLL,LLLL,LLLLL,LLLLLL > >> LLI,LLLI,LLLLI,LLLLLI,LLLLLLI avgt 10 6.813 ± 0.317 ns/op > >> ClassForNameBench.switchStatement LL,LLL,LLLL,LLLLL,LLLLLL > >> LLI,LLLI,LLLLI,LLLLLI,LLLLLLI avgt 10 6.601 ± 0.061 ns/op > >> > >> ...compared to runtime BMH class generation and loading this is > >> really a very minor overhead. I would just use Class.forName(module, > >> className) and reduce the complexity of the plugin. > >> > >> What do you think? > >> > >> > >> Here's the benchmark: > >> > >> package jdk.test; > >> > >> import org.openjdk.jmh.annotations.*; > >> import org.openjdk.jmh.infra.Blackhole; > >> > >> import java.io.Serializable; > >> import java.lang.invoke.MethodHandle; > >> import java.util.Iterator; > >> import java.util.Set; > >> import java.util.concurrent.TimeUnit; > >> import java.util.stream.Collectors; > >> import java.util.stream.Stream; > >> > >> @BenchmarkMode(Mode.AverageTime) > >> @Fork(value = 1, warmups = 0) > >> @Warmup(iterations = 10) > >> @Measurement(iterations = 10) > >> @OutputTimeUnit(TimeUnit.NANOSECONDS) > >> @State(Scope.Thread) > >> public class ClassForNameBench { > >> > >> static final String BMH = "java/lang/invoke/BoundMethodHandle"; > >> static final String SPECIES_PREFIX_NAME = "Species_"; > >> static final String SPECIES_PREFIX_PATH = BMH + "$" + > >> SPECIES_PREFIX_NAME; > >> > >> @Param({"LL,LLL,LLLL,LLLLL,LLLLLL"}) > >> public String generate; > >> > >> @Param({"LLI,LLLI,LLLLI,LLLLLI,LLLLLLI"}) > >> public String lookup; > >> > >> private String[] generatedTypes; > >> private String[] lookedUpTypes; > >> private Set<String> generatedNames; > >> private String[] lookedUpNames; > >> > >> @Setup(Level.Trial) > >> public void setup() { > >> generatedTypes = generate.trim().split("\\s+,\\s+"); > >> lookedUpTypes = lookup.trim().split("\\s+,\\s+"); > >> generatedNames = Stream.of(generatedTypes) > >> .map(types -> SPECIES_PREFIX_PATH + > >> shortenSignature(types)) > >> .collect(Collectors.toSet()); > >> lookedUpNames = Stream.of(lookedUpTypes) > >> .map(types -> SPECIES_PREFIX_PATH + > >> shortenSignature(types)) > >> .toArray(String[]::new); > >> } > >> > >> @Benchmark > >> public void classForName(Blackhole bh) { > >> for (String name : lookedUpNames) { > >> try { > >> bh.consume(Class.forName(name, false, > >> MethodHandle.class.getClassLoader())); > >> } catch (ClassNotFoundException e) { > >> bh.consume(e); > >> } > >> } > >> } > >> > >> @Benchmark > >> public void classForNameInModule(Blackhole bh) { > >> for (String name : lookedUpNames) { > >> bh.consume(Class.forName(MethodHandle.class.getModule(), name)); > >> } > >> } > >> > >> @Benchmark > >> public void hashSetContains(Blackhole bh) { > >> for (String name : lookedUpNames) { > >> bh.consume(generatedNames.contains(name)); > >> } > >> } > >> > >> @Benchmark > >> public void switchStatement(Blackhole bh) { > >> for (String types : lookedUpTypes) { > >> bh.consume(getBMHSwitch(types)); > >> } > >> } > >> > >> static Class<?> getBMHSwitch(String types) { > >> // should be in sync with @Param generate above... > >> switch (types) { > >> case "LL": return Object.class; > >> case "LLL": return Serializable.class; > >> case "LLLL": return Iterator.class; > >> case "LLLLL": return Throwable.class; > >> case "LLLLLL": return String.class; > >> default: return null; > >> } > >> } > >> > >> // copied from non-public LambdaForm > >> static String shortenSignature(String signature) { > >> // Hack to make signatures more readable when they show up in > >> method names. > >> final int NO_CHAR = -1, MIN_RUN = 3; > >> int c0, c1 = NO_CHAR, c1reps = 0; > >> StringBuilder buf = null; > >> int len = signature.length(); > >> if (len < MIN_RUN) return signature; > >> for (int i = 0; i <= len; i++) { > >> // shift in the next char: > >> c0 = c1; c1 = (i == len ? NO_CHAR : signature.charAt(i)); > >> if (c1 == c0) { ++c1reps; continue; } > >> // shift in the next count: > >> int c0reps = c1reps; c1reps = 1; > >> // end of a character run > >> if (c0reps < MIN_RUN) { > >> if (buf != null) { > >> while (--c0reps >= 0) > >> buf.append((char) c0); > >> } > >> continue; > >> } > >> // found three or more in a row > >> if (buf == null) > >> buf = new StringBuilder().append(signature, 0, i - > >> c0reps); > >> buf.append((char) c0).append(c0reps); > >> } > >> return (buf == null) ? signature : buf.toString(); > >> } > >> } > >> > >> > >> > >> Regards, Peter > >> > >>> > >>> All-in-all I lean towards moving forward with the first, simpler > >>> revision of this > >>> patch[1] and then evaluate if webrev.02 or a String-switch+static > >>> resolve > >>> as a follow-up. > >>> > >>> A compromise would be to keep the SpeciesLookup class introduced here > >>> to allow removing all overhead in case the plugin is disabled. > >>> > >>> Mandy: I've not found any test (under jdk/test/tools/jlink or > >>> elsewhere) which > >>> has to be updated when adding a plugin like this. > >>> > >>> Thanks! > >>> > >>> /Claes > >>> > >>> [1] http://cr.openjdk.java.net/~redestad/8152641/webrev.01/ > >> > > > >
