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

Eliminate all magic builtin libraries #5591

Merged
merged 11 commits into from Feb 11, 2019

Conversation

Projects
None yet
3 participants
@headius
Copy link
Member

commented Feb 6, 2019

JRuby has for a long time had special "built-in" libraries that did not live on the filesystem but appeared to do so. We replaced "so" with "jar" for extensions and registered about a dozen special libraries this way, mostly equivalent to MRI's native extensions.

However, bootsnap hooks into require, and expects that the files it sees loading will actually exist somewhere on the filesystem. This may be a bit fragile, if any impl has more than filesystem-based resources (as in JRuby, where we support sources from several different formats of URL), but by moving these builtin libraries to real files it allows bootsnap to work without failing to load those libraries.

I have taken two approaches to making these files no longer "built-in":

  • For libraries that are loaded at boot or where we always want to load our internal extension, I replaced the builtin and and requires of it with the boot of the actual library.
  • For libraries that have other code, I have incorporated the direct load of our internal extension into those files.

This relates to Shopify/bootsnap#162 and (probably) eliminates the need for Shopify/bootsnap#200.

TODO:

  • Green build
  • Get stdlib tweaks back into our stdlib fork
  • Use a better mechanism for loading these libraries? (@kares?)
  • Test with bootsnap

headius added some commits Feb 5, 2019

Move builtin libraries to direct loads or external files.
This is for Shopify/bootsnap#162, and should allow all our
"built in" libraries to have their require paths cached like any
other library.

This eliminates the need for Shopify/bootsnap#200.

Other changes that come along with this:

* Remainder of Continuation logic moved to the .rb library and
  removed from native.
* net/protocol native library removed.
* NKFLibrary removed since it only loaded NKF class.
* digest/digest/bubblebabble.rb removed since it does not exist
  in MRI (it's just digest/bubblebabble.rb).
* Methods for builting libs are now deprecated and do nothing.
  This include the "internal_libraries" method added for
  Shopify/bootsnap#200.
* Replaces "etc.so" with "etc" in standard libraries, since the
  .so version will only search for exts (which will be .jar files
  in JRuby)

@headius headius requested review from enebo and kares Feb 6, 2019

@headius headius added this to the JRuby 9.2.6.0 milestone Feb 6, 2019

headius added some commits Feb 6, 2019

@headius

This comment has been minimized.

Copy link
Member Author

commented Feb 6, 2019

There's a commit in this PR that works around an apparent problem with the "loaded features" index internally. After a modification is made directly to $LOADED_FEATURES, manually-added "provides" no longer appear to work. See #5590.

@kares
Copy link
Member

left a comment

could this one be post-poned for at least 9.2.7, please? (looking from the phone here)

would like to look into this deeper ... JRuby::Util had a new ext loading mechanism. which we started using recently as a prefered way.

we should try to avoid JI self-reflection (JRuby.runtime etc) during boot, preferably also during JRuby's std libraries loading since it leaves an inconsistent internal state (due UNDEF and similar). the 'design' idea was to expose the minimal iface from JRuby::Util ... which now, in part, seems to be thrown away.

@headius

This comment has been minimized.

Copy link
Member Author

commented Feb 6, 2019

@kares Yeah the new mechanism is what I had in mind. I think almost all the changes I made should work ok with that. Java, JRuby, and JRubyUtil libraries will continue to be initialized from Ruby.java since they need to be there for other pieces, but the remaining libraries should be able to use your ext method. Sound ok?

I don't think there's any reason this has to be in 9.2.6 if we've got a 9.2.7 coming before end of month. Clock's ticking on getting a stable release for Rails 6.

JRuby::Util actually didn't change much at all. The only thing that is now deprecated is the listing of internal libraries that bootsnap was going to use to work around our buitins. They could still release that patch (Shopify/bootsnap#162) or we could just say JRuby 9.2.x is needed for bootsnap to work.

@headius

This comment has been minimized.

Copy link
Member Author

commented Feb 6, 2019

I updated all external libs to use load_ext except nkf, because the library did nothing but initialize the class and I deleted it. It could be restored.

@headius

This comment has been minimized.

Copy link
Member Author

commented Feb 6, 2019

@kares Need you to review ae9d88d for sure; I made this change just to get better logging of the exception that was breaking various ext loads, but once I made it the breaking loads started to work again. I suspect something about your re-raising as NameError was screwing up some extension load/fallback logic.

headius added some commits Feb 6, 2019

Restore a minimal RubyContinuation class for third-party users.
Specifically, I know that the current racc "cparse" ext release
uses RubyContinuation directly. This has been patched on racc
master, but there has been no release for quite some time.

See tenderlove/racc#91.
@headius

This comment has been minimized.

Copy link
Member Author

commented Feb 6, 2019

I have tested with bootsnap in Rails 5.2.0, and all seems to be working properly. I added some logging to prove that bootsnap is properly caching paths to all required libraries:

$ JRUBY_OPTS=-w jruby -S rails s
used bootsnap cache for active_support/dependencies
used bootsnap cache for thread
used bootsnap cache for concurrent/map
used bootsnap cache for concurrent/constants
used bootsnap cache for concurrent/synchronization
used bootsnap cache for concurrent/utility/engine
used bootsnap cache for concurrent/synchronization/abstract_object
used bootsnap cache for concurrent/utility/native_extension_loader
used bootsnap cache for concurrent/concurrent_ruby.jar
used bootsnap cache for concurrent/synchronization/mri_object
...

I also see our former built-in libraries in this list, like cgi/escape. There are probably tests in bootsnap we could run but it works now where it didn't work for me before.

@kares
Copy link
Member

left a comment

looking much better,
great to see some old unused code gone and and explicit CatchThrow class

Show resolved Hide resolved core/src/main/ruby/jruby/kernel.rb
Show resolved Hide resolved core/src/main/ruby/jruby/kernel/file.rb
@@ -0,0 +1,2 @@
# Load built-in nkf library
org.jruby.ext.nkf.RubyNKF.createNKF(JRuby.runtime)

This comment has been minimized.

Copy link
@kares

kares Feb 6, 2019

Member

you could add a static RubyNKF.load(org.jruby.Ruby) which delegates to createNKF
its not loaded on boot and (guessing NKF) is rarely used thus not really blocker

@enebo enebo merged commit a67a9c5 into master Feb 11, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@kares

This comment has been minimized.

Copy link
Member

commented Feb 11, 2019

not all comments where addressed esp. JRuby.runtime.posix.native? will cause self-reflection again.
which we kind of tried avoiding during normal boot ... as a resolution for another reported issue ...

@enebo

This comment has been minimized.

Copy link
Member

commented Feb 11, 2019

@kares I am happy to revert this unless @headius or you can just ammend by removing the reflection.

@headius

This comment has been minimized.

Copy link
Member Author

commented Feb 11, 2019

@kares You're right, I'll restore those uses of JRuby::Util. I did not realize the purpose was to avoid extra reflection at boot, which I agree can get problematic (not to mention impacting startup time).

@kares

This comment has been minimized.

Copy link
Member

commented Feb 11, 2019

sounds good, I'll try to come up with a way of testing that the usual boot does not cause much Java proxy-ing

headius added a commit that referenced this pull request Feb 11, 2019

Address review comments from @kares in #5591.
* Restore uses of JRuby::Util to avoid extra reflection
* Use static "load" mechanism to load RubyNKF.

@headius headius deleted the no_builtins branch Feb 11, 2019

@headius

This comment has been minimized.

Copy link
Member Author

commented Feb 11, 2019

Everything has been addressed in d64c07f.

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.