From f2303405b1c46397a230840e62f91d6b6f35411a Mon Sep 17 00:00:00 2001 From: Jean Boussier Date: Fri, 14 Jan 2022 10:33:11 +0100 Subject: [PATCH] Minimize the Kernel.require extra stack frames Currently Bootsnap adds 3 stackframe per require call. In itself it's not a big deal, but it makes for really noisy backtrace on LoadError. --- CHANGELOG.md | 3 ++ .../core_ext/kernel_require.rb | 34 ++++++-------- .../load_path_cache/loaded_features_index.rb | 45 +++++++++--------- .../loaded_features_index_test.rb | 47 +++++-------------- 4 files changed, 52 insertions(+), 77 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c522b199..e40b466c 100644 --- a/CHANGELOG.md +++ b/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) diff --git a/lib/bootsnap/load_path_cache/core_ext/kernel_require.rb b/lib/bootsnap/load_path_cache/core_ext/kernel_require.rb index 9e9130f5..e8e7dd0d 100644 --- a/lib/bootsnap/load_path_cache/core_ext/kernel_require.rb +++ b/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 diff --git a/lib/bootsnap/load_path_cache/loaded_features_index.rb b/lib/bootsnap/load_path_cache/loaded_features_index.rb index 9e070f38..3e6ac722 100644 --- a/lib/bootsnap/load_path_cache/loaded_features_index.rb +++ b/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 diff --git a/test/load_path_cache/loaded_features_index_test.rb b/test/load_path_cache/loaded_features_index_test.rb index 6edd1a78..66b6599e 100644 --- a/test/load_path_cache/loaded_features_index_test.rb +++ b/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