Skip to content

Commit 753e48e

Browse files
committed
Fix performance regression in Rails helper
Previously reported performance regression #234 was a red herring. The `Engine` class and addition of adding `app/` asset path appears to be a big problem, but the reason it's slow is because then it would run through `precompiled_assets` where without that code it skips `precompiled_assets` thus skipping the code that is actually slow. Although `precompiled_assets` was memoized the cache wasn't actually working and the method cache was being thrown away every single time. This became very obvious when our test suite (Basecamp) went from 54 seconds to 320 seconds to run on sprockets-rails master. The problem is that when the env is reset the `precompiled_assets` gets reset instead of keeping the cache. I moved this method onto the config to behave like the other methods, for example `assets_manifest`. Benchmarks: On a brand new application with a single show test that simply renders a view with a few assets. Before this change: ``` Calculating ------------------------------------- Running test... 27.000 i/100ms ------------------------------------------------- Running test... 305.554 (± 8.2%) i/s - 1.539k ``` After this change: ``` Calculating ------------------------------------- Running test... 55.000 i/100ms ------------------------------------------------- Running test... 558.135 (± 5.4%) i/s - 2.805k ```
1 parent 6389649 commit 753e48e

File tree

4 files changed

+16
-7
lines changed

4 files changed

+16
-7
lines changed

lib/sprockets/rails/helper.rb

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ def initialize(source)
2121
include Sprockets::Rails::Utils
2222

2323
VIEW_ACCESSORS = [:assets_environment, :assets_manifest,
24-
:assets_precompile,
24+
:assets_precompile, :precompiled_assets,
2525
:assets_prefix, :digest_assets, :debug_assets]
2626

2727
def self.included(klass)
@@ -227,11 +227,6 @@ def lookup_debug_asset(path, options = {})
227227

228228
asset
229229
end
230-
231-
# Internal: Generate a Set of all precompiled assets logical paths.
232-
def precompiled_assets
233-
@precompiled_assets ||= assets_manifest.find(assets_precompile || []).map(&:logical_path)
234-
end
235230
end
236231
end
237232
end

lib/sprockets/rails/utils.rb

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,11 @@ module Utils
66
def using_sprockets4?
77
Gem::Version.new(Sprockets::VERSION) >= Gem::Version.new('4.0.0')
88
end
9+
10+
# Internal: Generate a Set of all precompiled assets logical paths.
11+
def build_precompiled_list(manifest, assets)
12+
manifest.find(assets || []).map(&:logical_path)
13+
end
914
end
1015
end
1116
end

lib/sprockets/railtie.rb

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,9 @@ class Configuration
2626

2727
# Returns Sprockets::Manifest for app config.
2828
attr_accessor :assets_manifest
29+
30+
# Returns array of already precompiled assets
31+
attr_accessor :precompiled_assets
2932
end
3033

3134
class Engine < Railtie
@@ -150,8 +153,11 @@ def self.build_manifest(app)
150153
app.routes.prepend do
151154
mount app.assets => config.assets.prefix
152155
end
156+
app.assets_manifest = build_manifest(app)
157+
app.precompiled_assets = build_precompiled_list(app.assets_manifest, config.assets.precompile)
158+
else
159+
app.assets_manifest = build_manifest(app)
153160
end
154-
app.assets_manifest = build_manifest(app)
155161

156162
ActionDispatch::Routing::RouteWrapper.class_eval do
157163
class_attribute :assets_prefix
@@ -176,6 +182,7 @@ def self.build_manifest(app)
176182

177183
self.assets_environment = app.assets
178184
self.assets_manifest = app.assets_manifest
185+
self.precompiled_assets = app.precompiled_assets
179186
end
180187
end
181188
end

test/test_helper.rb

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ def setup
2525
@view.assets_manifest = @manifest
2626
@view.assets_prefix = "/assets"
2727
@view.assets_precompile = %w( manifest.js )
28+
@view.precompiled_assets = @view.build_precompiled_list(@manifest, @view.assets_precompile)
2829
@view.request = ActionDispatch::Request.new({
2930
"rack.url_scheme" => "https"
3031
})
@@ -729,6 +730,7 @@ def test_stylesheet_link_tag_integrity
729730
class AssetUrlHelperLinksTarget < HelperTest
730731
def test_precompile_allows_links
731732
@view.assets_precompile = ["url.css"]
733+
@view.precompiled_assets = @view.build_precompiled_list(@manifest, @view.assets_precompile)
732734
assert @view.asset_path("url.css")
733735
assert @view.asset_path("logo.png")
734736

0 commit comments

Comments
 (0)