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

review (cleanup) boot -> standard JRuby extension loading #5205

Merged
merged 28 commits into from Jun 28, 2018

Conversation

Projects
None yet
2 participants
@kares
Copy link
Member

kares commented Jun 1, 2018

... while looking into jruby loading performance and making JI native, managed to review the boot process.
there was some legacy and obvious things that we do not need to parse or can afford to lazy load instead.

the most interesting part (some cleanup noise through the commits) is an extension loading support :
JRuby::Util.load_ext(...) or the more official JRuby.load_ext (ideally after require 'jruby')

avoiding JRuby.runtime, and JI proxy loading the bootstrap Java class, JRuby is able to boot 5+% faster.

note, that we already delivered a ~ 5% improvement in 9.2.0 (over 9.1) this is measured compared to 9.2.0

also this is very useful internally since self-reflecting (JI loading org.jruby pieces) leads to a weird internal state (there were already some work-arounds on that from e.g. subclasses returning weird junk due UNDEF).

NOTE: things to consider

  • do we force users extension authors to do a require 'jruby'
    now, its auto-loaded from kernel which was likely not intentional - should only auto require 'java'
  • in which case we shall think about NOT auto require 'jruby'
    (most exts tend to have an explicit require 'jruby' or use the new load_ext mechanism)
  • need to propagate load_ext changes to gems we tend to boot on every run (due RGs) :
    jruby-openssl, jruby-readline, psych, jar-dependencies
    ... and get them to propagate releases (so this PR delivers the promised loading speed improvement)

@kares kares added this to the JRuby 9.2.1.0 milestone Jun 1, 2018

@kares kares self-assigned this Jun 1, 2018

@kares kares requested review from headius and enebo Jun 1, 2018

@kares kares changed the title cleanup jruby boot -> improve performance review (cleanup) boot -> standard JRuby extension loading Jun 1, 2018

@@ -1717,7 +1717,6 @@ private void initBuiltins() {
addLazyBuiltin("java.rb", "java", "org.jruby.javasupport.Java");
addLazyBuiltin("jruby.rb", "jruby", "org.jruby.ext.jruby.JRubyLibrary");
addLazyBuiltin("jruby/util.rb", "jruby/util", "org.jruby.ext.jruby.JRubyUtilLibrary");
addLazyBuiltin("jruby/type.rb", "jruby/type", "org.jruby.ext.jruby.JRubyTypeLibrary");

This comment has been minimized.

Copy link
@kares

kares Jun 1, 2018

Author Member

org.jruby.ext.jruby.JRubyTypeLibrary class was gone for quite some time - as explained in the commit

Class realFacadeClass = Class.forName("org.jruby.util.SunSignalFacade");
return (SignalFacade)realFacadeClass.newInstance();
} catch(Throwable e) {
return org.jruby.util.SunSignalFacade.class.newInstance();

This comment has been minimized.

Copy link
@kares

kares Jun 1, 2018

Author Member

somehow ended up hitting this Class.forName spot (at least) twice while 'naively' profiling.
likely a false alarm, but I still did review the class and its signal.rb pieces ...

@kares

This comment has been minimized.

Copy link
Member Author

kares commented Jun 15, 2018

// ping @headius and @enebo for a review - so we can start setting up JRuby.load_ext in default gems

@enebo

enebo approved these changes Jun 15, 2018

Copy link
Member

enebo left a comment

requiring both 'require "java"' or 'require "jruby"' is semantically required even though they both incidentally worked.

You have some code with if and else if not joined with {. Same for catch.

Depending on autoload feels weird to me but we run rails so it is ok by me I guess.

@kares kares force-pushed the ji-lazy-pp2 branch from 7b8d196 to 4ad609e Jun 18, 2018

@kares

This comment has been minimized.

Copy link
Member Author

kares commented Jun 18, 2018

OK, cleaned up the code-style.

requiring both 'require "java"' or 'require "jruby"' is semantically required even though they both incidentally worked.

at this point than we could remove require 'jruby' from booting up on all JRuby boots?
its unfortunate ... esp. since most of it is covered by the internal JRuby::Util helpers
the downside is mostly parts (user exts) doing JRuby.runtime without the require ...

@kares kares merged commit 459262a into master Jun 28, 2018

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/travis-ci/push The Travis CI build could not complete due to an error
Details

@kares kares deleted the ji-lazy-pp2 branch Jun 28, 2018

kares added a commit that referenced this pull request Oct 23, 2018

only require 'java', users should explicitly require 'jruby'
seems to makes sense as using JRuby internals make the runtime "dirty"
e.g. ObjectSpace will find invalid class pieces such as for UNDEF

still, its kind of a small "break" in term-of backwards compatibility
although most scripts floating around do the explicit require 'jruby'
before scripting with internals such as `JRuby.runtime`

left-over from #5205 as noted at #5233
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.