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

Add runtime using GraalJS on TruffleRuby #107

Merged
merged 7 commits into from
Oct 19, 2021

Conversation

eregon
Copy link
Contributor

@eregon eregon commented Sep 23, 2021

This PR adds a new runtime for ExecJS when running on TruffleRuby in JVM mode and Graal.js is available.
So it needs truffleruby+graalvm and also export TRUFFLERUBYOPT=--jvm --polyglot. The standalone truffleruby does not include Graal.js.

This new backend evaluates everything in-process much like the mini_racer backend.
ExecJS's Context are correctly isolated by using Truffle inner contexts.
All tests pass: https://github.com/eregon/execjs/runs/3688209195?check_suite_focus=true#step:9:72

@eregon eregon force-pushed the graaljs-runtime-ready branch from 6c16c8f to 13d3c5d Compare September 30, 2021 20:26
@eregon
Copy link
Contributor Author

eregon commented Sep 30, 2021

This PR is ready now.
@pixeltrix @byroot Could you review/merge this PR? (or who may I ping for review?)

@byroot
Copy link
Member

byroot commented Oct 1, 2021

I'll try to find some time today.

Copy link
Member

@byroot byroot left a comment

Choose a reason for hiding this comment

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

Looks quite straight forward to me. I'm not a fan to have to include a -head version on CI because GitHub Actions have no proper "allow failures" feature, but I suppose that's ok.

I'll leave it a couple days for @pixeltrix to chime in if he wants to, if not I'll happily merge, I can't release though.

lib/execjs/graaljs_runtime.rb Outdated Show resolved Hide resolved
@eregon
Copy link
Contributor Author

eregon commented Oct 1, 2021

I'm not a fan to have to include a -head version on CI because GitHub Actions have no proper "allow failures" feature, but I suppose that's ok.

I could switch to the the non-head version when 21.3 is released on October 19.
I'd suggest to merge the PR before though, and I can replace when released.

@byroot
Copy link
Member

byroot commented Oct 1, 2021

I could switch to the the non-head version when 21.3 is released on October 19.

Sounds good.

@byroot
Copy link
Member

byroot commented Oct 19, 2021

@eregon could you rebase? I fixed the jscore test suite on master.

@byroot
Copy link
Member

byroot commented Oct 19, 2021

And sorry for the delay, this slipped my mind.

@eregon
Copy link
Contributor Author

eregon commented Oct 19, 2021

@byroot Will do, but I'll wait for the release which should come later today, so then we can test against a release in CI :)

* Use Truffle inner contexts to provide correct isolation between ExecJS::Context
* To run the tests:
TRUFFLERUBYOPT="--jvm --polyglot" bundle exec rake test:graaljs TESTOPTS="--seed=0 --verbose"
* Full command without subprocess:
TRUFFLERUBYOPT="--jvm --polyglot" jt -u jvm-js ruby -w -Ilib:test -I $PWD/vendor/bundle/truffleruby/*/gems/rake-13.0.1/lib $PWD/vendor/bundle/truffleruby/*/gems/rake-13.0.1/lib/rake/rake_test_loader.rb test/test_execjs.rb --seed=0 --verbose
* Try command:
TRUFFLERUBYOPT="--jvm --polyglot" jt -u jvm-js ruby -Ilib -rexecjs -e 'p ExecJS.eval("2 + 3")'
* If the GraalJSRuntime is not used it is not necessarily an issue,
  as it e.g. fall back to an ExternalRuntime like node.
@eregon eregon force-pushed the graaljs-runtime-ready branch from db2490c to 313ffba Compare October 19, 2021 17:34
@eregon eregon force-pushed the graaljs-runtime-ready branch from 732a5e3 to 8d4412a Compare October 19, 2021 17:58
@eregon
Copy link
Contributor Author

eregon commented Oct 19, 2021

@byroot byroot merged commit 39118e2 into rails:master Oct 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants