Merge pull request #393 from Shopify/require-backtrace

Minimize the Kernel.require extra stack frames

CHANGELOG.md

@@ -1,5 +1,8 @@
 # Unreleased
 
+* Minimize the Kernel.require extra stack frames. (#393)
+  This should reduce the noise generated by bootsnap on `LoadError`.
+
 # 1.9.4
 
 * Ignore absolute paths in the loaded feature index. (#385)

lib/bootsnap/load_path_cache/core_ext/kernel_require.rb

@@ -18,18 +18,15 @@ module Kernel
 
   alias_method(:require_without_bootsnap, :require)
 
-  # Note that require registers to $LOADED_FEATURES while load does not.
-  def require_with_bootsnap_lfi(path, resolved = nil)
-    Bootsnap::LoadPathCache.loaded_features_index.register(path, resolved) do
-      require_without_bootsnap(resolved || path)
-    end
-  end
-
   def require(path)
-    return false if Bootsnap::LoadPathCache.loaded_features_index.key?(path)
+    string_path = path.to_s
+    return false if Bootsnap::LoadPathCache.loaded_features_index.key?(string_path)
 
-    if (resolved = Bootsnap::LoadPathCache.load_path_cache.find(path))
-      return require_with_bootsnap_lfi(path, resolved)
+    if (resolved = Bootsnap::LoadPathCache.load_path_cache.find(string_path))
+      # Note that require registers to $LOADED_FEATURES while load does not.
+      ret = require_without_bootsnap(resolved)
+      Bootsnap::LoadPathCache.loaded_features_index.register(string_path, resolved)
+      return ret
     end
 
     raise(Bootsnap::LoadPathCache::CoreExt.make_load_error(path))
@@ -39,10 +36,13 @@ def require(path)
   rescue Bootsnap::LoadPathCache::ReturnFalse
     false
   rescue Bootsnap::LoadPathCache::FallbackScan
-    fallback = true
-  ensure
-    if fallback
-      require_with_bootsnap_lfi(path)
+    if (cursor = Bootsnap::LoadPathCache.loaded_features_index.cursor(string_path))
+      ret = require_without_bootsnap(path)
+      resolved = Bootsnap::LoadPathCache.loaded_features_index.identify(string_path, cursor)
+      Bootsnap::LoadPathCache.loaded_features_index.register(string_path, resolved)
+      ret
+    else # If we're not given a cursor, it means we don't need to register the path (likely an absolute path)
+      require_without_bootsnap(path)
     end
   end
 
@@ -82,10 +82,6 @@ def autoload(const, path)
   rescue Bootsnap::LoadPathCache::ReturnFalse
     false
   rescue Bootsnap::LoadPathCache::FallbackScan
-    fallback = true
-  ensure
-    if fallback
-      autoload_without_bootsnap(const, path)
-    end
+    autoload_without_bootsnap(const, path)
   end
 end

lib/bootsnap/load_path_cache/loaded_features_index.rb

@@ -69,6 +69,25 @@ def key?(feature)
         @mutex.synchronize { @lfi.key?(feature) }
       end
 
+      def cursor(short)
+        unless Bootsnap.absolute_path?(short.to_s)
+          $LOADED_FEATURES.size
+        end
+      end
+
+      def identify(short, cursor)
+        $LOADED_FEATURES[cursor..-1].detect do |feat|
+          offset = 0
+          while (offset = feat.index(short, offset))
+            if feat.index(".", offset + 1) && !feat.index("/", offset + 2)
+              break true
+            else
+              offset += 1
+            end
+          end
+        end
+      end
+
       # There is a relatively uncommon case where we could miss adding an
       # entry:
       #
@@ -83,28 +102,8 @@ def key?(feature)
       #    not quite right; or
       # 2. Inspect $LOADED_FEATURES upon return from yield to find the matching
       #    entry.
-      def register(short, long = nil)
-        # Absolute paths are not a concern.
-        if Bootsnap.absolute_path?(short.to_s)
-          return yield
-        end
-
-        if long.nil?
-          len = $LOADED_FEATURES.size
-          ret = yield
-          long = $LOADED_FEATURES[len..-1].detect do |feat|
-            offset = 0
-            while (offset = feat.index(short, offset))
-              if feat.index(".", offset + 1) && !feat.index("/", offset + 2)
-                break true
-              else
-                offset += 1
-              end
-            end
-          end
-        else
-          ret = yield
-        end
+      def register(short, long)
+        return if Bootsnap.absolute_path?(short)
 
         hash = long.hash
 
@@ -123,8 +122,6 @@ def register(short, long = nil)
           @lfi[short] = hash
           (@lfi[altname] = hash) if altname
         end
-
-        ret
       end
 
       private

test/load_path_cache/loaded_features_index_test.rb

@@ -15,29 +15,17 @@ def test_successful_addition
         refute(@index.key?("bundler"))
         refute(@index.key?("bundler.rb"))
         refute(@index.key?("foo"))
-        @index.register("bundler", "/a/b/bundler.rb") {}
+        @index.register("bundler", "/a/b/bundler.rb")
         assert(@index.key?("bundler"))
         assert(@index.key?("bundler.rb"))
         refute(@index.key?("foo"))
       end
 
-      def test_no_add_on_raise
-        refute(@index.key?("bundler"))
-        refute(@index.key?("bundler.rb"))
-        refute(@index.key?("foo"))
-        assert_raises(RuntimeError) do
-          @index.register("bundler", "/a/b/bundler.rb") { raise }
-        end
-        refute(@index.key?("bundler"))
-        refute(@index.key?("bundler.rb"))
-        refute(@index.key?("foo"))
-      end
-
       def test_infer_base_from_ext
         refute(@index.key?("bundler"))
         refute(@index.key?("bundler.rb"))
         refute(@index.key?("foo"))
-        @index.register("bundler.rb") {}
+        @index.register("bundler.rb", nil)
         assert(@index.key?("bundler"))
         assert(@index.key?("bundler.rb"))
         refute(@index.key?("foo"))
@@ -54,7 +42,7 @@ def test_only_strip_elidable_ext
         refute(@index.key?("descriptor.rb"))
         refute(@index.key?("descriptor"))
         refute(@index.key?("foo"))
-        @index.register("descriptor.pb.rb") {}
+        @index.register("descriptor.pb.rb", nil)
         assert(@index.key?("descriptor.pb"))
         assert(@index.key?("descriptor.pb.rb"))
         refute(@index.key?("descriptor.rb"))
@@ -70,7 +58,7 @@ def test_shared_library_ext_considered_elidable
         refute(@index.key?("descriptor.rb"))
         refute(@index.key?("descriptor"))
         refute(@index.key?("foo"))
-        @index.register("libgit2.dylib") {}
+        @index.register("libgit2.dylib", nil)
         assert(@index.key?("libgit2.dylib"))
         refute(@index.key?("libgit2.dylib.rb"))
         refute(@index.key?("libgit2.rb"))
@@ -81,7 +69,7 @@ def test_cannot_infer_ext_from_base # Current limitation
         refute(@index.key?("bundler"))
         refute(@index.key?("bundler.rb"))
         refute(@index.key?("foo"))
-        @index.register("bundler") {}
+        @index.register("bundler", nil)
         assert(@index.key?("bundler"))
         refute(@index.key?("bundler.rb"))
         refute(@index.key?("foo"))
@@ -91,7 +79,7 @@ def test_purge_loaded_feature
         refute(@index.key?("bundler"))
         refute(@index.key?("bundler.rb"))
         refute(@index.key?("foo"))
-        @index.register("bundler", "/a/b/bundler.rb") {}
+        @index.register("bundler", "/a/b/bundler.rb")
         assert(@index.key?("bundler"))
         assert(@index.key?("bundler.rb"))
         refute(@index.key?("foo"))
@@ -105,7 +93,7 @@ def test_purge_multi_loaded_feature
         refute(@index.key?("bundler"))
         refute(@index.key?("bundler.rb"))
         refute(@index.key?("foo"))
-        @index.register("bundler", "/a/b/bundler.rb") {}
+        @index.register("bundler", "/a/b/bundler.rb")
         assert(@index.key?("bundler"))
         assert(@index.key?("bundler.rb"))
         refute(@index.key?("foo"))
@@ -119,7 +107,10 @@ def test_register_finds_correct_feature
         refute(@index.key?("bundler"))
         refute(@index.key?("bundler.rb"))
         refute(@index.key?("foo"))
-        @index.register("bundler", nil) { $LOADED_FEATURES << "/a/b/bundler.rb" }
+        cursor = @index.cursor("bundler")
+        $LOADED_FEATURES << "/a/b/bundler.rb"
+        long = @index.identify("bundler", cursor)
+        @index.register("bundler", long)
         assert(@index.key?("bundler"))
         assert(@index.key?("bundler.rb"))
         refute(@index.key?("foo"))
@@ -136,23 +127,11 @@ def test_derives_initial_state_from_loaded_features
         refute(index.key?("minitest/autorun.so"))
       end
 
-      def test_works_with_pathname
-        path = "bundler.rb"
-        pathname = Pathname.new(path)
-        @index.register(pathname, path) { true }
-        assert(@index.key?(pathname))
-      end
-
       def test_ignores_absolute_paths
         path = "#{Dir.mktmpdir}/bundler.rb"
-        @index.register(path) { true }
-        refute(@index.key?(path))
-
-        path = "#{Dir.mktmpdir}/bundler.rb"
-        pathname = Pathname.new(path)
-        @index.register(pathname, path) { true }
+        assert_nil @index.cursor(path)
+        @index.register(path, path)
         refute(@index.key?(path))
-        refute(@index.key?(pathname))
       end
     end
   end