Looks good. Wow, this must have been a lot of work!
Am 2016-01-19 um 15:00 schrieb Attila Szegedi:
Yes. If we want to backport, we’ll create an adjusted patch. FWIW, backporting
would depend on backporting Hannes’ long removal too, as the generated bytecode
now has logic compatible with that (never passing longs as primitive parameters
to functions; always boxing them to Long).
I'm in the process of backporting the long removal and some earlier
bugs, so that won't be a problem.
Hannes
* could there be a simple JS test like?
var obj = {
run: function() {
print("hello")
}
}
var r = new java.lang.Runnable(obj);
// "hello" printed
r.run()
obj.run = function() { print("world") }
// "world" printed
r.run()
I was actually thinking about whether to add such a test… We didn’t have tests
that codified it otherwise for the old adapter. The current behavior feels more
natural, but I’m still not convinced whether I want to acknowledge it in a
test. Tests don’t set the behavior in stone, but they’re still indicative of
expectations. To be honest, for me, the fact that adapters will now reflect
changes in the delegate is incidental. The simplification and efficiency
improvements in the adapter code were my primary goals.
Attila.
-Sundar
On 1/15/2016 9:08 PM, Attila Szegedi 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.
