Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace Nashorn with GraalVM #1857

Merged
merged 44 commits into from
Jun 10, 2019
Merged

Replace Nashorn with GraalVM #1857

merged 44 commits into from
Jun 10, 2019

Conversation

plata
Copy link
Collaborator

@plata plata commented Feb 23, 2019

No description provided.

@plata
Copy link
Collaborator Author

plata commented Mar 4, 2019

@madoar I don't know how to replace the ScriptObjectMirror by Value while still using our InteractiveScriptSession functional interface. Do you have an idea?

@madoar
Copy link
Collaborator

madoar commented Mar 4, 2019

Can you give me an example where you face the problems?

@plata
Copy link
Collaborator Author

plata commented Mar 4, 2019

E.g. in ShortcutRunner.

@madoar
Copy link
Collaborator

madoar commented Mar 4, 2019

My IntelliJ doesn't seem to be able to find the polyglot package (for import org.graalvm.polyglot.Value;).
Do you have an idea?

@madoar
Copy link
Collaborator

madoar commented Mar 4, 2019

With the import working it should be working with:

    public void run(ShortcutDTO shortcutDTO, List<String> arguments, Consumer<Exception> errorCallback) {
        final InteractiveScriptSession interactiveScriptSession = scriptInterpreter.createInteractiveSession();

        interactiveScriptSession.eval("include([\"engines\", \"wine\", \"shortcuts\", \"reader\"]);",
                ignored -> interactiveScriptSession.eval("new ShortcutReader()", output -> {
                    final Value shortcutReader = (Value) output;

                    shortcutReader.invokeMember("of", shortcutDTO);
                    shortcutReader.invokeMember("run", arguments);
                }, errorCallback), errorCallback);
    }

@plata
Copy link
Collaborator Author

plata commented Mar 5, 2019

I had tried this before already but I got: PolyglotMap cannot be cast to PolyglotValue. I think you only get Value if you use Context.

@madoar
Copy link
Collaborator

madoar commented Mar 5, 2019

After pulling the newest changes from upstream I got my branch working and did some tests.
You're right I stumbled across the same error like you (the one with PolyglotMap and Value).

After debugging the code a little I identified the location of the problem. The problem is caused by the "cast" in: https://github.com/graalvm/graaljs/blob/0385428b1c2f2cc4212a85612435c2ea0f105488/graal-js/src/com.oracle.truffle.js.scriptengine/src/com/oracle/truffle/js/scriptengine/GraalJSScriptEngine.java#L205

As far as I found out polyglotContext.eval(source) returns a Value object, which is the then converted to a PolyglotMap object by the .as(Object.class) call.

I think that the best solution is to change our graal ScriptInterpreter to use a graal Context instead of a ScriptEngine.

@plata
Copy link
Collaborator Author

plata commented Mar 5, 2019

I've also tried that and got "Java is not defined". If you want to have a look, I can make a pr with my current state.

madoar and others added 4 commits March 5, 2019 13:03
- make PhoenicisScriptEngine an interface
- cleanup ShortcutRunner
- add some comments
- change the constructor argument of the JSAScriptEngine class
@plata
Copy link
Collaborator Author

plata commented Mar 5, 2019

Currently, I get

TypeError: INVOKE on JavaObject[com.oracle.truffle.js.javaadapters.org.phoenicis.engines.Engine@4c66c7 (com.oracle.truffle.js.javaadapters.org.phoenicis.engines.Engine)] failed due to: java.lang.ClassCastException: Cannot convert '(0) []'(language: JavaScript, type: Array) to Java type 'java.lang.String': Unsupported target type.

during an installation.

@madoar
Copy link
Collaborator

madoar commented Mar 5, 2019

Does this occur when "casting" the Value to Installer or when accessing one of its methods?
Which application are you executing?

@plata
Copy link
Collaborator Author

plata commented Mar 5, 2019

Notepad++. The installation starts just fine and installs the correct Wine version but then it crashes. I'm assuming that it happens in Wine#run() but no idea how I could debug this.

@plata
Copy link
Collaborator Author

plata commented Mar 5, 2019

I can confirm my Wine#run() theory.

@madoar
Copy link
Collaborator

madoar commented Mar 5, 2019

I just got:

Exception in thread "pool-3-thread-3" java.lang.ClassCastException: class org.graalvm.polyglot.Value cannot be cast to class java.util.Map (org.graalvm.polyglot.Value is in module org.graalvm.sdk of loader 'app'; java.util.Map is in module java.base of loader 'bootstrap')
	at [email protected]/org.phoenicis.engines.EngineSettingsManager.lambda$fetchAvailableEngineSettings$1(EngineSettingsManager.java:75)
	at [email protected]/org.phoenicis.scripts.engine.PhoenicisInteractiveScriptSession.eval(PhoenicisInteractiveScriptSession.java:34)
	at [email protected]/org.phoenicis.scripts.interpreter.BackgroundScriptInterpreter.lambda$createInteractiveSession$1(BackgroundScriptInterpreter.java:43)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
	at java.base/java.lang.Thread.run(Thread.java:834)

when removing an installed application and container

@plata
Copy link
Collaborator Author

plata commented Mar 5, 2019

EngineSettingsManager l.75. Not quite sure how to handle this.

@madoar
Copy link
Collaborator

madoar commented Mar 5, 2019

About your previous problem:
I have no clue, where exactly the problem is located. The error message indicates that a javascript array is passed to the Java method where a String is required.

The problem for me is that the responsible script code for this stuff spans 3+ files, which include both the engine "implementation" and "object" code. After following two or three invocations I don't know anymore if the run invocation now targets the "implementation" object, the "object" object or maybe even the Java interface?

@plata
Copy link
Collaborator Author

plata commented Mar 5, 2019

I added some debug output but everything looked ok. The call hierarchy is not that difficult really: InstallerScriptWine(Object)#run → Java Engine interface → Wine(Implementation)#run. The problem occurs when the Java interface is called from Wine(Object)#run because this is the place where we go from Javascript to Java.

@plata
Copy link
Collaborator Author

plata commented Mar 5, 2019

I found it. It's a bug in scripts. I will create a PR.

@madoar
Copy link
Collaborator

madoar commented Jun 5, 2019

Ok, I've experimented a bit. I can reproduce the exception(s). Interestingly they only seem to occur when executing Phoenicis from the IDE. If I install it via a built deb archive I don't get the error message and the application starts like it should.

I believe the reason for the exception is a missing parameter which needs to be passed to the JVM. Somehow we set this parameter/option correctly inside the deb installer but at least I don't do that inside the IDE.

@plata
Copy link
Collaborator Author

plata commented Jun 6, 2019

That's an interesting find. Nevertheless, I think it's something which should be fixed before we can merge this.

@madoar
Copy link
Collaborator

madoar commented Jun 9, 2019

I've searched some more but still can't find the reason for the error messages. The only think I found out is that the errors are thrown when parsing a javascript regex. For example an exception is thrown for the lines:

I can't find any significant differences between my IntelliJ setup and the options the installer script sets.

@qparis
Copy link
Member

qparis commented Jun 9, 2019

If you replace ALL_MODULE_PATH with jdk.crypto.ec,java.base,javafx.base,javafx.web,javafx.media,javafx.graphics,javafx.controls,java.naming,java.sql,java.scripting,jdk.scripting.nashorn,jdk.internal.vm.ci,jdk.internal.vm.compiler,org.graalvm.truffle,java.desktop,java.prefs,java.xml

it should work

Copy link
Collaborator

@madoar madoar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can confirm replacing ALL_MODULE_PATH with jdk.crypto.ec,java.base,javafx.base,javafx.web,javafx.media,javafx.graphics,javafx.controls,java.naming,java.sql,java.scripting,jdk.scripting.nashorn,jdk.internal.vm.ci,jdk.internal.vm.compiler,org.graalvm.truffle,java.desktop,java.prefs,java.xml solves the problem.

@qparis do you know how the two settings differ? I mean I thought that ALL_MODULE_PATH would auto-add all modules located on the module path?

@qparis
Copy link
Member

qparis commented Jun 9, 2019

Actually java.desktop,java.prefs,java.xml are not needed

@madoar Seems that some module are not detected automatically with ALL-MODULE-PATH

@qparis
Copy link
Member

qparis commented Jun 9, 2019

@plata Feel free to merge whenever you think we are stable enough.
Next step is to get rid of nashorn compatibility

@plata
Copy link
Collaborator Author

plata commented Jun 9, 2019

Do we need to update the IntelliJ IDEA documentation and and the flatpak? If so, what's the final list of required modules?

@qparis
Copy link
Member

qparis commented Jun 9, 2019

The list of package is the same than than in the packaging script.
I have no idea concerning flatpak. Do you pass the module list somewhere?

@qparis
Copy link
Member

qparis commented Jun 10, 2019

@plata Can this be merged?

@plata
Copy link
Collaborator Author

plata commented Jun 10, 2019

I'm doing a quick test then I will merge, yes.

@qparis
Copy link
Member

qparis commented Jun 10, 2019

Does it mean that it will not compile on Ubuntu?

@plata
Copy link
Collaborator Author

plata commented Jun 10, 2019

This is just for the IntelliJ IDEA documentation. Phoenicis itself is compiled with Java 11 already since b1b1ad5.

@qparis
Copy link
Member

qparis commented Jun 10, 2019

Ok

@plata
Copy link
Collaborator Author

plata commented Jun 10, 2019

Apart from that: @madoar is using Ubuntu or Linux Mint so we should be fine.

@plata plata merged commit 240e682 into master Jun 10, 2019
@plata plata deleted the graalvm branch June 10, 2019 09:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants