On Jul 14, 2012, at 5:43 AM, Rémi Forax wrote:
> On 07/14/2012 02:02 PM, David Schlosnagle wrote:
>> John,
>
> Hi David,
> I hijack the thread …
Thanks for the comments, Remi & David.
>> 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?
> 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.
Remi is correct.
>> default case should never be called, so don't 'optimize' for it.
Yes.
>>
>> 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
Yes, that was an oversight. Fixed.
>> 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?
This was a bug. We have added appropriate synchronization.
>> On lines 778-784: Should this use types.charAt(int) rather than
>> toCharArray() to copy the array?...
>
> ...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.
It's not a bottleneck. BMH species are few in number.
>> 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.
That is my sense of it. The only value to making named constants here would be
a slight increase in static checking, but I don't think it's worthwhile.
>> src/share/classes/java/lang/invoke/CountingMethodHandle.java
>>
>> On line 40: Should instance field target be final?
>> private final MethodHandle target;
>
> yes.
Even better: During the review I have been finding more dead code to
eliminate. This whole class has been finalized, in the sense of "deleted".
>> 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)?
It is a leftover. Thanks for spotting it.
>> 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.
This was an oversight due to some hasty coding. Thanks, fixed.
>>
>> 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?
No, those aren't relevant. Since the method is private and its name
is long enough already, I'm leaving it as is.
Thanks again!
— John
_______________________________________________
mlvm-dev mailing list
mlvm-dev@openjdk.java.net
http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
