varvet / godmin

Admin framework for Rails 5+
http://godmin-sandbox.herokuapp.com
MIT License
486 stars 47 forks source link

Slow rendering of table rows & cells #177

Closed Linuus closed 7 years ago

Linuus commented 8 years ago

If you render a lot of rows on an index page it is super slow.

Not sure if it is the Rails default partial rendering that is slow or if it is that we do a lot of checks for partials for each table cell or something... We should check if it is possible to speed it up though!

jensljungblad commented 8 years ago

I've been thinking that we could replace the partial locating stuff by creating small wrapper objects for columns and filters so that Rails can render the partials based on that somehow.

jensljungblad commented 8 years ago

I looked into this a bit recently and I'm not sure why I though ActiveModels worked the same as ActionController when it comes to rendering and inheritance. They don't... And the controllers pretty much do what we do in case of inheritance. They do a lookup using the lookup context. Our problem is that we do loads of those.

So my new idea is that we should cache the result of the partial lookup call. If it finds a partial for the column for the first row it will still be there when rendering the next row, so we don't need to check if it exists. That should give a good speed boost and not be too difficult to implement.

neves commented 8 years ago

Much better, but still slow in development. This is the log for 100 records:

  Rendered resource/columns/_actions.html.erb (24.2ms)
  Rendered resource/columns/_actions.html.erb (27.5ms)
  Rendered resource/columns/_actions.html.erb (13.2ms)
  ...
  Rendered resource/columns/_actions.html.erb (11.1ms)
  Rendered resource/columns/_actions.html.erb (8.7ms)
  Rendered resource/_table.html.erb (1824.1ms)

Now I removed all code from _actions.html.erb, but keeping the partial call in _table:

  Rendered resource/columns/_actions.html.erb (0.1ms)
  Rendered resource/columns/_actions.html.erb (0.0ms)
  Rendered resource/columns/_actions.html.erb (0.0ms)
...
  Rendered resource/columns/_actions.html.erb (0.0ms)
  Rendered resource/columns/_actions.html.erb (0.1ms)
  Rendered resource/columns/_actions.html.erb (0.1ms)
  Rendered resource/_table.html.erb (770.5ms)

Now I moved the code from from _actions to _table (inline, no partial) Rendered resource/_table.html.erb (860.0ms)

So the rails partial itself is slow for hundreds of tiny partial bits, like action buttons.

At last, I moved the code to a helper, the code got better but the speed a little slower, I don't know why: Rendered resource/_table.html.erb (935.3ms)

Here's the helper:

module GodminHelper
  def action_buttons(resource)
    content_tag :div, class: 'btn-group btn-group-sm pull-right' do
      concat(action_show resource) if policy(resource).show?
      concat(action_edit resource) if policy(resource).edit?
      concat(action_destroy resource) if policy(resource).destroy?
    end
  end

  def action_show(resource)
    link_to(
      translate_scoped("actions.show"),
      resource,
      class: "btn btn-default",
      title: translate_scoped("actions.show_title", resource: resource)
    )
  end

  def action_edit(resource)
    link_to(
      translate_scoped("actions.edit"),
      [:edit, resource],
      class: "btn btn-default",
      title: translate_scoped("actions.edit_title", resource: resource)
    )
  end

  def action_destroy(resource)
    link_to(
      translate_scoped("actions.destroy"),
      resource,
      method: :delete,
      class: "btn btn-danger",
      title: translate_scoped("actions.destroy_title", resource: resource),
      data: { confirm: translate_scoped("actions.confirm_message") }
    )
  end
end

But for reusability, I think it should be defined in a PresenterClass.

jensljungblad commented 8 years ago

Thanks for benchmarking this. Yeah we're open to other ways of customizing things. Right now we mostly use partial overrides but we've thought about using presenters as an alternative.

I'll just re-open this issue for now.

jensljungblad commented 7 years ago

See #223