[GR-19220] Optimize feature loading when require is called with absol…

…ute .rb path (#3276)

PullRequest: truffleruby/4023

CHANGELOG.md

@@ -16,6 +16,7 @@ Compatibility:
 Performance:
 
 * Optimize calls with `ruby2_keywords` forwarding by deciding it per call site instead of per callee thanks to [my fix in CRuby 3.2](https://bugs.ruby-lang.org/issues/18625) (@eregon).
+* Optimize feature loading when require is called with an absolute path to a .rb file (@rwstauner).
 
 Changes:
 

spec/ruby/core/kernel/shared/load.rb

@@ -1,5 +1,6 @@
 main = self
 
+# The big difference is Kernel#load does not attempt to add an extension to the passed path, unlike Kernel#require
 describe :kernel_load, shared: true do
   before :each do
     CodeLoadingSpecs.spec_setup
@@ -10,22 +11,31 @@
     CodeLoadingSpecs.spec_cleanup
   end
 
-  it "loads a non-extensioned file as a Ruby source file" do
-    path = File.expand_path "load_fixture", CODE_LOADING_DIR
-    @object.load(path).should be_true
-    ScratchPad.recorded.should == [:no_ext]
-  end
+  describe "(path resolution)" do
+    # This behavior is specific to Kernel#load, it differs for Kernel#require
+    it "loads a non-extensioned file as a Ruby source file" do
+      path = File.expand_path "load_fixture", CODE_LOADING_DIR
+      @object.load(path).should be_true
+      ScratchPad.recorded.should == [:no_ext]
+    end
 
-  it "loads a non .rb extensioned file as a Ruby source file" do
-    path = File.expand_path "load_fixture.ext", CODE_LOADING_DIR
-    @object.load(path).should be_true
-    ScratchPad.recorded.should == [:no_rb_ext]
-  end
+    it "loads a non .rb extensioned file as a Ruby source file" do
+      path = File.expand_path "load_fixture.ext", CODE_LOADING_DIR
+      @object.load(path).should be_true
+      ScratchPad.recorded.should == [:no_rb_ext]
+    end
 
-  it "loads from the current working directory" do
-    Dir.chdir CODE_LOADING_DIR do
-      @object.load("load_fixture.rb").should be_true
-      ScratchPad.recorded.should == [:loaded]
+    it "loads from the current working directory" do
+      Dir.chdir CODE_LOADING_DIR do
+        @object.load("load_fixture.rb").should be_true
+        ScratchPad.recorded.should == [:loaded]
+      end
+    end
+
+    # This behavior is specific to Kernel#load, it differs for Kernel#require
+    it "does not look for a c-extension file when passed a path without extension (when no .rb is present)" do
+      path = File.join CODE_LOADING_DIR, "a", "load_fixture"
+      -> { @object.send(@method, path) }.should raise_error(LoadError)
     end
   end
 

spec/ruby/core/kernel/shared/require.rb

@@ -212,6 +212,32 @@
 
 describe :kernel_require, shared: true do
   describe "(path resolution)" do
+    it "loads .rb file when passed absolute path without extension" do
+      path = File.expand_path "load_fixture", CODE_LOADING_DIR
+      @object.send(@method, path).should be_true
+      # This should _not_ be [:no_ext]
+      ScratchPad.recorded.should == [:loaded]
+    end
+
+    platform_is :linux, :darwin do
+      it "loads c-extension file when passed absolute path without extension when no .rb is present" do
+        path = File.join CODE_LOADING_DIR, "a", "load_fixture"
+        -> { @object.send(@method, path) }.should raise_error(Exception, /file too short/)
+      end
+    end
+
+    platform_is :darwin do
+      it "loads .bundle file when passed absolute path with .so" do
+        path = File.join CODE_LOADING_DIR, "a", "load_fixture.so"
+        -> { @object.send(@method, path) }.should raise_error(Exception, /load_fixture\.bundle.+file too short/)
+      end
+    end
+
+    it "does not try an extra .rb if the path already ends in .rb" do
+      path = File.join CODE_LOADING_DIR, "d", "load_fixture.rb"
+      -> { @object.send(@method, path) }.should raise_error(LoadError)
+    end
+
     # For reference see [ruby-core:24155] in which matz confirms this feature is
     # intentional for security reasons.
     it "does not load a bare filename unless the current working directory is in $LOAD_PATH" do

spec/ruby/fixtures/code/d/load_fixture.rb.rb

@@ -0,0 +1 @@
+ScratchPad << :rbrb

spec/tags/core/kernel/require_tags.txt

@@ -2,8 +2,6 @@ slow:Kernel#require (concurrently) blocks based on the path
 slow:Kernel.require (concurrently) blocks based on the path
 slow:Kernel#require ($LOADED_FEATURES) unicode_normalize is part of core and not $LOADED_FEATURES
 slow:Kernel.require ($LOADED_FEATURES) unicode_normalize is part of core and not $LOADED_FEATURES
-fails:Kernel#require (non-extensioned path) loads a .rb extensioned file when a C-extension file exists on an earlier load path
-fails:Kernel#require (non-extensioned path) does not load a feature twice when $LOAD_PATH has been modified
 slow:Kernel#require ($LOADED_FEATURES) complex, enumerator, rational, thread, ruby2_keywords are already required
 slow:Kernel.require ($LOADED_FEATURES) complex, enumerator, rational, thread, ruby2_keywords are already required
 slow:Kernel#require complex, enumerator, rational, thread, ruby2_keywords, fiber are already required

src/main/java/org/truffleruby/language/loader/FeatureLoader.java

@@ -179,8 +179,8 @@ private String basenameWithoutExtension(String path) {
     }
 
     private boolean hasExtension(String path) {
-        return path.endsWith(TruffleRuby.EXTENSION) || path.endsWith(RubyLanguage.CEXT_EXTENSION) ||
-                path.endsWith(".so");
+        return path.endsWith(TruffleRuby.EXTENSION) || path.endsWith(".so") ||
+                (!Platform.CEXT_SUFFIX_IS_SO && path.endsWith(RubyLanguage.CEXT_EXTENSION));
     }
 
     public void setWorkingDirectory(String cwd) {
@@ -314,6 +314,7 @@ private String findFeatureImpl(String feature) {
         if (feature.startsWith(RubyLanguage.RESOURCE_SCHEME) || new File(feature).isAbsolute()) {
             found = findFeatureWithAndWithoutExtension(feature);
         } else if (hasExtension(feature)) {
+            String path = translateIfNativePath(feature);
             final RubyArray expandedLoadPath = (RubyArray) DispatchNode.getUncached().call(
                     context.getCoreLibrary().truffleFeatureLoaderModule,
                     "get_expanded_load_path");
@@ -324,7 +325,7 @@ private String findFeatureImpl(String feature) {
                     RubyLanguage.LOGGER.info(String.format("from load path %s...", loadPath));
                 }
 
-                String fileWithinPath = new File(loadPath, translateIfNativePath(feature)).getPath();
+                String fileWithinPath = new File(loadPath, path).getPath();
                 final String result = findFeatureWithExactPath(fileWithinPath);
 
                 if (result != null) {
@@ -368,41 +369,28 @@ private String findFeatureImpl(String feature) {
     }
 
     private String translateIfNativePath(String feature) {
-        if (!RubyLanguage.CEXT_EXTENSION.equals(".so") && feature.endsWith(".so")) {
+        if (!Platform.CEXT_SUFFIX_IS_SO && feature.endsWith(".so")) {
             final String base = feature.substring(0, feature.length() - 3);
             return base + RubyLanguage.CEXT_EXTENSION;
         } else {
             return feature;
         }
     }
 
+    // Only used when the path is absolute
     private String findFeatureWithAndWithoutExtension(String path) {
         assert new File(path).isAbsolute();
 
-        if (path.endsWith(".so")) {
-            final String pathWithNativeExt = translateIfNativePath(path);
-            final String asCExt = findFeatureWithExactPath(pathWithNativeExt);
-            if (asCExt != null) {
-                return asCExt;
+        if (hasExtension(path)) {
+            return findFeatureWithExactPath(translateIfNativePath(path));
+        } else {
+            final String asRuby = findFeatureWithExactPath(path + TruffleRuby.EXTENSION);
+            if (asRuby != null) {
+                return asRuby;
             }
-        }
-
-        final String asRuby = findFeatureWithExactPath(path + TruffleRuby.EXTENSION);
-        if (asRuby != null) {
-            return asRuby;
-        }
 
-        final String asCExt = findFeatureWithExactPath(path + RubyLanguage.CEXT_EXTENSION);
-        if (asCExt != null) {
-            return asCExt;
+            return findFeatureWithExactPath(path + RubyLanguage.CEXT_EXTENSION);
         }
-
-        final String withoutExtension = findFeatureWithExactPath(path);
-        if (withoutExtension != null) {
-            return withoutExtension;
-        }
-
-        return null;
     }
 
     private String findFeatureWithExactPath(String path) {

src/main/java/org/truffleruby/platform/Platform.java

@@ -34,6 +34,7 @@ public abstract class Platform extends BasicPlatform {
 
     public static final String LIB_SUFFIX = determineLibSuffix();
     public static final String CEXT_SUFFIX = OS == OS_TYPE.DARWIN ? ".bundle" : LIB_SUFFIX;
+    public static final boolean CEXT_SUFFIX_IS_SO = CEXT_SUFFIX.equals(".so");
 
     private static String determineLibSuffix() {
         switch (OS) {