Optimize feature loading when require is called with absolute .rb path

[rwstauner]

If require is called with an absolute path like /path/to/thing.rb we can try that as is first rather than trying .rb.rb and .rb.so first.

After enabling bootsnap's LoadPathCache on TruffleRuby we noticed that it was still doing a lot of ENOENT stat calls and found that it was trying odd-looking suffixes first:

$ bin/jt ruby --ruby.log-feature-location -e 'require "#{ENV["PWD"]}/tmp/rws.rb"' 2>&1 | grep rws.rb
  'require "#{ENV["PWD"]}/tmp/rws.rb"'
[ruby] INFO: starting search from -e:1 for feature /rws-data/repos/truffleruby-ws/truffleruby/tmp/rws.rb...
[ruby] INFO: trying /rws-data/repos/truffleruby-ws/truffleruby/tmp/rws.rb.rb...
[ruby] INFO: trying /rws-data/repos/truffleruby-ws/truffleruby/tmp/rws.rb.so...
[ruby] INFO: trying /rws-data/repos/truffleruby-ws/truffleruby/tmp/rws.rb...
[ruby] INFO: found in /rws-data/repos/truffleruby-ws/truffleruby/tmp/rws.rb
found rws.rb

With this change it only has to try one file (instead of three):

$ bin/jt ruby --ruby.log-feature-location -e 'require "#{ENV["PWD"]}/tmp/rws.rb"' 2>&1 | grep rws.rb
  'require "#{ENV["PWD"]}/tmp/rws.rb"'
[ruby] INFO: starting search from -e:1 for feature /rws-data/repos/truffleruby-ws/truffleruby/tmp/rws.rb...
[ruby] INFO: trying /rws-data/repos/truffleruby-ws/truffleruby/tmp/rws.rb...
[ruby] INFO: found in /rws-data/repos/truffleruby-ws/truffleruby/tmp/rws.rb
found rws.rb

I considered making this check "if the path has an extension", but since the next condition looks specifically for .so and does something extra I thought leaving it as .rb might be safest.

I tried to dig through the history of the file for extra context on the rest of this function.
The only one that stood to me was 8d10af1 but I don't fully understand the scenario there.

I also wasn't sure if there are tests that were worth adding for this... have any pointers?

What do you think? Does this make sense? Is there other opportunity for improvement to this function?

[eregon]

Yes, this makes a lot of sense.
Of course for user-written require calls it's quite rare they specify the .rb at the end, but for Bootsnap that's actually the common case.

[eregon]

I wonder if this case is actually needed semantically, i.e. can require ever load anything which is not a .rb or a .so/bundle/dll?
It seems not:

$ echo 'p :a' > a
$ ruby -I. -e 'require "a"'
cannot load such file -- a (LoadError)

$ mv a a.rb
$ ruby -I. -e 'require "a"'
:a

There are specs for this kind of stuff in spec/ruby/core/kernel/shared/require.rb.
It would be good to add one for this case if there is not already one.

Given we already had this bug before, and there is no tag in spec/tags/core/kernel/require_tags.txt that sounds like it it seems there is no spec for this already, so could you add one, and then remove this part?

[eregon]

Ah but my example above uses $LOAD_PATH and findFeatureWithAndWithoutExtension() is not used for $LOAD_PATH search.
However the behavior is as I said above:

$ ruby -v --disable-gems -I. -e 'require "#{Dir.pwd}/a"'
ruby 3.2.2 (2023-03-30 revision e51014f9c0) [x86_64-linux]
-e:1:in `require': cannot load such file -- /home/eregon/tmp/loaddir/a (LoadError)

$ ruby -v --disable-gems -I. -e 'require "#{Dir.pwd}/a"'
truffleruby 24.0.0-dev-54e96355, like ruby 3.2.2, GraalVM CE Native [x86_64-linux]
:a

i.e. there we are not compatible with CRuby, and this shows we don't need to try the path as-is if it doesn't end in .rb, .so or .bundle.
But we still need to check if it ends in .bundle.
So how about merging the first 2 ifs, or the opposite, make it a if .rb, if .so, if macOS && .bundle?

[eregon]

BTW I also noticed that files like a.rb.rb don't get loaded on CRuby by require "#{Dir.pwd}/a.rb", only by require "#{Dir.pwd}/a.rb.rb". So we could also structure this like:

if hasExtension(path)
  on macOS replace .so by .bundle first (translateIfNativePath does that)
  try as-is
else
  try + .rb
  try + .CEXT_EXTENSION
end

[rwstauner]

Thanks, that makes sense to me!

I was curious if there was a way to test that the java code wasn't trying extra calls (like "a.rb.rb").

We have a spec for absolute paths with .rb
I added one for absolute path with .so on darwin
and 2 for absolute paths without extensions (rb and C).

Are we missing anything?

[rwstauner]

It looks like it doesn't like trying to load C-named files that are just text files:

Kernel.require (path resolution) loads c-extension file when passed absolute path without extension when no .rb is present ERROR
RuntimeError: /rws-data/repos/truffleruby-ws/truffleruby/spec/ruby/fixtures/code/a/load_fixture.so: file too short

I'm not surprised, but also not sure how these files have been used before.
Do we want to keep the test and inspect the error message (which will assert that we attempted to load the right file)? Or is there another way to write that test?

[eregon]

It looks like CRuby doesn't pass those specs: https://github.com/oracle/truffleruby/actions/runs/6364893890/job/17281491472?pr=3276, maybe there is different behavior for require and load :/

Probably we should move the specs which differ behavior for both to :kernel_require instead of :kernel_require_basic.

[eregon]

Given both CRuby and TruffleRuby error with a /file too short/ exception in that case it seems reasonable to check that.

[eregon]

I was curious if there was a way to test that the java code wasn't trying extra calls (like "a.rb.rb").

I'm not aware of one, that seems difficult to test. We could add a benchmark perharps but that would likely be noisy based on disk access, etc.

[eregon]

I moved the specs around and fixed them and did some trivial optimization, now all should be green :)

[rwstauner]

Great, thanks for your help!