Skip to content

Commit

Permalink
Reduce the Kernel.require extra stack frames some more.
Browse files Browse the repository at this point in the history
Now bootsnap should only add one extra frame per `require` call.

It also use less exceptions, so likely better performance wise even
though I didn't bother benchmarking it.
  • Loading branch information
byroot committed Jan 20, 2022
1 parent 12169cd commit 23b619f
Show file tree
Hide file tree
Showing 6 changed files with 38 additions and 47 deletions.
3 changes: 3 additions & 0 deletions CHANGELOG.md
@@ -1,11 +1,14 @@
# Unreleased

* Reduce the `Kernel.require` extra stack frames some more. Now bootsnap should only add one extra frame per `require` call.

* Better check `freeze` option support in JSON compile cache.
Previously `JSON.load_file(..., freeze: true)` would be cached even when the msgpack version is missing support for it.

# 1.10.1

* Fix `Kernel#autoload`'s fallback path always being executed.

* Consider `unlink` failing with `ENOENT` as a success.

# 1.10.0
Expand Down
3 changes: 1 addition & 2 deletions lib/bootsnap/load_path_cache.rb
Expand Up @@ -2,8 +2,7 @@

module Bootsnap
module LoadPathCache
ReturnFalse = Class.new(StandardError)
FallbackScan = Class.new(StandardError)
FALLBACK_SCAN = BasicObject.new

DOT_RB = ".rb"
DOT_SO = ".so"
Expand Down
6 changes: 3 additions & 3 deletions lib/bootsnap/load_path_cache/cache.rb
Expand Up @@ -65,7 +65,7 @@ def find(feature, try_extensions: true)
# returns false as if it were already loaded; however, there is no
# file to find on disk. We've pre-built a list of these, and we
# return false if any of them is loaded.
raise(LoadPathCache::ReturnFalse, "", []) if BUILTIN_FEATURES.key?(feature)
return false if BUILTIN_FEATURES.key?(feature)

# The feature wasn't found on our preliminary search through the index.
# We resolve this differently depending on what the extension was.
Expand All @@ -89,15 +89,15 @@ def find(feature, try_extensions: true)
else
# other, unknown extension. For example, `.rake`. Since we haven't
# cached these, we legitimately need to run the load path search.
raise(LoadPathCache::FallbackScan, "", [])
return FALLBACK_SCAN
end
end

# In development mode, we don't want to confidently return failures for
# cases where the file doesn't appear to be on the load path. We should
# be able to detect newly-created files without rebooting the
# application.
raise(LoadPathCache::FallbackScan, "", []) if @development_mode
return FALLBACK_SCAN if @development_mode
end

def unshift_paths(sender, *paths)
Expand Down
54 changes: 22 additions & 32 deletions lib/bootsnap/load_path_cache/core_ext/kernel_require.rb
Expand Up @@ -6,36 +6,30 @@ module Kernel
alias_method(:require_without_bootsnap, :require)

def require(path)
fallback = false
string_path = path.to_s
return false if Bootsnap::LoadPathCache.loaded_features_index.key?(string_path)

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

error = LoadError.new(+"cannot load such file -- #{path}")
error.instance_variable_set(:@path, path)
raise error
rescue Bootsnap::LoadPathCache::ReturnFalse
false
rescue Bootsnap::LoadPathCache::FallbackScan
fallback = true
ensure
# We raise from `ensure` so that any further exception don't have FallbackScan as a cause
# See: https://github.com/Shopify/bootsnap/issues/250
if fallback
resolved = Bootsnap::LoadPathCache.load_path_cache.find(string_path)
if Bootsnap::LoadPathCache::FALLBACK_SCAN.equal?(resolved)
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)
return ret
else
return require_without_bootsnap(path)
end
elsif false == resolved
return false
elsif resolved.nil?
error = LoadError.new(+"cannot load such file -- #{path}")
error.instance_variable_set(:@path, path)
raise error
else
# 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
end

Expand All @@ -61,24 +55,20 @@ def load(path, wrap = false)
class Module
alias_method(:autoload_without_bootsnap, :autoload)
def autoload(const, path)
fallback = false
# NOTE: This may defeat LoadedFeaturesIndex, but it's not immediately
# obvious how to make it work. This feels like a pretty niche case, unclear
# if it will ever burn anyone.
#
# The challenge is that we don't control the point at which the entry gets
# added to $LOADED_FEATURES and won't be able to hook that modification
# since it's done in C-land.
autoload_without_bootsnap(const, Bootsnap::LoadPathCache.load_path_cache.find(path) || path)
rescue Bootsnap::LoadPathCache::ReturnFalse
false
rescue Bootsnap::LoadPathCache::FallbackScan
fallback = true
ensure
# We raise from `ensure` so that any further exception don't have FallbackScan as a cause
# See: https://github.com/Shopify/bootsnap/issues/250
if fallback
resolved = Bootsnap::LoadPathCache.load_path_cache.find(path)
if Bootsnap::LoadPathCache::FALLBACK_SCAN.equal?(resolved)
autoload_without_bootsnap(const, path)
elsif resolved == false
return false
else
autoload_without_bootsnap(const, resolved || path)
end
end
end
2 changes: 1 addition & 1 deletion lib/bootsnap/load_path_cache/loaded_features_index.rb
Expand Up @@ -92,7 +92,7 @@ def identify(short, cursor)
# entry:
#
# If the user asked for e.g. `require 'bundler'`, and we went through the
# `FallbackScan` pathway in `kernel_require.rb` and therefore did not
# `FALLBACK_SCAN` pathway in `kernel_require.rb` and therefore did not
# pass `long` (the full expanded absolute path), then we did are not able
# to confidently add the `bundler.rb` form to @lfi.
#
Expand Down
17 changes: 8 additions & 9 deletions test/load_path_cache/cache_test.rb
Expand Up @@ -29,14 +29,15 @@ def teardown
# versions aren't a big deal, but feel free to fix the test.
def test_builtin_features
cache = Cache.new(NullCache, [])
assert_raises(ReturnFalse) { cache.find("thread") }
assert_raises(ReturnFalse) { cache.find("thread.rb") }
assert_raises(ReturnFalse) { cache.find("enumerator") }
assert_raises(ReturnFalse) { cache.find("enumerator.so") }
assert_equal false, cache.find("thread")
assert_equal false, cache.find("thread.rb")
assert_equal false, cache.find("enumerator")
assert_equal false, cache.find("enumerator.so")

if RUBY_PLATFORM =~ /darwin/
assert_raises(ReturnFalse) { cache.find("enumerator.bundle") }
assert_equal false, cache.find("enumerator.bundle")
else
assert_raises(FallbackScan) { cache.find("enumerator.bundle") }
assert_same FALLBACK_SCAN, cache.find("enumerator.bundle")
end

bundle = RUBY_PLATFORM =~ /darwin/ ? "bundle" : "so"
Expand Down Expand Up @@ -140,9 +141,7 @@ def test_development_mode
refute(dev_no_cache.find("new"))

dev_yes_cache.stubs(:now).returns(time + 28)
assert_raises(Bootsnap::LoadPathCache::FallbackScan) do
dev_yes_cache.find("new")
end
assert_same Bootsnap::LoadPathCache::FALLBACK_SCAN, dev_yes_cache.find("new")
dev_yes_cache.stubs(:now).returns(time + 31)
assert(dev_yes_cache.find("new"))
end
Expand Down

0 comments on commit 23b619f

Please sign in to comment.