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
Merged

Conversation

kares
Copy link
Member

@kares 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 June 1, 2018 13:55
@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");
Copy link
Member Author

@kares kares Jun 1, 2018

Choose a reason for hiding this comment

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

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();
Copy link
Member Author

Choose a reason for hiding this comment

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

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
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

Copy link
Member

@enebo enebo left a comment

Choose a reason for hiding this comment

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

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
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
@kares kares deleted the ji-lazy-pp2 branch June 28, 2018 11:17
kares added a commit that referenced this pull request Oct 23, 2018
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants