This is great! The code is so much better now. +1
/M > On 15 Jan 2016, at 16:38, Attila Szegedi <szege...@gmail.com> wrote: > > Please review the second take on JDK-8133299 "Nashorn Java adapters should > not early bind to functions" at > <http://cr.openjdk.java.net/~attila/8133299/webrev.jdk9.01> for > <https://bugs.openjdk.java.net/browse/JDK-8133299> > > I made some further changes: > > - while the code would now be able to invoke just about any Nashorn callable > object as an implementation for an adapter method, I restricted it to > ScriptFunction only following a discussion with Sundar. The old adapter only > supported ScriptFunction, so there's no backwards compatibility issue. > - I have also made the bytecode of the adapter methods much smaller, by > extracting some oft-repeated bytecode sequences into static methods on > JavaAdapterServices (wrapThrowable(), unsupported()). > - I reworked the way setGlobal works, again drastically reducing the bytecode > size and moving all of the logic into JavaAdapterServices. > JavaAdapterServices.setGlobal() now sets the necessary global and returns a > Runnable that when run() will restore the previous global. The bytecode thus > no longer has any logic for dealing with globals; it just invokes > JAS.setGlobal(), stores the returned Runnable, and invokes run() on it in > finally block. > - The generator also detects when the INVOKEDYNAMIC dyn:call would need more > than 255 slots for its parameter list, and introduces a Nashorn-compliant > (Object callee, Object this, Object[] args) variable arity invocation in its > place. > - Added a fairly comprehensive test suite. > > With these changes, this work should be complete. > > It’s probably hard to review the changes in JavaAdapterBytecodeGenerator just > by looking at the diff, as the changes are quite significant, amounting to a > partial rewrite. It’s probably easier to apply the diff and review the > resulting new code of the class. A fairly good way to review the effect of > these changes is to add “-pc” to line 102 of JavaAdapterTest.java and run it > with and without the rest of the changes applied to see what kind of code is > being generated. > > Attila. > >> On Jan 4, 2016, at 2:48 PM, Attila Szegedi <szege...@gmail.com> wrote: >> >> >> On Mon, Jan 4, 2016 at 5:13 AM, Sundararajan Athijegannathan >> <sundararajan.athijegannat...@oracle.com> wrote: >> I'm yet to look at the code. Is it possible to use any callable for methods? >> >> For members of a delegate ScriptObject, yes. The adapter constructors still >> only accept either a ScriptObject or a ScriptFunction, but if you pass in an >> object, its members implementing methods can now be arbitrary callables. >> >> If so, do we have tests to cover those cases (like JSObject "functions", >> DynamicMethods etc.)? In particular, sandbox tests to make sure can't get >> any more privilege by implementing an interface (for eg. binding sensitive >> Java method as function implementing interface method and making sure it >> gets SecurityException when interface method is called). >> >> We generate separate adapter classes for every ProtectionDomain, so I'm >> quite confident everything is secure, but it's better to not be >> overconfident when it comes to security… surely we can add such tests. Do >> you have some go-to sensitive functionality for testing in this fashion? >> >> FWIW, I actually started writing some tests for other functionality; what I >> have so far is tests for correct conversion of primitive return values as >> well as tests for try/finally for restoring old Global being executed when >> UnsupportedOperationException is thrown. (All tests I wrote pass, so the >> code as it is now is correct for these.) I wanted to write few more tests, >> specifically for binding to vararg functions. >> >> Actually, I realized that there's one deficiency in the current code that >> wasn't there before my changes, namely since the CALL operation takes >> explicit callee and this arguments, the adapter code will actually fail to >> generate a delegate for a method with more than 253 parameters. I might need >> to introduce a special case for this, to have them packed into an Object >> array in this case and do a Nashorn vararg invocation… >> >> Speaking of which, anyone knows why is LinkerCallSite.ARGLIMIT set to 250 >> and not 253? >> >> Attila. >> >> >> Thanks, >> -Sundar >> >> >> On 1/2/2016 8:31 PM, Attila Szegedi wrote: >> Please review JDK-8133299 "Nashorn Java adapters should not early bind to >> functions" at <http://cr.openjdk.java.net/~attila/8133299/webrev.jdk9> for >> <https://bugs.openjdk.java.net/browse/JDK-8133299> >> >> See implementation notes in >> <https://bugs.openjdk.java.net/browse/JDK-8133299?focusedCommentId=13883269&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13883269> >> >> Also note that this changeset is based on current tip (rev 1584, >> [da397aea8ada]) and is as such independent of the change sets for >> JDK-8144917 and JDK-8144919 that are still pending review. >> >> Thanks, >> Attila. >> >> >
