Add configuration and support for freezing string literals in compiled templates

[mitchellhenke]

What are you trying to accomplish?

This is a proof-of-concept of closing #1883 (if it's helpful for discussion there)

What approach did you choose and why?

I mostly followed the structure of the Rails pull request. I'm not super familiar with templating engines or their implementation, so I don't have a lot to draw on unfortunately.

Anything you want to highlight for special attention from reviewers?

The configuration feels kind of weird. Rails has a configuration for this, and it might be reasonable to consider inheriting that configuration rather than have a separate one for view_component?

I suspect most people enabling it in Rails (it defaults to false) would want to have it for their view_component templates, but it does potentially break applications if they are mutating strings.

[boardfish]

You raise a good point about delegating back to Rails' own config for this. It would be possible to inherit from the Rails default from within the defaults method since it isn't called until Rails is ready to receive that. I'd recommend doing so and updating the docs accordingly.

[boardfish]

SimpleCov says this line's not tested, just FYI!

[mitchellhenke]

Thanks for catching that, added a test in b15f939 which should cover the inline template case.

[mitchellhenke]

You raise a good point about delegating back to Rails' own config for this. It would be possible to inherit from the Rails default from within the defaults method since it isn't called until Rails is ready to receive that. I'd recommend doing so and updating the docs accordingly.

I tried some quick testing and it looks like defaults is called when the gem is required, which is prior to the configuration being set (at least in my application). My effort started with 7f0db9a, is there a different approach that could work better?

[boardfish]

Ah, that probably does come ahead of, say, config/application.rb or an initializer in a way that is non-obvious. We could advise folks in the docs or changelog for this option that they can mirror Rails' config option across wherever they're setting config for ViewComponent, but that it's dependent on load order.

[mitchellhenke]

The other option I thought about was defaulting to :rails or similar and using the Rails configuration value at template compile-time if . It's a little goofier of an implementation though to have to check in that way (and it would also seem to affect inherited):

    def render_in(view_context, &block)
      if ViewComponent::Base.config.frozen_string_literal == :rails
        self.class.compile(raise_errors: true, frozen_string_literal: Rails.application.config.action_view.frozen_string_literal)
      else
        self.class.compile(raise_errors: true, frozen_string_literal: ViewComponent::Base.config.frozen_string_literal)
      end
      # ...
    end

[mitchellhenke]

Sorry for the repeated comments, I tried another approach in 60a92d2 to make use of the "view_component.set_configs" initializer. I think I prefer it to the above compile-time conditionals, but I'm open to direction on this.

[boardfish]

This is awesome! I wonder if it'd be possible to write a test to ensure that the value is inherited from Rails' config?

[mitchellhenke]

This is awesome! I wonder if it'd be possible to write a test to ensure that the value is inherited from Rails' config?

I tried, and it got a little bit weird in 385cb87. Open to suggestions on how to structure it, which test cases are important, etc.

I'm also getting a failure in CI / Test compatibility with Primer ViewComponents, but I can't tell what change of mine would have broken it?

[boardfish]

That seems like the right way to do it to me! Awesome work figuring that out – I would never have guessed how to do it myself 🤯

I'm hoping it's just a flaky spec failure, so I've rerun the job. If this goes green, I think we can approve it!

[mitchellhenke]

Thanks!

For the spec failure, I merged main to see if that would help, but main seems to be failing too. It looks like there may be a couple issues in the Primer ViewComponents test. Initially I was getting something along the lines of:

Failure:
Primer::ViewComponents::ConstantsTest#test_get_symbolizes_array_elements [/Users/mitchellehenke/projects/view_components/test/lib/constants_test.rb:22]:
--- expected
+++ actual
@@ -1 +1 @@
-[":button", ":a", ":summary", ":clipboard-copy"]
+[":button", ":a", ":summary"]



bin/rails test /Users/mitchellehenke/projects/view_components/test/lib/constants_test.rb:15

.F

Failure:
Primer::ViewComponents::ConstantsTest#test_get_array [/Users/mitchellehenke/projects/view_components/test/lib/constants_test.rb:12]:
--- expected
+++ actual
@@ -1 +1 @@
-["button", "a", "summary", "clipboard-copy"]
+["button", "a", "summary"]

But the newer failure appears to be related to gem versions.

[mitchellhenke]

Looks like updating to Rails 7.1.1 here in the Primer tests fixes the gem issue

[reeganviljoen]

Looks like updating to Rails 7.1.1 here in the Primer tests fixes the gem issue

@mitchellhenke I had the same issue by merging main back into your branch your issues should be fixed

[mitchellhenke]

Thanks, rebased, and it looks like CI is happy!

I can squash commits or whatever is preferred prior to merging if needed.

[camertron]

Wow, great work @mitchellhenke, thank you!

Unfortunately I'm not seeing any performance improvements when running the ViewComponent benchmarks. It looks like the ERB template engine already adds .freeze onto literal strings, but even if I remove those calls I still see no change in performance. I'm not against this change at all, and I think it could be beneficial for other template engines like HAML, Slim, etc, but I don't understand why there's no perf impact. Any ideas here?

[mitchellhenke]

Wow, great work @mitchellhenke, thank you!

Unfortunately I'm not seeing any performance improvements when running the ViewComponent benchmarks. It looks like the ERB template engine already adds .freeze onto literal strings, but even if I remove those calls I still see no change in performance. I'm not against this change at all, and I think it could be beneficial for other template engines like HAML, Slim, etc, but I don't understand why there's no perf impact. Any ideas here?

Thanks! I think most of the benefit will be in memory usage, with maybe a tiny performance gain, depending on the application and templates.

I started a memory-benchmark branch to try to set up both a memory performance benchmark and a performance benchmark of rendering with and without freezing string literals. A minimal template that where a significant portion is made up of string literals in Ruby shows some memory improvements and an insignificant "improvement" in performance.

The example template is intended to try to get some of the benefit of string freezing by putting a literal in a Ruby block. The template and results are below, but I think it's worth trying a variety to see the impact. In running performance benchmarks. I'm also not sure I've set up the benchmarks appropriately.

<p><%= "string" %></p>

# bundle exec rake memory_benchmark
no freezing
Total allocated: 668247 bytes (12340 objects)
Total retained:  4293 bytes (33 objects)

freezing
Total allocated: 602105 bytes (11114 objects)
Total retained:  600 bytes (7 objects)

-9.90% difference in bytes allocated
-9.94% difference in object allocations
Calculating -------------------------------------
          not frozen    358.882k (±19.1%) i/s -      6.104M in  18.927677s
              frozen    365.115k (±19.1%) i/s -      6.294M in  18.897046s

Comparison:
              frozen:   365114.5 i/s
          not frozen:   358882.4 i/s - same-ish: difference falls within error

[mitchellhenke]

To add a bit, I totally understand if the complexity is not worth the improvement 🙂

[camertron]

@mitchellhenke ahh ok that makes sense 👍 I thought we would see a significant speed-up as well, since Ruby would have to execute fewer malloc calls etc, but I think Ruby actually just keeps the memory it has already malloced and re-uses it. Another reminder it's always good to validate your assumptions, especially when it comes to perf 😅

...depending on the application and templates.

Yeah, I wonder if an app like GitHub will see memory savings over time with this change. My guess is no because the ERB templating engine already freezes the majority of literal strings in a given template.

To add a bit, I totally understand if the complexity is not worth the improvement

🤔 I'm chatting with another maintainer about this. It's really not too bad complexity-wise, so I'm inclined to accept the PR. Will circle back today.

[camertron]

Ok @mitchellhenke the consensus is we'd like to see some numbers from a real-world app before merging. Is that feasible for you?

[mitchellhenke]

@camertron I can definitely try! I found some issues that prevented the string freezing from happening in production/eager load environments and applied a patch in fba786e.

I suspect applications doing more than static content may not be able to see it on the speed performance side (depending on their usage of view_component).

A simple rails new application with one component being rendered sees a small, though significant, improvement via derailed_benchmarks (repo here)

$ DERAILED_SCRIPT_COUNT=500 TEST_COUNT=100 derailed exec perf:app
[...]

❤️ ❤️ ❤️  (Statistically Significant) ❤️ ❤️ ❤️

[452601c] (0.2497 seconds) "freeze" ref: "452601cd94f5f35d840e143067287d6066e67ee2"
  FASTER 🚀🚀🚀 by:
    1.0075x [older/newer]
    0.7444% [(older - newer) / older * 100]
[daf373f] (0.2515 seconds) "no freeze" ref: "daf373fdd45458cbb44b1772ecff57bcd27f02fd"

Iterations per sample: 100
Samples: 353

Test type: Kolmogorov Smirnov
Confidence level: 99.0 %
Is significant? (max > critical): true
D critical: 0.11429933888451545
D max: 0.20265419778521762

A more complex repo I tested on didn't have a significant difference (though it was a very very small improvement).

[reeganviljoen]

@camertron I think the best way to get actual production metrics is to include it in a release as experimental maybe config.experimental.freeze.strings and ask for feedback in the docs

[mitchellhenke]

Happy to make adjustments to the configuration, just let me know!

[mitchellhenke]

Wanted to check in on this quick 🙂

I am happy to make changes to clarify this as more experimental and change the default to disabling if that's preferable.

[joelhawksley]

@mitchellhenke thanks for taking the time to dig into this idea! Given how intrusive the change is, I'm hesitant to merge it even as an experimental flag given the current (very thorough ❤️) benchmark data.

Would you be willing to run your fork in production to get a real-world impact of the change?

[mitchellhenke]

@joelhawksley I'll see if I can. I've got a little bit of prior work to take care of first (we're a bit behind on view_component versions already and I can't guarantee a production test, but I'd like to.

Failing real production, I can try a more "prod-like" environment.

[reeganviljoen]

@mitchellhenke any updates from your end ?

[mitchellhenke]

@reeganviljoen sorry for the delay!

I did a test in a deployed environment and the difference in performance is not significant. I think the potential for memory reduction may be helpful in the cases where there many or large string constants in the templates. I don't see much of that use case in my experience, so I'm guessing the changes here are not worthwhile.

I am glad to close this PR, but appreciate you all taking the time to provide feedback and evaluate it.

[reeganviljoen]

@joelhawksley any comments ?

[joelhawksley]

I don't think it's worth the trouble at this point. Thank you for the exploratory work @mitchellhenke ❤️