Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

3.x super #507

Closed
wants to merge 19 commits into from
Closed
Show file tree
Hide file tree
Changes from 14 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/sprockets/base.rb
Expand Up @@ -100,7 +100,7 @@ def inspect
end

def compress_from_root(uri)
URITar.new(uri, self).compress
URITar.fast_compress(uri, self.root)
end

def expand_from_root(uri)
Expand Down
2 changes: 1 addition & 1 deletion lib/sprockets/cache.rb
Expand Up @@ -60,7 +60,7 @@ def self.default_logger
# cache - A compatible backend cache store instance.
def initialize(cache = nil, logger = self.class.default_logger)
@cache_wrapper = get_cache_wrapper(cache)
@fetch_cache = Cache::MemoryStore.new(1024)
@fetch_cache = Cache::MemoryStore.new
@logger = logger
end

Expand Down
2 changes: 1 addition & 1 deletion lib/sprockets/cache/memory_store.rb
Expand Up @@ -12,7 +12,7 @@ class Cache
#
class MemoryStore
# Internal: Default key limit for store.
DEFAULT_MAX_SIZE = 1000
DEFAULT_MAX_SIZE = 10000

# Public: Initialize the cache store.
#
Expand Down
37 changes: 35 additions & 2 deletions lib/sprockets/digest_utils.rb
Expand Up @@ -84,6 +84,39 @@ def detect_digest_class(bytes)
}
private_constant :ADD_VALUE_TO_DIGEST

def a_v_t_d(c, v, d)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this isn't significantly faster, only 10% on ruby 2.4 and even slower on older versions

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, interesting, it makes the digest completely disappear from my callgraph,
what os, cpu are did you try on?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well, if 10% is not significant ...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

10% improvement looks nice, but if this call takes for instance only 5% of the entire compilation process, than it's only a microoptimization without any significant effect.

I was wondering how fast this digest optimization is, so I tested it on our pipeline (big project) and here're my numbers (spent time in this method, same hw)
ruby 2.4.2 windows 29,4% faster
ruby 2.4.2 linux 17,1% faster
ruby 2.1.9 windows 8,6% slower
ruby 2.1.9 linux 17.6% faster
jruby 9.1.13.0 windows 10,3% slower
jruby 9.1.13.0 linux 11,1% slower

I didn't have time to dig deeper. In my opinion your implementation should be faster, but optimizations are sometimes tricky :)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the numbers, terrific :)
With the compilation time going down 10 times, the digest became very well visible in the call graph, therefore i looked at it, because that small improvement became more significant with overall times going down that much.

ruby 2.1.* is obsolete since spring 2017

jruby only supports rails 4, which is, except for severe security issues, out of support too
And if a case is slower on jruby, and if this project really wants to support jruby, this may be a good case for opening a 'performance: case' ticket over at jruby. :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Originally I optimized this using refinements, it's even faster than what we ended up with in MRI. But it made it way slower on jruby.

If you're going to do microbenchmarks, please use benchmark-ips as in

Last time we looked at this the hash method was faster than the case method. A big improvement was added when we started using compare with identity https://github.com/rails/sprockets/pull/439/files.

case c
when 'String'.freeze then d << v
when 'FalseClass'.freeze then d << 'FalseClass'.freeze
when 'TrueClass'.freeze then d << 'TrueClass'.freeze
when 'NilClass'.freeze then d << 'NilClass'.freeze
when 'Symbol'.freeze
d << 'Symbol'.freeze
d << v.to_s
when 'Integer'.freeze
d << 'Integer'.freeze
d << v.to_s
when 'Array'.freeze
d << 'Array'.freeze
v.each { |element| a_v_t_d(element.class.name, element, d) }
when 'Hash'.freeze
d << 'Hash'.freeze
v.sort.each { |array| a_v_t_d('Array', array, d) }
when 'Set'.freeze
d << 'Set'.freeze
a_v_t_d('Array',v.to_a, d)
when 'Encoding'.freeze
d << 'Encoding'.freeze
d << v.name
when 'Fixnum'.freeze
d << 'Integer'.freeze
d << v.to_s
when 'Bignum'.freeze
d << 'Integer'.freeze
d << v.to_s
else raise TypeError, "couldn't digest #{c} - #{c.class} -- #{v}"
end
end
# Internal: Generate a hexdigest for a nested JSON serializable object.
#
# This is used for generating cache keys, so its pretty important its
Expand All @@ -94,8 +127,8 @@ def detect_digest_class(bytes)
# Returns a String digest of the object.
def digest(obj)
digest = digest_class.new

ADD_VALUE_TO_DIGEST[obj.class].call(obj, digest)
a_v_t_d(obj.class.name, obj, digest)
# ADD_VALUE_TO_DIGEST[obj.class].call(obj, digest)
digest.digest
end

Expand Down
1 change: 1 addition & 0 deletions lib/sprockets/http_utils.rb
Expand Up @@ -56,6 +56,7 @@ def parse_q_values(values)
#
# Returns Array of matched Strings from available Array or [].
def find_q_matches(q_values, available, &matcher)
return [] if available == []
matcher ||= lambda { |a, b| a == b }

matches = []
Expand Down
18 changes: 15 additions & 3 deletions lib/sprockets/loader.rb
Expand Up @@ -30,6 +30,8 @@ module Loader
#
# Returns Asset.
def load(uri)
@@gorilla_ass_cache ||= {}
app_root = Sprockets.check_modified_root
unloaded = UnloadedAsset.new(uri, self)
if unloaded.params.key?(:id)
unless asset = asset_from_cache(unloaded.asset_key)
Expand All @@ -52,9 +54,19 @@ def load(uri)
# will confusingly be called again with `paths` set to nil where the asset will be
# loaded from disk.
if paths
digest = DigestUtils.digest(resolve_dependencies(paths))
if uri_from_cache = cache.get(unloaded.digest_key(digest), true)
asset_from_cache(UnloadedAsset.new(uri_from_cache, self).asset_key)
if uri.to_s[7..(app_root.length+6)] != app_root
unless @@gorilla_ass_cache.has_key?(uri)
digest = DigestUtils.digest(resolve_dependencies(paths))
if uri_from_cache = cache.get(unloaded.digest_key(digest), true)
@@gorilla_ass_cache[uri] = asset_from_cache(UnloadedAsset.new(uri_from_cache, self).asset_key)
end
end
@@gorilla_ass_cache[uri]
else
digest = DigestUtils.digest(resolve_dependencies(paths))
if uri_from_cache = cache.get(unloaded.digest_key(digest), true)
asset_from_cache(UnloadedAsset.new(uri_from_cache, self).asset_key)
end
end
else
load_from_unloaded(unloaded)
Expand Down
17 changes: 14 additions & 3 deletions lib/sprockets/path_utils.rb
Expand Up @@ -12,10 +12,21 @@ module PathUtils
#
# Returns nil if the file does not exist.
def stat(path)
if File.exist?(path)
File.stat(path.to_s)
app_root = Sprockets.check_modified_root
if (path[0...app_root.length] != app_root)
@@gorilla_patch_cached_stats ||= {}
return @@gorilla_patch_cached_stats[path] if @@gorilla_patch_cached_stats.has_key?(path)
@@gorilla_patch_cached_stats[path] = if File.exist?(path)
File.stat(path)
else
nil
end
else
nil
if File.exist?(path)
File.stat(path)
else
nil
end
end
end

Expand Down
10 changes: 10 additions & 0 deletions lib/sprockets/paths.rb
Expand Up @@ -23,6 +23,16 @@ def root=(path)
end
private :root=

# requests outside of check_modified_root will be cached
def check_modified_root
config[:check_modified_root]
end
def check_modified_root=(path)
self.config = hash_reassoc(config, :check_modified_root) do
File.expand_path(path)
end
end

# Returns an `Array` of path `String`s.
#
# These paths will be used for asset logical path lookups.
Expand Down
18 changes: 16 additions & 2 deletions lib/sprockets/resolve.rb
Expand Up @@ -153,6 +153,12 @@ def parse_accept_options(mime_type, types)
end

def path_matches(load_path, logical_name, logical_basename)
@@gorilla_djungle_banana_cache ||= {}
app_root = Sprockets.check_modified_root
is_in_app = load_path[0...app_root.size] == app_root

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might make sense to stringify app_root here. That config option can be nil.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, that method call is used in several places. Might make sense to always return a string from it instead.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is_in_app = load_path.start_with?(app_root.to_s)

if !is_in_app && @@gorilla_djungle_banana_cache.has_key?([load_path, logical_name, logical_basename])
return @@gorilla_djungle_banana_cache[[load_path, logical_name, logical_basename]]
end
dirname = File.dirname(File.join(load_path, logical_name))
candidates = dirname_matches(dirname, logical_basename)
deps = file_digest_dependency_set(dirname)
Expand All @@ -170,11 +176,18 @@ def path_matches(load_path, logical_name, logical_basename)
end

deps.merge(file_digest_dependency_set(dirname))

return candidates.select { |fn, _| file?(fn) }, deps
result = candidates.select { |fn, _| file?(fn) }, deps
@@gorilla_djungle_banana_cache[[load_path, logical_name, logical_basename]] = result if !is_in_app
result
end

def dirname_matches(dirname, basename)
@@gorilla_djungle_banana_tree_cache ||= {}
app_root = Sprockets.check_modified_root
is_in_app = dirname[0...app_root.size] == app_root
if !is_in_app && @@gorilla_djungle_banana_tree_cache.has_key?([dirname, basename])
return @@gorilla_djungle_banana_cache[[dirname, basename]]
end
candidates = []
entries = self.entries(dirname)
entries.each do |entry|
Expand All @@ -184,6 +197,7 @@ def dirname_matches(dirname, basename)
candidates << [File.join(dirname, entry), type]
end
end
@@gorilla_djungle_banana_tree_cache[[dirname, basename]] = candidates if !is_in_app
candidates
end

Expand Down
29 changes: 22 additions & 7 deletions lib/sprockets/uri_tar.rb
Expand Up @@ -13,11 +13,11 @@ def initialize(uri, env)
@root = env.root
@env = env
uri = uri.to_s
if uri.include?("://".freeze)
@scheme, _, @path = uri.partition("://".freeze)
@scheme << "://".freeze
if uri.include?('://'.freeze)
@scheme, _, @path = uri.partition('://'.freeze)
@scheme << '://'.freeze
else
@scheme = "".freeze
@scheme = ''.freeze
@path = uri
end
end
Expand All @@ -35,6 +35,21 @@ def compress
scheme + compressed_path
end

def self.fast_compress(uri, root)
if uri.include?('://'.freeze)
scheme, _, path = uri.partition('://'.freeze)
scheme << '://'.freeze
else
scheme = ''.freeze
path = uri
end
if compressed_path = PathUtils.split_subpath(root, path)
scheme + compressed_path
else
scheme + path
end
end

# Internal: Tells us if we are using an absolute path
#
# Nix* systems start with a `/` like /Users/schneems.
Expand Down Expand Up @@ -66,7 +81,7 @@ def expand
else
# We always want to return an absolute uri,
# make sure the path starts with a slash.
scheme + File.join("/".freeze, root, path)
scheme + File.join('/'.freeze, root, path)
end
end
end
Expand All @@ -82,8 +97,8 @@ def expand
# Returns String
def compressed_path
# windows
if !@root.start_with?("/".freeze) && path.start_with?("/".freeze)
consistent_root = "/".freeze + @root
if !@root.start_with?('/'.freeze) && path.start_with?('/'.freeze)
consistent_root = '/'.freeze + @root
else
consistent_root = @root
end
Expand Down
8 changes: 4 additions & 4 deletions lib/sprockets/uri_utils.rb
Expand Up @@ -48,7 +48,7 @@ def split_file_uri(uri)
path.force_encoding(Encoding::UTF_8)

# Hack for parsing Windows "file:///C:/Users/IEUser" paths
path.gsub!(/^\/([a-zA-Z]:)/, '\1'.freeze)
path = path[1..-1] if File::ALT_SEPARATOR

[scheme, host, path, query]
end
Expand All @@ -59,7 +59,7 @@ def split_file_uri(uri)
def join_file_uri(scheme, host, path, query)
str = "#{scheme}://"
str << host if host
path = "/#{path}" unless path.start_with?("/")
path = "/#{path}" unless path.start_with?('/'.freeze)
str << URI::Generic::DEFAULT_PARSER.escape(path)
str << "?#{query}" if query
str
Expand All @@ -72,7 +72,7 @@ def join_file_uri(scheme, host, path, query)
# Returns true or false.
def valid_asset_uri?(str)
# Quick prefix check before attempting a full parse
str.start_with?("file://") && parse_asset_uri(str) ? true : false
str.start_with?('file://'.freeze) && parse_asset_uri(str) ? true : false
rescue URI::InvalidURIError
false
end
Expand Down Expand Up @@ -109,7 +109,7 @@ def parse_asset_uri(uri)
#
# Returns String URI.
def build_asset_uri(path, params = {})
join_file_uri("file", nil, path, encode_uri_query_params(params))
join_file_uri('file', nil, path, encode_uri_query_params(params))
end

# Internal: Parse file-digest dependency URI.
Expand Down
27 changes: 16 additions & 11 deletions test/test_uri_utils.rb
Expand Up @@ -45,15 +45,16 @@ def test_split_file_uri

parts = split_file_uri("file:///usr/local/var/github/app/assets/javascripts/application.js")
assert_equal ['file', nil, '/usr/local/var/github/app/assets/javascripts/application.js', nil], parts
if DOSISH
parts = split_file_uri("file:///C:/Documents%20and%20Settings/davris/FileSchemeURIs.doc")
assert_equal ['file', nil, 'C:/Documents and Settings/davris/FileSchemeURIs.doc', nil], parts

parts = split_file_uri("file:///C:/Documents%20and%20Settings/davris/FileSchemeURIs.doc")
assert_equal ['file', nil, 'C:/Documents and Settings/davris/FileSchemeURIs.doc', nil], parts
parts = split_file_uri("file:///D:/Program%20Files/Viewer/startup.htm")
assert_equal ['file', nil, 'D:/Program Files/Viewer/startup.htm', nil], parts

parts = split_file_uri("file:///D:/Program%20Files/Viewer/startup.htm")
assert_equal ['file', nil, 'D:/Program Files/Viewer/startup.htm', nil], parts

parts = split_file_uri("file:///C:/Program%20Files/Music/Web%20Sys/main.html?REQUEST=RADIO")
assert_equal ['file', nil, 'C:/Program Files/Music/Web Sys/main.html', 'REQUEST=RADIO'], parts
parts = split_file_uri("file:///C:/Program%20Files/Music/Web%20Sys/main.html?REQUEST=RADIO")
assert_equal ['file', nil, 'C:/Program Files/Music/Web Sys/main.html', 'REQUEST=RADIO'], parts
end
end

def test_join_uri_path
Expand Down Expand Up @@ -98,8 +99,10 @@ def test_parse_file_paths
parse_asset_uri("file:///usr/local/var/github/app/assets/javascripts/application.js")
assert_equal ["/usr/local/var/github/app/assets/javascripts/foo bar.js", {}],
parse_asset_uri("file:///usr/local/var/github/app/assets/javascripts/foo%20bar.js")
assert_equal ["C:/Users/IEUser/Documents/github/app/assets/javascripts/application.js", {}],
parse_asset_uri("file:///C:/Users/IEUser/Documents/github/app/assets/javascripts/application.js")
if DOSISH
assert_equal ["C:/Users/IEUser/Documents/github/app/assets/javascripts/application.js", {}],
parse_asset_uri("file:///C:/Users/IEUser/Documents/github/app/assets/javascripts/application.js")
end
end

def test_parse_query_params
Expand Down Expand Up @@ -152,8 +155,10 @@ def test_parse_file_digest_uri
parse_file_digest_uri("file-digest:///usr/local/var/github/app/assets/javascripts/application.js")
assert_equal "/usr/local/var/github/app/assets/javascripts/foo bar.js",
parse_file_digest_uri("file-digest:///usr/local/var/github/app/assets/javascripts/foo%20bar.js")
assert_equal "C:/Users/IEUser/Documents/github/app/assets/javascripts/application.js",
parse_file_digest_uri("file-digest:///C:/Users/IEUser/Documents/github/app/assets/javascripts/application.js")
if DOSISH
assert_equal "C:/Users/IEUser/Documents/github/app/assets/javascripts/application.js",
parse_file_digest_uri("file-digest:///C:/Users/IEUser/Documents/github/app/assets/javascripts/application.js")
end
end

def test_build_file_digest_uri
Expand Down