Paul, thanks for the feedback!
See my answers inline.
Updated webrevs:
http://cr.openjdk.java.net/~vlivanov/8037209/webrev.03/
http://cr.openjdk.java.net/~vlivanov/8038261/webrev.02/
On 4/4/14 3:33 PM, Paul Sandoz wrote:
Hi,
Reviews of two patches in the queue.
Paul.
http://cr.openjdk.java.net/~vlivanov/8037209/webrev.02/
Looks good, though I don't claim to understand all the nuances around casts and
JVM/bytecode correctness. Minor stuff below.
InvokerByteCodeGenerator:
--
Unused method:
static boolean match(MemberName member, MethodHandle fn) {
if (member == null || fn == null) return false;
return member.equals(fn.internalMemberName());
}
Done.
MethodHandleImpl
--
If you are gonna remove the weakly typed wrapper methods for array access, you
might as well remove USE_WEAKLY_TYPED_ARRAY_ACCESSORS and it's use?
- // Weakly typed wrappers of Object[] accessors:
- static Object getElementL(Object a, int i) { return
getElementL((Object[])a, i); }
- static void setElementL(Object a, int i, Object x) {
setElementL((Object[]) a, i, x); }
- static Object getElementL(Object arrayClass, Object a, int i)
{ return getElementL((Class<?>) arrayClass, (Object[])a, i); }
- static void setElementL(Object arrayClass, Object a, int i, Object x)
{ setElementL((Class<?>) arrayClass, (Object[])a, i, x); }
-
Or otherwise for expediency just leave it in until the array improvements patch
follows? IMHO better to be consistent either way.
Moved to 8038261.
VerifyType
--
Typo in docs:
* <p>
* The primitive type 'void' does not interconvert with any other type,
* even though it is legal to drop any type from the stack and "return
void".
* The stack effects, though are difference between void and any other
type,
* so it is safer to report a non-trivial conversion.
s/difference/different
Done.
http://cr.openjdk.java.net/~vlivanov/8038261/webrev.01/
To re-iterate this is a nice improvement over the previous approach.
InvokerByteCodeGenerator
--
For this specialization:
if (defc == ArrayAccessor.class &&
match(member, ArrayAccessor.OBJECT_ARRAY_GETTER)) {
mv.visitInsn(Opcodes.AALOAD);
} else if (defc == ArrayAccessor.class &&
match(member, ArrayAccessor.OBJECT_ARRAY_SETTER)) {
mv.visitInsn(Opcodes.AASTORE);
} else if (member.isMethod()) {
IIUC all stuff on the stack is correctly placed for the substitution of the
invokestatic instruction to ArrayAccessor.set/getElementL with an aastore/load
instruction.
Correct. setElementL/getElementL are interchangeable with aastore/aaload
bytecode instructions w.r.t. stack layout.
This makes we wonder if there is a more general approach for direct or
direct-like MHs to be visited and provide their own snippets of ASM producing
code.
It's an interesting idea. I'm not aware of existing logic to achieve it,
but I'll definitely experiment to see how it shapes out. There's an
alternative approach using intrinsics, but it doesn't introduce direct
coupling between MHs and bytecode shapes.
Is it worthwhile introducing such direct coupling for a specific case, as that
tends to increase the inter-connective complexity of the code. How much
performance gain is achieved?
Setters/getters for primitive arrays can be special-cased in the same
manner, but these special cases don't buy much. Accessors
(ArrayAccessor.getElement*/setElement*) are very simple anyway. I
haven't seen any significant performance difference on octane.
I'll experiment with that further.
The last two re-assigments are never used and are reassigned again at the top
of the loop:
// Update cached form name's info in case an intrinsic spanning
multiple names was encountered.
name = lambdaForm.names[i];
member = name.function.member();
rtype = name.function.methodType().returnType();
I did it intentionally. There were bugs when cached values become stale
due to processing of multi-name intrinsics and they were erroneously
used. There's a refactoring of how intrinsics are implemented waiting in
the queue, so I'd like to leave it as is for now.
Best regards,
Vladimir Ivanov
