komposable / komponent

An opinionated way of organizing front-end code in Ruby on Rails, based on components
http://komponent.io
MIT License
427 stars 31 forks source link

Performance #122

Closed aldhsu closed 4 months ago

aldhsu commented 5 years ago

We are considering using Komponent but I have a question regarding the performance of rendering.

A contrived example:

# _button_component.html.erb
<button class="button"><%= button_text %></button>

# _user_component.html.erb
<div class="user">
  <h3><% user.full_name %></h3>
  <%  ["Message", "Block", "Details"].each do |action| %>
    <%= c "button", button_text: action %>
  <% end %>
</div>

#users/index.html.erb
<% User.all.each do |user| %>
  <%= c "user", user: user %>
<% end %>

screen shot 2018-10-26 at 3 24 21 pm

Nested partials is a known issue for performance in Rails as the partials are all read from disk, everytime they are used. They recommend using the collection helper so that the partial can be read once for a collection. Any tips on how to avoid these issues? Have you found caching is sufficient?

Spone commented 5 years ago

Hi @aldhsu, thanks for considering using Komponent!

We use it in production on several websites and performance hasn't been an issue for now. You can always use component caching if you need to improve it.

In the future, we could also add a collection helper (component_collection maybe?) and achieve the same kind of optimization as Rails does with render partial: "item", collection: @items

aldhsu commented 5 years ago

OK thanks for the reply @Spone . I'll do some more testing and post the results back here. Closing for now.

aldhsu commented 5 years ago

For future reference. I did some more digging and this is more of a problem in development. Nested partials and using render without the collection helper will still degrade performance as expected in production, but it may still fit in your performance envelope.

In production, Rails has a variety of optimisations for partial rendering (caching the path etc.).

Fresh install of Rails no other gems. Tested using Apache Bench ab -n 50 http://127.0.0.1:8000/.

<%# using Komponent %>
<% 40.times do %>
  <%= c('wrapper') do %>
    <%= c 'wrapper' do %>
      Hello
    <% end %>
    <%= c 'wrapper' do %>
      Hallo
    <% end %>
    <%= c 'wrapper' do %>
      Hola
    <% end %>
  <% end %>
<% end %>
<%# Inline %>
<% 40.times do %>
  <div class="wrapper">
    <div class='wrapper'>
      Hello
    </div>
    <div class='wrapper'>
      Hallo
    </div>
    <div class='wrapper'>
      Hola
    </div>
  </div>
<% end %>

Average Response Time (milliseconds)

Environment Development Production
Komponent 400-800* 75
Inline template 133 5

*I had to eyeball the Komponent in development because for some reason it runs extremely slow through Apache Bench (4000s) but only took 1/10th of that through a browser.

Spone commented 5 years ago

Thanks for benchmarking it!

I also noticed a slowdown in development, on component-heavy pages, but I'm not sure how we could solve that, since we can't really use cache while developing...

aldhsu commented 5 years ago

@Spone I see this as ActionView's problem. A performance optimisation could be made to keep the compiled template in memory during a single page render rather than going to disk everytime. Performance would then be more inline with the partials rendered with the collection option for components that are used adhoc throughout a page.

If you have a collection to render I recommend using render option render <partial>, collection: <collection> until Komponent builds it in to the helper.

For the above example, change to something like:

<%# index.html.erb %>
<%= render "komponent_hellos", collection: (1..40).to_a %>
<%# komponent_hellos.html.erb %>
<%= c('wrapper') do %>
  <%= c 'wrapper' do %>
    Hello
  <% end %>
  <%= c 'wrapper' do %>
    Hallo
  <% end %>
  <%= c 'wrapper' do %>
    Hola
  <% end %>
<% end %>

I am able to benchmark this version in development (Apache Bench was reporting mean response of 4s before) at 327ms and it brings down production response time to 10ms.

Spone commented 5 years ago

We will definitely consider adding a component_collection helper then. Let me reopen the issue.

Spone commented 5 years ago

Also, it depends on your specific case, but if you render a parameter-less component, a quick win is to render the component once, capture it to a string, and reuse the string subsequently, like this:

<% button_component = capture { c 'button' } %>

<% 40.times do %>
    <%= button_component %>
<% end %>
xdmx commented 5 years ago

I'm also considering komponent as an alternative to structure my front end components, and I've run some tests with different variances.

You can find the code here (I've borrowed the base from @aldhsu repo :raised_hands: ).

These are the results (in ms) I've got by running ab -n 50 http://127.0.0.1:3000/pages/index{1,2,3,4,5,6,7}:

    mean development mean production
index1 komponent 575 80
index2 card 260 13
index3 html 31 5
index4 card2 972 32
index5 card2 collection 769 29
index6 card collection 40 5
index7 komponent collection 593 84

Obviously html is the fastest one as it doesn't have to load any file. What I was surprised to see is the card2 and card2 collection timings in development, which decreased a lot in production.

I also didn't get any improvement between komponent and komponent collection, I even tried to add cached: true but it didn't change anything :thinking:

Spone commented 5 years ago

Hi @xdmx, thanks for running these benchmarks!

We definitely have a lot to improve in terms of performance, especially in development.

I think it's normal you get similar results between your index1 and index7 test because you're doing nearly the same thing (rendering 50 times the nested_yielder component).

We welcome all contributions on this matter :) I think the lowest hanging fruit is adding a component_collection helper that would read the partial file only once.

xdmx commented 5 years ago

Yeah, development could definitely use some speed up.

About the index1 and index7, I wanted to check out the suggestion to use collection from @aldhsu previous comment, but I couldn't get that improvement even by using it.

I think the issue, and the thing to improve are nested components as well. component_collection may help to load the partial file only once, but it also need to load any other nested partials/components only once, otherwise there's still be slowdowns for loading those. I have no idea how to achieve this though :grimacing:

langsharpe commented 5 years ago

I ran index1 (the nested component test) above through Stackprof, and found something interesting.

==================================
  Mode: cpu(1000)
  Samples: 212 (0.00% miss rate)
  GC: 47 (22.17%)
==================================
     TOTAL    (pct)     SAMPLES    (pct)     FRAME
        47  (22.2%)          47  (22.2%)     (garbage collection)
        19   (9.0%)          19   (9.0%)     Pathname#chop_basename
        17   (8.0%)          17   (8.0%)     ActiveSupport::SafeBuffer#initialize
       160  (75.5%)          12   (5.7%)     Komponent::ComponentRenderer#_render
        20   (9.4%)           8   (3.8%)     Pathname#plus
         7   (3.3%)           6   (2.8%)     ActiveSupport::Inflector#inflections
         6   (2.8%)           6   (2.8%)     ActionView::Base#assign
         6   (2.8%)           6   (2.8%)     ActionView::Resolver.caching
        34  (16.0%)           5   (2.4%)     Pathname#join
        12   (5.7%)           5   (2.4%)     ActiveSupport::Inflector#camelize
        18   (8.5%)           4   (1.9%)     Komponent::ComponentPathResolver#path_has_component?
         4   (1.9%)           4   (1.9%)     ActiveSupport::SafeBuffer#html_safe?
         6   (2.8%)           4   (1.9%)     ActiveSupport::SafeBuffer#concat
         4   (1.9%)           4   (1.9%)     Set#include?
        37  (17.5%)           3   (1.4%)     Komponent::ComponentRenderer#resolved_component_path

At least 8.5% of the time is being spent in ComponentPathResolver. By adding a cache performance can be improved.

Using ab -n 50 http://127.0.0.1:3000/pages/index{1,2,3,4,5,6,7} as above
Production mode

index4 - 26ms 
index1 (without resolver cache) - 92ms 
index1 (with resolver cache) - 68ms

It's not a huge win but I think it would benefit from caching, especially on pages that render a lot of components.

Spone commented 5 years ago

Hi @langsharpe thanks for taking the time to run some tests!

Do you see any drawback in including this cache mechanism by default? Would you mind opening a PR for this?

langsharpe commented 5 years ago

Do you see any drawback in including this cache mechanism by default? The only drawback is it wouldn't work in development mode. It would probably need to be configurable.

It would also add concurrent-ruby as a dependency. I'm not very familiar with it. I just copy and pasted what the Rails gem does with it.

aldhsu commented 5 years ago

I had this discussion with @langsharpe and thought this might be interesting.

I was trying to flesh out my understanding of what the partial rendering for a collection does to speed up rendering: https://github.com/rails/rails/blob/e69ff43060c1194d2a3bd9b8d9e23f3ae26b84b5/actionview/lib/action_view/renderer/partial_renderer.rb#L448-L468 Pointing at the wrong code, should be: https://github.com/rails/rails/blob/e69ff43060c1194d2a3bd9b8d9e23f3ae26b84b5/actionview/lib/action_view/renderer/partial_renderer.rb#L426-L446

This looks superficially similar to what you have done @langsharpe with caching of the path. I think the difference is it's not just the lookup of the file. It seems like find_template actually instantiates some evaluated ERB (maybe), so this is cached as well. I'm inferring this because it can then just call template.render(view, locals) with the template pulled out of the cache.

I think there could be gains if we could cache lib/komponent/component_renderer.rb#_render in a similar way.

Alternatively, I have been looking at the code for https://github.com/jensljungblad/components (inspired by Komponent) which uses classes, rather than require_dependency the module.

It's render method is just @view.render partial: to_partial_path, object: self.

If you have a class then you can cache the template as a class var (probably still turned off in dev) using find_template like Rails does and rebind locals and the view everytime render is called.

I could be way off base I haven't tried any of the above.

Spone commented 5 years ago

That's super interesting, thanks for sharing these ideas! Also I'm discovering the components library which looks promising!

Would you be willing to contribute code for improving this?

aldhsu commented 5 years ago

I had a crack at making a proof of concept https://github.com/fivegoodfriends/komponent/tree/cache-templates to see what kind of gain could be expected.

Results aren't as drastic as I would have hoped. Scenario: @langsharpe test for index1, 500 times component: In production mode

State Mean
Without Cache 463ms
Cached 316ms
Spone commented 5 years ago

Thanks for giving it a try :+1: What about development mode? As your solution is using memoization, it can be safely used in development, right?

aldhsu commented 5 years ago

I haven't really thought about whether it was safe to do or not yet. I would have to look into the internals of the controller and template. My assumptions are you get a new controller every request and the cached template has no identifying information.

It seems to make a huge difference in development: @langsharpe test for index1, 500 times component. In development mode

State Mean
WIthout Cache 7078ms
Cached 365ms

This makes sense because in development the paths aren't cached and it's one of hot paths with this kind of rendering. The change I made to caching the template, means this is skipped.

I'll have to dig into the collection view some more to find out how they are getting 10x improvements vs an collection.each do render