Hello Stefan, On 27/05/20 10:06 pm, Stefan Bodewig wrote: > Hi all > > I've changed master to also download the things required to use GraalVM > JavaScript as language and changed JavaxScriptRunner to make it work a > bit better. > > This uses > https://github.com/graalvm/graaljs/blob/master/docs/user/ScriptEngine.md > which is not the preferred way of using GraalVM, but the least intrusive > one for us. An alternative could be to add another ScriptRunner that > explicitly uses the GraalVM Context[1], but I'm not sure it is worth the > effort. Should we? > > The change to JavaxScriptRunner is to disable Graal's security > restrictions as without that the <script>s would be more restricted than > before. Without allowHostAccess the scripts wouldn't even be allowed to > call public methods on Java objects unless they had a specific > annotation. I think this is required for what we want to allow <script> > to do. We might be able to make this configurable, but I don't see a > convenient way to do it. It would probably be easier to provide GraalVM > specific variants of the script family of tasks and types if we want to > do something like this. > > I have not enabled full Nashorn compatibility[2]. This means some > JavaScript code will need to be changed. For example our own "squares" > example fails as it invokes setMessage(Number) on the Echo task - > Nashorn seems to wrap this in a String in order to match the method's > signature. Should we enable full Nashorn compatibility?
I had a look at the related commits: https://github.com/apache/ant/commit/19ed75326ffcecaa08a2e22778bb01f7ded70d1c https://github.com/apache/ant/commit/be9b424d1237fb368be81da764bdd065481007c1 https://github.com/apache/ant/commit/cb8c8f106b2ef4b5bc9f7643ab38e4e5096fc70e They overall look fine to me. I have one suggestion/question related to this - instead of this[1] property, we can perhaps just enable full Nashorn compatibility using the "polyglot.js.nashorn-compat" property set to true? The reason I say this is, the property name itself describes its nature unlike the "allowAllAccess" property. I think we should enable full Nashorn compatibility because IMO the whole goal of switching the GraalJS script engine implementation is to allow existing Nashorn based scripts to work fine. The graaljs (internal) testsuite uses this approach[2] in their testcases to get a nashorn compatible script engine. I think what we should do additionally, irrespective of the property that we set, is starting Java 15, log a WARN message which states that the "javascript" script type no longer has a script engine shipped in the JDK and hence we are internally using GraalJS as a short term option and perhaps in the long run these "javascript" based scripts will no longer be supported unless a specific engine is made available in the classpath by the user? > Finally, the unit test we've disabled for Java15 fails with GraalVM on > any version of Java as it is (a) really really fast and (b) the compiler > is rather slow (i.e. takes about as long as the Nashorn compiler). > > The test runs the same script 20 times and expects a pre-compiled > version to be faster than the non-compiled one. In my personal > non-scientific benchmarks when comparing the non-compiled versions > GraalVM is about four times faster than Nashorn for a single > execution. It also seems to become faster if the script is executed > repeatedly. > > Compilation takes more than twenty times the time it takes to run the > non-compiled script (and running the compiled script isn't really much > faster than the non-compiled version) so we'd need to run a lot more > scripts to make the test pass. I'd probably disable the timing > comparison when we detect we are running GraalVM. Normally I would have said that any test that relies on timings like these is probably going to be flaky and should be disabled. However, in this specific case, it runs this operation N number of times and the "compiled" attribute is a user exposed attribute meant to imply that a compiled script needs to be used and we don't have any other way of asserting that the compiled script was indeed used (unless we tweak some code to internally expose this detail). So I think it's fine for this test to be run on all version except when GraalJS gets used. Having said that, it's interesting that a compiled script in GraalJS takes more time to run than a non-compiled script. Perhaps something to check later and report if necessary. [1] https://github.com/apache/ant/commit/be9b424d1237fb368be81da764bdd065481007c1#diff-8f893edfeaf46967019e8f5844ffe3dcR202 [2] https://github.com/graalvm/graaljs/blob/360a07b1c60feceea5c03d0d3c276c5f7eb5299b/graal-js/src/com.oracle.truffle.js.scriptengine.test/src/com/oracle/truffle/js/scriptengine/test/TestUtil.java#L58 -Jaikiran
