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/
