Skip to content

Commit 6d15620

Browse files
authored
Merge pull request #2758 from ViewComponent/joelhawksley-benchmark-optimization
Speed up component rendering and eager loading
2 parents 3ae54b3 + 6ae6e3e commit 6d15620

5 files changed

Lines changed: 113 additions & 14 deletions

File tree

‎docs/CHANGELOG.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,10 @@ nav_order: 6
1010

1111
## main
1212

13+
* Speed up eager loading by sharing sidecar directory listings during compilation, and reduce per-render state reset overhead.
14+
15+
*Joel Hawksley*
16+
1317
* Ignore unresolved partial dependencies inferred from dynamic renders, such as `render UI.button`, without requiring configuration for component facades. Real partials and explicit dependencies are preserved.
1418

1519
*Joel Hawksley*

‎lib/view_component/base.rb‎

Lines changed: 4 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -508,16 +508,10 @@ def __vc_safe_around_render_output(output)
508508
# slot setters or `with_content`); reuse of an instance that has slot or
509509
# `with_content` state is guarded against separately by
510510
# `__vc_check_reused_instance!`.
511-
RENDER_STATE_IVARS = %i[
512-
@__vc_controller
513-
@__vc_helpers
514-
@__vc_request
515-
].freeze
516-
517511
def __vc_reset_render_state!
518-
RENDER_STATE_IVARS.each do |ivar|
519-
remove_instance_variable(ivar) if instance_variable_defined?(ivar)
520-
end
512+
remove_instance_variable(:@__vc_controller) if defined?(@__vc_controller)
513+
remove_instance_variable(:@__vc_helpers) if defined?(@__vc_helpers)
514+
remove_instance_variable(:@__vc_request) if defined?(@__vc_request)
521515
end
522516

523517
# Raises when a component instance is rendered more than once.
@@ -915,7 +909,7 @@ def __vc_initialize_signatures
915909
end
916910

917911
def __vc_files_ending_with(extensions, pattern)
918-
paths = Dir[pattern]
912+
paths = CompileCache.sidecar_files_matching(pattern)
919913
extensions.flat_map { |extension| paths.select { |path| path.end_with?(extension) } }
920914
end
921915

‎lib/view_component/compile_cache.rb‎

Lines changed: 22 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -26,14 +26,16 @@ def invalidate!
2626
cache.clear
2727
end
2828

29-
# Sidecar files can't change while the application eager loads: within the
30-
# block, each component's are looked up once. Fiber storage rather than
31-
# module state, so the cache is gone once boot is done.
29+
# Sidecar files can't change while the application eager loads: share
30+
# directory listings and look up each component's files once within the
31+
# block. Fiber storage keeps the cache from surviving boot.
3232
def with_sidecar_files_cache
3333
Fiber[:__vc_sidecar_files] = {}
34+
Fiber[:__vc_sidecar_directories] = {}
3435
yield
3536
ensure
3637
Fiber[:__vc_sidecar_files] = nil
38+
Fiber[:__vc_sidecar_directories] = nil
3739
end
3840

3941
def sidecar_files_for(klass, extensions)
@@ -44,6 +46,23 @@ def sidecar_files_for(klass, extensions)
4446
sidecar_files[extensions] ||= yield.freeze
4547
end
4648

49+
def sidecar_files_matching(pattern)
50+
directories = Fiber[:__vc_sidecar_directories]
51+
return Dir[pattern] unless directories
52+
53+
filename = File.basename(pattern)
54+
prefix = filename.delete_suffix(".*")
55+
return Dir[pattern] if prefix.match?(/[*?{}\[\]\\]/)
56+
57+
directory = File.dirname(pattern)
58+
# Index the shared directory by component name instead of scanning all
59+
# its entries again for every component and extension.
60+
entries = directories[directory] ||= Dir.glob("#{directory}/*", File::FNM_DOTMATCH)
61+
.group_by { |path| File.basename(path).split(".", 2).first }
62+
candidates = entries.fetch(prefix.split(".", 2).first, [])
63+
candidates.select { |path| File.fnmatch?(filename, File.basename(path)) }
64+
end
65+
4766
# Invalidates the cache when a template or a translation file is added,
4867
# changed, or removed. Changes to Ruby files reload the application instead.
4968
def template_watcher(file_watcher)

‎test/sandbox/test/base_test.rb‎

Lines changed: 73 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -100,13 +100,85 @@ def test_sidecar_files_are_looked_up_once_while_eager_loading
100100

101101
File.write(template, "<div></div>")
102102
assert_empty component.__vc_sidecar_files(["erb"])
103-
assert_equal [template], component.sidecar_files(["erb"])
103+
assert_empty component.sidecar_files(["erb"])
104104
end
105105

106106
assert_equal [template], component.__vc_sidecar_files(["erb"])
107107
end
108108
end
109109

110+
def test_cached_sidecar_directory_listings_preserve_file_discovery
111+
components = [
112+
TemplateAndSidecarDirectoryTemplateComponent,
113+
CssSidecarFileComponent,
114+
TranslatableComponent,
115+
SlotsComponent::MyHighlightComponent
116+
]
117+
extensions = %w[erb css yml]
118+
expected = components.map { |component| component.sidecar_files(extensions) }
119+
120+
ViewComponent::CompileCache.with_sidecar_files_cache do
121+
assert_equal expected, components.map { |component| component.__vc_sidecar_files(extensions) }
122+
end
123+
end
124+
125+
def test_sidecar_directory_listings_are_shared_across_components_and_extensions
126+
Dir.mktmpdir do |directory|
127+
paths = %w[first_component.html.erb first_component.yml second_component.html+phone.erb].map do |name|
128+
File.join(directory, name).tap { |path| File.write(path, "") }
129+
end
130+
scans = 0
131+
glob = Dir.method(:glob)
132+
133+
scan = lambda do |*args|
134+
scans += 1
135+
glob.call(*args)
136+
end
137+
138+
Dir.stub(:glob, scan) do
139+
ViewComponent::CompileCache.with_sidecar_files_cache do
140+
assert_equal paths.first(2).sort, ViewComponent::CompileCache.sidecar_files_matching("#{directory}/first_component.*")
141+
assert_equal [paths.last], ViewComponent::CompileCache.sidecar_files_matching("#{directory}/second_component.*")
142+
assert_equal 1, scans
143+
end
144+
end
145+
end
146+
end
147+
148+
def test_sidecar_directory_listings_are_discarded_after_an_error
149+
Dir.mktmpdir do |directory|
150+
pattern = "#{directory}/temporary_component.*"
151+
assert_raises(RuntimeError) do
152+
ViewComponent::CompileCache.with_sidecar_files_cache do
153+
assert_empty ViewComponent::CompileCache.sidecar_files_matching(pattern)
154+
raise "compilation failed"
155+
end
156+
end
157+
158+
template = File.join(directory, "temporary_component.html.erb")
159+
File.write(template, "")
160+
assert_equal [template], ViewComponent::CompileCache.sidecar_files_matching(pattern)
161+
ViewComponent::CompileCache.with_sidecar_files_cache do
162+
assert_equal [template], ViewComponent::CompileCache.sidecar_files_matching(pattern)
163+
end
164+
end
165+
end
166+
167+
def test_cached_sidecar_directory_listings_preserve_glob_matching
168+
Dir.mktmpdir do |directory|
169+
%w[example.html.erb example.part.html.erb example_other.html.erb .example.html.erb].each do |name|
170+
File.write(File.join(directory, name), "")
171+
end
172+
patterns = %w[example.* example.part.* example_other.* .example.* example{,_other}.* missing.* subdirectory/example.*]
173+
.map { |name| File.join(directory, name) }
174+
expected = patterns.map { |pattern| Dir[pattern] }
175+
176+
ViewComponent::CompileCache.with_sidecar_files_cache do
177+
assert_equal expected, patterns.map { |pattern| ViewComponent::CompileCache.sidecar_files_matching(pattern) }
178+
end
179+
end
180+
end
181+
110182
def test_does_not_render_additional_newline_with_render_in
111183
without_template_annotations do
112184
ActionView::Template::Handlers::ERB.strip_trailing_newlines = true

‎test/sandbox/test/reused_instance_test.rb‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,16 @@ def test_helpers_controller_request_are_re_derived_on_each_render
9999
end
100100
end
101101

102+
def test_reset_render_state_removes_nil_memoized_values
103+
component = ContextSnapshotComponent.new
104+
variables = %i[@__vc_controller @__vc_helpers @__vc_request]
105+
variables.each { |variable| component.instance_variable_set(variable, nil) }
106+
107+
component.send(:__vc_reset_render_state!)
108+
109+
variables.each { |variable| refute component.instance_variable_defined?(variable) }
110+
end
111+
102112
def test_rendering_same_collection_twice_does_not_raise
103113
# Collection rebuilds child components per render, so the same Collection
104114
# object can be safely rendered twice (each child instance is fresh).

0 commit comments

Comments
 (0)