Re: review request (L): JDK changes for 7023639: JSR 292 method handle invocation needs a fast path for compiled code
On 07/23/2012 10:31 PM, John Rose wrote: On Jul 23, 2012, at 2:27 AM, Aleksey Shipilev wrote: The code does not need to be scalable, because the number of entries in the cache is small (order of 10-100) and scales only with type schema complexity, not workload complexity. If I had a nickel... Not sure if this logic is applicable in this particular case. This is the potential performance cliff you are eager to get rid of with new implementation. Given enough users and applications make use of this code, someone will eventually step and burn on this. This wishful thinking it's okay to use synchronized here, because this couldn't possibly get contended lead to many beautiful scalability bottlenecks throughout the JDK. While it is usually a simple thing to fix, I'm keen on not allowing to make the same mistakes over and over again. So in this case, static synchronized is the correct choice. I shall wait for mainline integration to complete and then try to run the microbenchmarks against the new backend; will see if this potential issue is the practical one. -Aleksey. signature.asc Description: OpenPGP digital signature ___ mlvm-dev mailing list mlvm-dev@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
Re: review request (L): JDK changes for 7023639: JSR 292 method handle invocation needs a fast path for compiled code
On Jul 24, 2012, at 2:55 AM, Aleksey Shipilev aleksey.shipi...@oracle.com wrote: On 07/23/2012 10:31 PM, John Rose wrote: On Jul 23, 2012, at 2:27 AM, Aleksey Shipilev wrote: The code does not need to be scalable, because the number of entries in the cache is small (order of 10-100) and scales only with type schema complexity, not workload complexity. If I had a nickel... Not sure if this logic is applicable in this particular case. This is the potential performance cliff you are eager to get rid of with new implementation. No it's not. We know exactly what causes the performance cliff. It's a completely unrelated issue in the VM. -- Chris Given enough users and applications make use of this code, someone will eventually step and burn on this. This wishful thinking it's okay to use synchronized here, because this couldn't possibly get contended lead to many beautiful scalability bottlenecks throughout the JDK. While it is usually a simple thing to fix, I'm keen on not allowing to make the same mistakes over and over again. So in this case, static synchronized is the correct choice. I shall wait for mainline integration to complete and then try to run the microbenchmarks against the new backend; will see if this potential issue is the practical one. -Aleksey. ___ mlvm-dev mailing list mlvm-dev@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev ___ mlvm-dev mailing list mlvm-dev@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
Re: review request (L): JDK changes for 7023639: JSR 292 method handle invocation needs a fast path for compiled code
On 07/24/2012 07:15 PM, Christian Thalinger wrote: Not sure if this logic is applicable in this particular case. This is the potential performance cliff you are eager to get rid of with new implementation. No it's not. We know exactly what causes the performance cliff. It's a completely unrelated issue in the VM. Sorry for misguided definition there, thought quoting does the trick for me. I was meant to say that the scalability bottlenecks like these pop out quickly, unexpected and hit hard. The difference between single-threaded and two-threaded versions could be so dramatic, so you can easily say it goes down the sink. It's inconvenient to fix one cliff and introduce the other, albeit completely unrelated. -Aleksey. signature.asc Description: OpenPGP digital signature ___ mlvm-dev mailing list mlvm-dev@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
Re: review request (L): JDK changes for 7023639: JSR 292 method handle invocation needs a fast path for compiled code
On 07/22/2012 03:45 AM, John Rose wrote:
On Jul 18, 2012, at 1:43 AM, Aleksey Shipilev wrote:
Yes.
Thanks John. I'm having a glance over the fix in new webrev, and this
feels even worse:
+ private static SpeciesData get(String types) {
+// Acquire cache lock for query.
+SpeciesData d = lookupCache(types);
+if (!d.isPlaceholder())
+return d;
+Class? extends BoundMethodHandle cbmh;
+synchronized (d) {
+// Use synch. on the placeholder to prevent multiple
instantiation of one species.
+// Creating this class forces a recursive call to
getForClass.
+cbmh = Factory.generateConcreteBMHClass(types);
+}
+// Reacquire cache lock.
+d = lookupCache(types);
+// Class loading must have upgraded the cache.
+assert(d != null !d.isPlaceholder());
+return d;
+}
+ static SpeciesData getForClass(String types, Class? extends
BoundMethodHandle clazz) {
+// clazz is a new class which is initializing its
SPECIES_DATA field
+return updateCache(types, new SpeciesData(types, clazz));
+}
+ private static synchronized SpeciesData lookupCache(String types) {
+SpeciesData d = CACHE.get(types);
+if (d != null) return d;
+d = new SpeciesData(types);
+assert(d.isPlaceholder());
+CACHE.put(types, d);
+return d;
+}
+ private static synchronized SpeciesData updateCache(String types,
SpeciesData d) {
+SpeciesData d2;
+assert((d2 = CACHE.get(types)) == null || d2.isPlaceholder());
+assert(!d.isPlaceholder());
+CACHE.put(types, d);
+return d;
+}
Global synchronization is the performance smell, and this looks to be
potential scalability bottleneck (it sends shivers down my spine every
time I see static synchronized in the same line. That is not to
mention synchronizing on placeholder looks suspicious and error-prone.
Do you need help rewriting this with CHM?
-Aleksey.
signature.asc
Description: OpenPGP digital signature
___
mlvm-dev mailing list
mlvm-dev@openjdk.java.net
http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
Re: review request (L): JDK changes for 7023639: JSR 292 method handle invocation needs a fast path for compiled code
On 07/22/2012 04:16 AM, John Rose wrote: P.S. If there's something you don't like in one of the files, let us know. As I noted before, we can (and will) roll more adjustments into the next push. I have a question about $PREPARED_FORMS there. It looks like it is not used anywhere in the code, should we eliminate it whatsoever? If not, then I have trouble understanding if we need to explicitly override CHM defaults, especially concurrency level there. (I know Doug pushes back on specialized constructors in CHMv8 because some of the parameters are meaningless there). -Aleksey. signature.asc Description: OpenPGP digital signature ___ mlvm-dev mailing list mlvm-dev@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
Re: review request (L): JDK changes for 7023639: JSR 292 method handle invocation needs a fast path for compiled code
John, Both webrevs point to jdk changes. Where are hotspot changes? Vladimir John Rose wrote: On Jul 13, 2012, at 2:41 AM, John Rose wrote: Here is that webrev: http://cr.openjdk.java.net/~jrose/7023639/webrev.jdk.00/ These are the changes to JDK code that accompany the JVM changes already under review. I have updated both webrevs to their final contents, as follows: http://cr.openjdk.java.net/~jrose/7023639/webrev.01/ http://cr.openjdk.java.net/~jrose/7023639/webrev.jdk.01/ In the same folder are incremental webrevs, for the record. Thanks to all reviewers for their help. — John P.S. If there's something you don't like in one of the files, let us know. As I noted before, we can (and will) roll more adjustments into the next push. ___ mlvm-dev mailing list mlvm-dev@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
Re: review request (L): JDK changes for 7023639: JSR 292 method handle invocation needs a fast path for compiled code
On Jul 23, 2012, at 2:27 AM, Aleksey Shipilev wrote: Global synchronization is the performance smell, and this looks to be potential scalability bottleneck (it sends shivers down my spine every time I see static synchronized in the same line. That is not to mention synchronizing on placeholder looks suspicious and error-prone. I believe the code is correct. The purpose is to load as many distinct species of BMH tuples as the application type schema requires. The use of placeholders is patterned after the JVM's internal class loading algorithm. The code does not need to be scalable, because the number of entries in the cache is small (order of 10-100) and scales only with type schema complexity, not workload complexity. So in this case, static synchronized is the correct choice. Do you need help rewriting this with CHM? No, but thanks for the help! — John___ mlvm-dev mailing list mlvm-dev@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
Re: review request (L): JDK changes for 7023639: JSR 292 method handle invocation needs a fast path for compiled code
On Jul 23, 2012, at 11:15 AM, Vladimir Kozlov wrote: Both webrevs point to jdk changes. Where are hotspot changes? Oops, fixed. Please try again: http://cr.openjdk.java.net/~jrose/7023639/webrev.01/ http://cr.openjdk.java.net/~jrose/7023639/webrev.jdk.01/ — John ___ mlvm-dev mailing list mlvm-dev@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
Re: review request (L): JDK changes for 7023639: JSR 292 method handle invocation needs a fast path for compiled code
On Jul 13, 2012, at 2:41 AM, John Rose wrote: Here is that webrev: http://cr.openjdk.java.net/~jrose/7023639/webrev.jdk.00/ These are the changes to JDK code that accompany the JVM changes already under review. I have updated both webrevs to their final contents, as follows: http://cr.openjdk.java.net/~jrose/7023639/webrev.01/ http://cr.openjdk.java.net/~jrose/7023639/webrev.jdk.01/ In the same folder are incremental webrevs, for the record. Thanks to all reviewers for their help. — John P.S. If there's something you don't like in one of the files, let us know. As I noted before, we can (and will) roll more adjustments into the next push.___ mlvm-dev mailing list mlvm-dev@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
Re: review request (L): JDK changes for 7023639: JSR 292 method handle invocation needs a fast path for compiled code
On Jul 18, 2012, at 1:43 AM, Aleksey Shipilev wrote: This feels outright wrong: +static MapString, Data CACHE = new IdentityHashMap(); I couple of questions pops out in my mind: 1. Is this code supposed to be thread-safe? It is then wrong to use unguarded IdentityHashMap without external synchronization. 2. Do we really need a second .get() check, if code is not thread-safe anyway? This code allows two threads to call put() on the same key anyway. 3. Do the $types *need* to be interned? Or is this the premature optimization (gone wrong, btw)? You are right on all points. Fixed. I would rather like to see CHM.putIfAbsent()-based cache on plain non-interned strings there. Even if interned strings are required, we could intern them on successful map update, not every time we look up the key. Yes. Using a IdentityHashMap here is an anti-pattern; sorry to propagate the bad example. Thanks, — John ___ mlvm-dev mailing list mlvm-dev@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
Re: review request (L): JDK changes for 7023639: JSR 292 method handle invocation needs a fast path for compiled code
(sorry for not replying to original message, just subscribed)
This feels outright wrong:
+static MapString, Data CACHE = new IdentityHashMap();
+
+static Data get(String types) {
+final String key = types.intern();
+Data d = CACHE.get(key);
+if (d == null) {
+d = make(types);
+Data e = CACHE.get(key);
+if (e != null) {
+d = e;
+} else {
+CACHE.put(key, d);
+}
+}
+return d;
+}
I couple of questions pops out in my mind:
1. Is this code supposed to be thread-safe? It is then wrong to use
unguarded IdentityHashMap without external synchronization.
2. Do we really need a second .get() check, if code is not thread-safe
anyway? This code allows two threads to call put() on the same key anyway.
3. Do the $types *need* to be interned? Or is this the premature
optimization (gone wrong, btw)?
I would rather like to see CHM.putIfAbsent()-based cache on plain
non-interned strings there. Even if interned strings are required, we
could intern them on successful map update, not every time we look up
the key.
-Aleksey.
___
mlvm-dev mailing list
mlvm-dev@openjdk.java.net
http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
Re: review request (L): JDK changes for 7023639: JSR 292 method handle invocation needs a fast path for compiled code
On Fri, Jul 13, 2012 at 5:41 AM, John Rose john.r.r...@oracle.com wrote:
On Jul 11, 2012, at 5:53 PM, John Rose wrote:
As some of you have noticed, Chris Thalinger, Michael Haupt, and I have been
working on the mlvm patches [1] for JEP-160 [2] for several months. These
changes make method handles more optimizable. They refactor lots of magic
out of the JVM and into more manageable Java code.
…
An associated webrev for hotspot-comp/jdk/ will be posted soon; it is
already present on mlvm-dev for the curious to examine. (This change set
also deletes a lot of old code.)
Here is that webrev:
http://cr.openjdk.java.net/~jrose/7023639/webrev.jdk.00/
These are the changes to JDK code that accompany the JVM changes already
under review.
There are 2900 LOC deleted, and 7000 LOC added. Key changes:
- method handle behavior is fully represented by LambdaForm objects
- chained method handles (including adapter method handles) are gone
- an ASM-based bytecode spinner compiles LambdaForms when they warm up
- bound method handles are compactly represented without boxing
- the private symbol-resolution interface to the JVM (MemberName) is improved
- unit tests have more systematic coverage
- a number of minor bugs are fixed
This is implementation work. No public Java APIs are changed, although the
javadoc is slightly edited for clarity.
Please have a look.
— John
John,
I had a few quick comments on your webrev. Some of these might be
considered premature or unnecessary optimization, so take them as you
wish.
src/share/classes/java/lang/invoke/BoundMethodHandle.java
On lines 131-132: Just out of curiosity, why use pos+1 in one line
versus 1+pos on the next? Would it be worthwhile to extract pos+1 to a
local int end to save some bytecode?
MethodHandle bindArgument(int pos, char basicType, Object value) {
int end = pos + 1;
MethodType type = type().dropParameterTypes(pos, end);
LambdaForm form = internalForm().bind(basicType, end, tcount());
if (basicType == 'I' !(value instanceof Integer)) {
// Cf. ValueConversions.unboxInteger
value = ValueConversions.primitiveConversion(Wrapper.INT,
value, true);
}
return cloneExtend(type, form, basicType, value);
}
On lines 202-212: Similarly would it be worthwhile to extract types()
to a local String within internalValues()?
final Object internalValues() {
String types = types();
Object[] boundValues = new Object[types.length()];
for (int i = 0; i boundValues.length; ++i) {
try {
switch (types.charAt(i)) {
case 'L': boundValues[i] = argL(i); break;
case 'I': boundValues[i] = argI(i); break;
case 'F': boundValues[i] = argF(i); break;
case 'D': boundValues[i] = argD(i); break;
case 'J': boundValues[i] = argJ(i); break;
default : throw new InternalError(unexpected type:
+ types.charAt(i));
}
} catch (Throwable t) {
throw new InternalError(t);
}
}
return Arrays.asList(boundValues);
}
On line 464: Is there any reason CACHE should not be final?
static final MapString, Data CACHE = new IdentityHashMap();
On lines 473-486: Do the accesses and mutations of CACHE need to be
thread safe or are the uses assumed to be safe by other means?
On lines 778-784: Should this use types.charAt(int) rather than
toCharArray() to copy the array?
private static String makeSignature(String types, boolean ctor) {
StringBuilder buf = new StringBuilder(SIG_INCIPIT);
for (int i = 0; i types.length(); ++i) {
buf.append(typeSig(types.charAt(i)));
}
return buf.append(')').append(ctor ? V : BMH_SIG).toString();
}
Would it be worthwhile for clarity to define constants to represent
the BaseTypes ('B', 'C', 'D', 'F', 'I', 'J', 'S', 'Z') and ObjectType
('L') per JVM spec section 4.3.2 for use throughout these
java.lang.invoke.* implementation classes (e.g. the various
switch/case in BoundMethodHandle, and DirectMethodHandle.bindArgument,
throughout LambdaForm)? I assume that anyone touching this code would
be familiar with these types so constants may actually reduce clarity
for some developers.
src/share/classes/java/lang/invoke/CountingMethodHandle.java
On line 40: Should instance field target be final?
private final MethodHandle target;
src/share/classes/java/lang/invoke/Invokers.java
On lines 322-326: Is this err.initCause(ex) needed since the cause is
already passed to the 2 arg InternalError constructor (maybe leftover
from before the constructor was added in JDK8)?
} catch (Exception ex) {
throw new InternalError(Exception while resolving inexact
invoke, ex);
}
src/share/classes/java/lang/invoke/MemberName.java
On lines 81-90
Re: review request (L): JDK changes for 7023639: JSR 292 method handle invocation needs a fast path for compiled code
On 07/14/2012 02:02 PM, David Schlosnagle wrote:
On Fri, Jul 13, 2012 at 5:41 AM, John Rose john.r.r...@oracle.com
wrote:
On Jul 11, 2012, at 5:53 PM, John Rose wrote:
As some of you have noticed, Chris Thalinger, Michael Haupt, and I
have been working on the mlvm patches [1] for JEP-160 [2] for
several months. These changes make method handles more
optimizable. They refactor lots of magic out of the JVM and into
more manageable Java code.
…
An associated webrev for hotspot-comp/jdk/ will be posted soon; it
is already present on mlvm-dev for the curious to examine. (This
change set also deletes a lot of old code.)
Here is that webrev:
http://cr.openjdk.java.net/~jrose/7023639/webrev.jdk.00/
These are the changes to JDK code that accompany the JVM changes
already under review.
There are 2900 LOC deleted, and 7000 LOC added. Key changes:
- method handle behavior is fully represented by LambdaForm objects
- chained method handles (including adapter method handles) are gone
- an ASM-based bytecode spinner compiles LambdaForms when they warm up
- bound method handles are compactly represented without boxing
- the private symbol-resolution interface to the JVM (MemberName)
is improved
- unit tests have more systematic coverage
- a number of minor bugs are fixed
This is implementation work. No public Java APIs are changed,
although the javadoc is slightly edited for clarity.
Please have a look.
— John
John,
Hi David,
I hijack the thread ...
I had a few quick comments on your webrev. Some of these might be
considered premature or unnecessary optimization, so take them as you
wish.
src/share/classes/java/lang/invoke/BoundMethodHandle.java
On lines 131-132: Just out of curiosity, why use pos+1 in one line
versus 1+pos on the next? Would it be worthwhile to extract pos+1 to a
local int end to save some bytecode?
MethodHandle bindArgument(int pos, char basicType, Object value) {
int end = pos + 1;
MethodType type = type().dropParameterTypes(pos, end);
LambdaForm form = internalForm().bind(basicType, end, tcount());
if (basicType == 'I' !(value instanceof Integer)) {
// Cf. ValueConversions.unboxInteger
value = ValueConversions.primitiveConversion(Wrapper.INT,
value, true);
}
return cloneExtend(type, form, basicType, value);
}
Usually you should not care about this kind of change, micro-optimizing
is useless.
Or this code is not called enough and you don't care or this code is hot
and
the JIT will do what should be done and because the JIT may inline
the code of dropParameterTypes and bind(), it will better optimize that you
because it will be able to share more sub-expressions.
Now just for fun, if you count the number of bytecodes, I think your
proposed code
is bigger because 'end' is the local variable 4 and the is no
istore_4/iload_4 so
the compiler will use the generic iload/istore which takes 2 bytes.
so pos + 1 is iload_1, iconst_1, iadd, so the current version use 6
bytecodes,
your version use iload_1, iconst_1, iadd, istore 4 + iload 4 * 2= 9
bytecodes
On lines 202-212: Similarly would it be worthwhile to extract types()
to a local String within internalValues()?
final Object internalValues() {
String types = types();
Object[] boundValues = new Object[types.length()];
for (int i = 0; i boundValues.length; ++i) {
try {
switch (types.charAt(i)) {
case 'L': boundValues[i] = argL(i); break;
case 'I': boundValues[i] = argI(i); break;
case 'F': boundValues[i] = argF(i); break;
case 'D': boundValues[i] = argD(i); break;
case 'J': boundValues[i] = argJ(i); break;
default : throw new InternalError(unexpected type:
+ types.charAt(i));
}
} catch (Throwable t) {
throw new InternalError(t);
}
}
return Arrays.asList(boundValues);
}
default case should never be called, so don't 'optimize' for it.
On line 464: Is there any reason CACHE should not be final?
static final MapString, Data CACHE = new IdentityHashMap();
this one may be important, static final = const for the VM
On lines 473-486: Do the accesses and mutations of CACHE need to be
thread safe or are the uses assumed to be safe by other means?
On lines 778-784: Should this use types.charAt(int) rather than
toCharArray() to copy the array?
private static String makeSignature(String types, boolean
ctor) {
StringBuilder buf = new StringBuilder(SIG_INCIPIT);
for (int i = 0; i types.length(); ++i) {
buf.append(typeSig(types.charAt(i)));
}
return buf.append(')').append(ctor ? V :
BMH_SIG).toString();
Re: review request (L): JDK changes for 7023639: JSR 292 method handle invocation needs a fast path for compiled code
BoundMethodHandle.java: I am concern that BMH subclass names are looks like constants names: BMH_LLI. In several places you have BMH constants and variables: + final ClassBoundMethodHandle BMH = BoundMethodHandle.class; + static final String BMH = java/lang/invoke/BoundMethodHandle; ThrowExceptionsTest.java: empty diffs ValueConversionsTest.java: remove commented print statement if it is not needed: -System.out.println(arrayType.getSimpleName()); +//System.out.println(arrayType.getSimpleName()); Vladimir John Rose wrote: On Jul 11, 2012, at 5:53 PM, John Rose wrote: As some of you have noticed, Chris Thalinger, Michael Haupt, and I have been working on the mlvm patches [1] for JEP-160 [2] for several months. These changes make method handles more optimizable. They refactor lots of magic out of the JVM and into more manageable Java code. … An associated webrev for hotspot-comp/jdk/ will be posted soon; it is already present on mlvm-dev for the curious to examine. (This change set also deletes a lot of old code.) Here is that webrev: http://cr.openjdk.java.net/~jrose/7023639/webrev.jdk.00/ These are the changes to JDK code that accompany the JVM changes already under review. There are 2900 LOC deleted, and 7000 LOC added. Key changes: - method handle behavior is fully represented by LambdaForm objects - chained method handles (including adapter method handles) are gone - an ASM-based bytecode spinner compiles LambdaForms when they warm up - bound method handles are compactly represented without boxing - the private symbol-resolution interface to the JVM (MemberName) is improved - unit tests have more systematic coverage - a number of minor bugs are fixed This is implementation work. No public Java APIs are changed, although the javadoc is slightly edited for clarity. Please have a look. — John ___ mlvm-dev mailing list mlvm-dev@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
