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

Improve performance by moving template compilation from #render_in to #render_template_for #1302

Merged
merged 8 commits into from
Mar 18, 2022

Conversation

camertron
Copy link
Contributor

@camertron camertron commented Mar 10, 2022

Summary

Currently, every call to #render_in calls self.class.compile, which compiles the template and attaches a new #render_template_for method to the component's class. If the template has already been compiled and cached, the compiler skips compilation and returns immediately.

While calling .compile may at first appear innocuous, it requires four or five method calls that essentially do nothing every time (except the first) the component is rendered. These method calls are pure overhead.

This PR moves the .compile call from #render_in to a temporary #render_template_for method. This temporary method invokes the compiler (which replaces #render_template_for with the compiled version), then calls the compiled version. At first glance it looks like recursion 😜

Because the code that invokes the compiler is replaced on first render, cache invalidation needs to work a little differently. Instead of simply clearing the cache, the old stub methods also must be added back into the component classes. I agree it's a little unorthodox, but thanks to Ruby's flexibility it works pretty well. To my knowledge, cache invalidation is only used in dev and test environments anyway, so all this additional dynamic method stuff shouldn't affect prod environments.

Other Information

The performance impact of the change is fairly significant.

Main

Warming up --------------------------------------
           component   295.000  i/100ms
              inline   388.000  i/100ms
             partial    30.000  i/100ms
Calculating -------------------------------------
           component      2.703k (± 8.9%) i/s -     27.140k in  10.127987s
              inline      3.676k (± 6.0%) i/s -     36.860k in  10.064954s
             partial    295.506  (± 8.8%) i/s -      2.940k in  10.036790s

Comparison:
              inline:     3676.0 i/s
           component:     2702.8 i/s - 1.36x  (± 0.00) slower
             partial:      295.5 i/s - 12.44x  (± 0.00) slower

This branch

Warming up --------------------------------------
           component   329.000  i/100ms
              inline   433.000  i/100ms
             partial    30.000  i/100ms
Calculating -------------------------------------
           component      3.194k (± 5.0%) i/s -     31.913k in  10.018582s
              inline      4.123k (± 6.5%) i/s -     41.135k in  10.029222s
             partial    301.406  (± 6.6%) i/s -      3.030k in  10.103421s

Comparison:
              inline:     4123.4 i/s
           component:     3194.5 i/s - 1.29x  (± 0.00) slower
             partial:      301.4 i/s - 13.68x  (± 0.00) slower

That's a 15% speedup 😄

@camertron camertron requested review from elia and a team as code owners March 10, 2022 06:13
Copy link
Collaborator

@elia elia left a comment

Choose a reason for hiding this comment

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

This is awesome 🤩
15% is not an everyday gain 🙌

Left a comment on how we can rely on inheritance in order to avoid the re-definition.

lib/view_component/compile_cache.rb Outdated Show resolved Hide resolved
@camertron camertron requested a review from elia March 10, 2022 22:30
Copy link
Collaborator

@elia elia left a comment

Choose a reason for hiding this comment

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

Awesome, left a few nits
But pre-approving in the meantime 👍

lib/view_component/compile_cache.rb Outdated Show resolved Hide resolved
lib/view_component/compiler.rb Outdated Show resolved Hide resolved
test/test_helper.rb Outdated Show resolved Hide resolved
@camertron camertron merged commit 80ad316 into main Mar 18, 2022
@camertron camertron deleted the compile_less branch March 18, 2022 23:19
aduth added a commit to 18F/identity-idp that referenced this pull request Mar 22, 2022
**Why**: 15% render performance bump, according to ViewComponent/view_component#1302 .

changelog: Internal, Dependencies, Update dependencies to latest version
aduth added a commit to 18F/identity-idp that referenced this pull request Mar 22, 2022
**Why**: 15% render performance bump, according to ViewComponent/view_component#1302 .

changelog: Internal, Dependencies, Update dependencies to latest version
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants