From 721877fcb77f0528cbe2b9d32878388c6fb8e2b2 Mon Sep 17 00:00:00 2001 From: Randy Stauner Date: Sun, 24 Sep 2023 05:00:29 -0700 Subject: [PATCH] Optimize feature loading when require is called with absolute .rb path 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. Add require specs for absolute paths: - with no extension (RB ext and C ext) - .so becomes .bundle on darwin --- CHANGELOG.md | 1 + spec/ruby/core/kernel/shared/load.rb | 38 ++++++++++++------- spec/ruby/core/kernel/shared/require.rb | 26 +++++++++++++ spec/ruby/fixtures/code/d/load_fixture.rb.rb | 1 + spec/tags/core/kernel/require_tags.txt | 2 - .../language/loader/FeatureLoader.java | 38 +++++++------------ .../org/truffleruby/platform/Platform.java | 1 + 7 files changed, 66 insertions(+), 41 deletions(-) create mode 100644 spec/ruby/fixtures/code/d/load_fixture.rb.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index 5b693c411d55..879ec076b129 100644 --- a/CHANGELOG.md +++ b/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: diff --git a/spec/ruby/core/kernel/shared/load.rb b/spec/ruby/core/kernel/shared/load.rb index 5c41c19bf6c2..0fe2d5ce167c 100644 --- a/spec/ruby/core/kernel/shared/load.rb +++ b/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 diff --git a/spec/ruby/core/kernel/shared/require.rb b/spec/ruby/core/kernel/shared/require.rb index 61081b200c3e..3e22f6fee2a1 100644 --- a/spec/ruby/core/kernel/shared/require.rb +++ b/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 diff --git a/spec/ruby/fixtures/code/d/load_fixture.rb.rb b/spec/ruby/fixtures/code/d/load_fixture.rb.rb new file mode 100644 index 000000000000..7e9217729a3e --- /dev/null +++ b/spec/ruby/fixtures/code/d/load_fixture.rb.rb @@ -0,0 +1 @@ +ScratchPad << :rbrb diff --git a/spec/tags/core/kernel/require_tags.txt b/spec/tags/core/kernel/require_tags.txt index b2a99682a533..fcc410e96e8e 100644 --- a/spec/tags/core/kernel/require_tags.txt +++ b/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 diff --git a/src/main/java/org/truffleruby/language/loader/FeatureLoader.java b/src/main/java/org/truffleruby/language/loader/FeatureLoader.java index 8140fb120860..9d68db2fefbd 100644 --- a/src/main/java/org/truffleruby/language/loader/FeatureLoader.java +++ b/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,7 +369,7 @@ 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 { @@ -376,33 +377,20 @@ private String translateIfNativePath(String 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) { diff --git a/src/main/java/org/truffleruby/platform/Platform.java b/src/main/java/org/truffleruby/platform/Platform.java index 8104889ef5b9..749a2481a504 100644 --- a/src/main/java/org/truffleruby/platform/Platform.java +++ b/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) {