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 Map<String, 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();
> }
toCharArray() may be more efficient because when you iterate on
the result, the VM can remove the bound checks. It's harder
to do the same with String.charAt() because String are respresented
with an offset and a length different, note that this is not true anymore
for jdk8, a recent patch remove sharing between strings (to make them
more optimizable).
Anyway, the real question here, how often this code will be called and
even if this code is called often, it's perhaps not a bottleneck.
> 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;
yes.
>
>
> 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);
see above.
> }
>
>
> Thanks,
> Dave
cheers,
Rémi
_______________________________________________
mlvm-dev mailing list
mlvm-dev@openjdk.java.net
http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
