Re: RFR(L): 8139885: implement JEP 274: enhanced method handles

[Paul Sandoz]

> On 19 Nov 2015, at 22:16, Michael Haupt  wrote:
> 
> Hi John, Paul,
> 
> thanks for your reviews - they have helped me polish the code and 
> documentation some more and add some more tests. The result is at 
> http://cr.openjdk.java.net/~mhaupt/8139885/webrev.02 
> 
> 

I quickly browsed it. Looks ok.


> I noticed both of you emphasised how the Streams API helps the implementation 
> - I second that. Streams "made my day" there. :-)
> 

:-)


>> 3458  * The loop handle's result type is the same as the sole loop 
>> variable's, i.e., the result type of {@code init}.
>> 
>> s/variable’s/variable ?
> 
> No; the genitive is intentional (result type = loop variable type, rather 
> than result type = loop variable).
> 

Doh, yes, now that I read it again.

Paul.


signature.asc
Description: Message signed with OpenPGP using GPGMail
___
mlvm-dev mailing list
mlvm-dev@openjdk.java.net
http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev

Re: RFR(L): 8139885: implement JEP 274: enhanced method handles

[Paul Sandoz]

HI Michael,

Nice work, not easy this area!


It may be worth considering, as a separate issue, updating the example section 
in JavaDoc to be under @apiNote.

I have a mild preference to maintain the 80 char limit for JavaDoc, to me it’s 
easier to read. For code i don’t mind a 100 or more limit.

Add @since 9 to new public methods.


MethodType
—

 470 /** Replace the last arrayLength parameter types with the component 
type of arrayType.
 471  * @param arrayType any array type
 472  * @param arrayLength the number of parameter types to change
 473  * @return the resulting type
 474  */

 475 /*non-public*/ MethodType asSpreaderType(Class arrayType, int pos, 
int arrayLength) {

 476 assert(parameterCount() >= arrayLength);

 477 int spreadPos = pos;

Might as well adjust the JavaDoc for the new pos parameter (as you have done 
for asCollectorType).


MethodHandles
—

 898  * @param spreadArgPos the position (zero-based index) in the argument 
list at which spreading should start.
 899  * @param arrayType usually {@code Object[]}, the type of the array 
argument from which to extract the spread arguments
 900  * @param arrayLength the number of arguments to spread from an 
incoming array argument
 901  * @return a new method handle which spreads an array argument at a 
given position,
 902  * before calling the original method handle
 903  * @throws NullPointerException if {@code arrayType} is a null 
reference
 904  * @throws IllegalArgumentException if {@code arrayType} is not an 
array type,
 905  * or if target does not have at least
 906  * {@code arrayLength} parameter types,
 907  * or if {@code arrayLength} is negative,
 908  * or if the resulting method handle's type would have
 909  * too many parameters

For the new asSpreader method, what are the constraints for spreadArgPos? What 
happens if < 0 or >= arrayLength for example?

Same applies to asCollector.

This is not in your patch but the assert in the following method is redundant 
(as reported by the IDE, it’s really handy sometimes apply the patch and view 
via diffs in the IDE), as DirectMethodHandle is not a subtype of 
BoundMethodHandle:

MethodHandle viewAsType(MethodType newType, boolean strict) {
// No actual conversions, just a new view of the same method.
// Note that this operation must not produce a DirectMethodHandle,
// because retyped DMHs, like any transformed MHs,
// cannot be cracked into MethodHandleInfo.
assert viewAsTypeChecks(newType, strict);
BoundMethodHandle mh = rebind();
assert(!((MethodHandle)mh instanceof DirectMethodHandle));
return mh.copyWith(newType, mh.form);
}


 958 /**
 959  * Determines if a class can be accessed from the lookup context 
defined by this {@code Lookup} object. The
 960  * static initializer of the class is not run.
 961  *
 962  * @param targetClass the class to be accessed
 963  *
 964  * @return the class that has been accessed

Has it really been accessed by “accessClass” or just checked that it can be 
accessed?


 965  *
 966  * @throws IllegalAccessException if the class is not accessible 
from the lookup class, using the allowed access
 967  * modes.
 968  * @exception SecurityException if a security manager is present 
and it
 969  *  refuses access
 970  */
 971 public Class accessClass(Class targetClass) throws 
IllegalAccessException {



Great use of streams in MethodHandles.loop and good correlation with the 
specified steps in the JavaDoc to that in the code. Really clear and well 
specified.



3458  * The loop handle's result type is the same as the sole loop 
variable's, i.e., the result type of {@code init}.

s/variable’s/variable ?


3488  * // assume MH_initZip, MH_zipPred, and MH_zipStep are handles to the 
above methods
3489  * MethodHandle loop = MethodHandles.doWhileLoop(MH_initZip, 
MH_zipPred, MH_zipStep);
3490  * List a = Arrays.asList(new String[]{"a", "b", "c", "d"});
3491  * List b = Arrays.asList(new String[]{"e", "f", "g", "h"});
3492  * List zipped = Arrays.asList(new String[]{"a", "e", "b", 
"f", "c", "g", "d", "h"});
3493  * assertEquals(zipped, (List) loop.invoke(a.iterator(), 
b.iterator()));
3494  * }

You don’t need to create an explicit array and pass that into Arrays.asList. 
Same for other related methods.



MethodHandleImpl
—

1720 static MethodHandle makeLoop(Class tloop, List targs, 
List tvars, List init,
1721  List step, 
List pred, List fini) {
1722 MethodHandle[] ainit = 
toArrayArgs(init).toArray(MH_ARRAY_SENTINEL);
1723 MethodHandle[] astep = 
toArrayArgs(step).toArray(MH_ARRAY_SENTINEL);
1724 MethodHandle[] apred = 

Re: RFR(L): 8139885: implement JEP 274: enhanced method handles

[Michael Haupt]

Hi John, Paul,

thanks for your reviews - they have helped me polish the code and documentation 
some more and add some more tests. The result is at 
http://cr.openjdk.java.net/~mhaupt/8139885/webrev.02

I noticed both of you emphasised how the Streams API helps the implementation - 
I second that. Streams "made my day" there. :-)

I'm replying below to those of your remarks I didn't address in the new webrev.

> Am 19.11.2015 um 05:25 schrieb John Rose :
> My usual practice, though, is to move argument checking and error reporting 
> code out of line, away from the main logic.  I think this is good style, and 
> it gives the JIT a little bit (a very little bit) of help.

In the most generic loop combinator, the tests are somewhat dispersed over the 
implementation because they depend on results that are not readily available at 
the beginning of the method's execution. I haven't put them all in one test 
method because I prefer early failure with clear messages over possibly hard to 
interpret failure due to inconsistencies in mid-run.

> One wishful item:  It would be nice if the javadoc examples could be 
> integrated into JavaDocExamplesTest.java.  I see that is messy since we are 
> now using TestNG instead of JUnit.  The point of JavaDocExamplesTest was to 
> make it easy to "sync" the javadoc with the examples, by having one place to 
> copy-and-paste the javadoc.

I've filed this RFE for that: https://bugs.openjdk.java.net/browse/JDK-8143343

> Am 19.11.2015 um 11:45 schrieb Paul Sandoz :
> I have a mild preference to maintain the 80 char limit for JavaDoc, to me 
> it’s easier to read. For code i don’t mind a 100 or more limit.

The formatting in the files I've touched is inconsistent; I'll stick with the 
120 character limit for both code and Javadoc.

> 3458  * The loop handle's result type is the same as the sole loop 
> variable's, i.e., the result type of {@code init}.
> 
> s/variable’s/variable ?

No; the genitive is intentional (result type = loop variable type, rather than 
result type = loop variable).

> I guess in the future there may be ample opporunity to specialize the generic 
> “loop” mechanism with LambdaForms?

Yep: https://bugs.openjdk.java.net/browse/JDK-8143211

Best,

Michael

-- 

 
Dr. Michael Haupt | Principal Member of Technical Staff
Phone: +49 331 200 7277 | Fax: +49 331 200 7561
Oracle Java Platform Group | LangTools Team | Nashorn
Oracle Deutschland B.V. & Co. KG, Schiffbauergasse 14 | 14467 Potsdam, Germany
  Oracle is committed to developing 
practices and products that help protect the environment

___
mlvm-dev mailing list
mlvm-dev@openjdk.java.net
http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev

Re: RFR(L): 8139885: implement JEP 274: enhanced method handles

[Michael Haupt]

Hi Vladimir,

> Am 17.11.2015 um 13:08 schrieb Vladimir Ivanov :
> Awesome! Looks really good, Michael!

thank you very much.

> src/java.base/share/classes/java/lang/invoke/MethodHandles.java:
> if (!hasPrivateAccess()
> || (specialCaller != lookupClass()
> +   // ensure non-abstract methods in superinterfaces can 
> be special-invoked
> +&& !(refc != null && refc.isInterface() && 
> refc.isAssignableFrom(specialCaller))
> && !(ALLOW_NESTMATE_ACCESS &&
> 
> Is it a fix for an existing bug? If it's the case, I'd prefer to see it as a 
> stand alone fix.

This is the implementation of the findSpecial capability to bind non-abstract 
methods in super interfaces, as mentioned in the JEP document.

> src/java.base/share/classes/java/lang/invoke/MethodHandleImpl.java:
> +static final MethodHandle MH_looper;
> +static final MethodHandle MH_countedLoopPred;
> +static final MethodHandle MH_countedLoopStep;
> +static final MethodHandle MH_iteratePred;
> +static final MethodHandle MH_initIterator;
> +static final MethodHandle MH_iterateNext;
> +static final MethodHandle MH_tryFinallyExec;
> +static final MethodHandle MH_tryFinallyVoidExec;
> 
> I think you have to adjust that part since Claes made MH constant 
> initialization lazy.

Yes, Claes had given me a heads-up early on (thanks Claes!), and the merge was 
easy enough. The new webrev is here: 
http://cr.openjdk.java.net/~mhaupt/8139885/webrev.01/

> Also, does it make sense to provide bytecode intrinsics for tryFinally and 
> loop combinators in InvokerBytecodeGenerator to compile them in more 
> efficient bytecode shapes. If yes, please, file corresponding RFEs.

Absolutely; this is on my to-do list anyway. 
https://bugs.openjdk.java.net/browse/JDK-8143211

Best,

Michael

-- 

 
Dr. Michael Haupt | Principal Member of Technical Staff
Phone: +49 331 200 7277 | Fax: +49 331 200 7561
Oracle Java Platform Group | LangTools Team | Nashorn
Oracle Deutschland B.V. & Co. KG, Schiffbauergasse 14 | 14467 Potsdam, Germany
  Oracle is committed to developing 
practices and products that help protect the environment

___
mlvm-dev mailing list
mlvm-dev@openjdk.java.net
http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev

Re: RFR(L): 8139885: implement JEP 274: enhanced method handles

[John Rose]

On Nov 13, 2015, at 8:39 AM, Michael Haupt  wrote:
> 
> Dear all,
> 
> please review this change.
> RFE: https://bugs.openjdk.java.net/browse/JDK-8139885
> Corresponding JEP: https://bugs.openjdk.java.net/browse/JDK-8130227
> Webrev: http://cr.openjdk.java.net/~mhaupt/8139885/webrev.00/ 
> 

Looks great.  One bug:

+static boolean isVoid(Class t) {
+return t == void.class || t == Void.class;
+}

I think there's nothing in the spec that calls for java.lang.Void to get 
special processing for try/finally.
If I'm right, get rid of isVoid; just check for void.class not Void.class.

I'd prefer to see reuse of misMatchedTypes() or newIllegalArgumentException() 
instead of err().  I would prefer to push formatting should into the 
error-producing subroutine.  This can be cleaned up later.  OTOH, the 
complexity of string concat instructions will be going down with Aleksey's 
work, so maybe I shouldn't be so sensitive to inline concat stuff.

My usual practice, though, is to move argument checking and error reporting 
code out of line, away from the main logic.  I think this is good style, and it 
gives the JIT a little bit (a very little bit) of help.

In that vein, the argument checking function foldArgumentChecks should stay 
private, shouldn't it?

I'm impressed to see how useful the Stream API is for doing the loop combinator.

Testing is good.  I'm glad to see new BigArity cases.

One wishful item:  It would be nice if the javadoc examples could be integrated 
into JavaDocExamplesTest.java.  I see that is messy since we are now using 
TestNG instead of JUnit.  The point of JavaDocExamplesTest was to make it easy 
to "sync" the javadoc with the examples, by having one place to copy-and-paste 
the javadoc.

Anyway, reviewed, conditional on that one bug fix.

— John

___
mlvm-dev mailing list
mlvm-dev@openjdk.java.net
http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev

Re: RFR(L): 8139885: implement JEP 274: enhanced method handles

[Vladimir Ivanov]

Awesome! Looks really good, Michael!

src/java.base/share/classes/java/lang/invoke/MethodHandles.java:
 if (!hasPrivateAccess()
 || (specialCaller != lookupClass()
+   // ensure non-abstract methods in 
superinterfaces can be special-invoked
+&& !(refc != null && refc.isInterface() && 
refc.isAssignableFrom(specialCaller))

 && !(ALLOW_NESTMATE_ACCESS &&

Is it a fix for an existing bug? If it's the case, I'd prefer to see it 
as a stand alone fix.


src/java.base/share/classes/java/lang/invoke/MethodHandleImpl.java:
+static final MethodHandle MH_looper;
+static final MethodHandle MH_countedLoopPred;
+static final MethodHandle MH_countedLoopStep;
+static final MethodHandle MH_iteratePred;
+static final MethodHandle MH_initIterator;
+static final MethodHandle MH_iterateNext;
+static final MethodHandle MH_tryFinallyExec;
+static final MethodHandle MH_tryFinallyVoidExec;

I think you have to adjust that part since Claes made MH constant 
initialization lazy.


Also, does it make sense to provide bytecode intrinsics for tryFinally 
and loop combinators in InvokerBytecodeGenerator to compile them in more 
efficient bytecode shapes. If yes, please, file corresponding RFEs.


Best regards,
Vladimir Ivanov

On 11/13/15 7:39 PM, Michael Haupt wrote:

Dear all,

please review this change.
RFE: https://bugs.openjdk.java.net/browse/JDK-8139885
Corresponding JEP: https://bugs.openjdk.java.net/browse/JDK-8130227
Webrev: http://cr.openjdk.java.net/~mhaupt/8139885/webrev.00/

Thanks,

Michael

--

Oracle 
Dr. Michael Haupt | Principal Member of Technical Staff
Phone: +49 331 200 7277 | Fax: +49 331 200 7561
OracleJava Platform Group | LangTools Team | Nashorn
Oracle Deutschland B.V. & Co. KG, Schiffbauergasse 14 | 14467 Potsdam,
Germany
Green Oracle    Oracle is committed to
developing practices and products that help protect the environment




___
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