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

Cache various lookups with Dry::Core::Cache #97

Merged
merged 2 commits into from
Dec 21, 2018
Merged

Conversation

timriley
Copy link
Member

This PR uses Dry::Core::Cache to cache a few lookups that could otherwise be costly:

  • Templates within paths
  • Part classes within namespaces
  • Scope classes within namespaces

@GustavoCaso I hope you don't mind me going ahead with this, I wanted to play around with what we could cache. And I figured we should use our existing caching helper from dry-core.

Before this change, we were ~5.5x slower than ActionController.

Warming up --------------------------------------
   action_controller     1.000  i/100ms
            dry-view     1.000  i/100ms
Calculating -------------------------------------
   action_controller      6.519  (± 0.0%) i/s -     33.000
            dry-view      1.171  (± 0.0%) i/s -      6.000

Comparison:
   action_controller:        6.5 i/s
            dry-view:        1.2 i/s - 5.56x slower

After this change, we're ~1.8x slower:

Warming up --------------------------------------
   action_controller     1.000  i/100ms
            dry-view     1.000  i/100ms
Calculating -------------------------------------
   action_controller      5.995  (±16.7%) i/s -     30.000
            dry-view      3.317  (± 0.0%) i/s -     17.000

Comparison:
   action_controller:        6.0 i/s
            dry-view:        3.3 i/s - 1.81x slower

Here's the latest profile diagram (run using Hotch and a new script I checked into the repo): bundle exec ruby benchmarks/profile_controller.rb

profile

So no more low hanging fruit, as far as I can tell? Would definitely appreciate any thoughts on taking things from here. 👋 @GustavoCaso @solnic

This was referenced Dec 21, 2018
@GustavoCaso
Copy link
Member

@solnic I decided not to use dry/core/cache because according to the docs:

Allows you to cache call results that are solely determined by arguments.

In my opinion the template? method cannot be solely cache by there arguments:

def template?(name, format)

name and format could be the same for multiple directories so that is why I need to create a unique key using dir as well. But seing @timriley changes looks like we can pass multiple arguments to dry/core/cache fetch_or_store(root, dir, name, format) do

I guess would be nice to update the docs as well 😄

Copy link
Member

@GustavoCaso GustavoCaso 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 great!! 🎉 🎉

@solnic
Copy link
Member

solnic commented Dec 21, 2018

@timriley LGTM, it's already a great start. We should be able to make it at least equally fast eventually. I'll take another look at perf improvements once I get a chance.

@timriley
Copy link
Member Author

@solnic cool, will do. FWIW I just moved some of the caching a little earlier, with
e178d41, and we're now just ~1.3-1.4x slower.

It's worth recognising that we're certainly doing a lot more in the dry-view side of this benchmark versus action view, at least in how we're preparing the scope for the template, e.g. checking for exposure dependencies, wrapping up values in parts, finding classes, etc., so this isn't truly an apples-for-apples comparison. Perhaps a better one would be e.g. using Draper to decorate every ivar in the Rails controller before it's passed to the template.

@solnic
Copy link
Member

solnic commented Dec 21, 2018

One more thing, MRI 2.6.0 has various optimizations that we benefit from, so here are the results running MRI 2.6.0.rc2:

∞ [ruby-2.6.0-rc2][node-v8.12.0] dry-view git(caching-with-dry-core) be ruby benchmarks/benchmark_controller.rb
Warming up --------------------------------------
   action_controller     1.000  i/100ms
            dry-view     1.000  i/100ms
Calculating -------------------------------------
   action_controller      7.135  (± 0.0%) i/s -     36.000  in   5.054691s
            dry-view      6.115  (± 0.0%) i/s -     31.000  in   5.077587s

Comparison:
   action_controller:        7.1 i/s
            dry-view:        6.1 i/s - 1.17x  slower

As you can see, the difference is meaningless, this probably means dry-view can easily become faster on future MRIs.

@timriley timriley merged commit de08edd into master Dec 21, 2018
@timriley timriley deleted the caching-with-dry-core branch December 21, 2018 12:18
@solnic
Copy link
Member

solnic commented Dec 21, 2018

It's worth recognising that we're certainly doing a lot more in the dry-view side of this benchmark versus action view, at least in how we're preparing the scope for the template, e.g. checking for exposure dependencies, wrapping up values in parts, finding classes, etc., so this isn't truly an apples-for-apples comparison. Perhaps a better one would be e.g. using Draper to decorate every ivar in the Rails controller before it's passed to the template.

That is correct. OTOH it is a legit scenario, and a very common one, so it's probably worth having such a basic benchmark. To balance this better adding a more advanced scenario would be great too!

@GustavoCaso GustavoCaso mentioned this pull request Dec 22, 2018
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.

3 participants