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 Map<String, 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 there is an implementation of equals(Object), but on
lines 593-603 there is a commented out implementation of
equals(Object) and hashCode(). Are the commented out versions for
future usage? There should probably be an implementation of hashCode()
consistent with equals, although MemberName may not be used in a hash
based structure. Also, it might be worthwhile to collocate the "TO BE"
implementations with near the current ones with a comment.
On lines 268-278: Should the isObjectPublicMethod() method also match
public Object methods getClass(), notify(), notifyAll(), wait(),
wait(long), and wait(long, int) or are those not intended to be used?
src/share/classes/java/lang/invoke/SimpleMethodHandle.java
On lines 41-45: 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 type2 = type().dropParameterTypes(pos, end);
LambdaForm form2 = internalForm().bind(basicType, end, 0);
return BoundMethodHandle.bindSingle(type2, form2, basicType, value);
}
Thanks,
Dave
_______________________________________________
mlvm-dev mailing list
mlvm-dev@openjdk.java.net
http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
