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

Add Config subclass with immutable Equalizer #156

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

HashNotAdam
Copy link

@HashNotAdam HashNotAdam commented Oct 9, 2022

When extending Dry::Configurable, the default is to initialize a Dry::Equalizer without immutability on the hash method.

When Dry::View fetches or stores a template, Dry::Core::Cache uses a hash of the arguments. Since this hashing will occur for every nested template, it can be quite computationally demanding. In the case of fetching or storing templates, the values being hashed shouldn't change and so the result can be memoized.

In my benchmarks, I'm using a highly contrived template that loads 100 nested templates that do nothing more than print out their name as well a couple of random numbers to ensure values aren't being cached so, while it is designed somewhat to replicate a site with many components, it's far from true to life. Saying this, my tests have found this has given me a roughly 75% performance increase:

Calculating -------------------------------------
              Before    100.263  (± 6.0%) i/s -      1.008k in  10.076792s
               After    174.497  (± 3.4%) i/s -      1.751k in  10.047389s

It's tricky to solve this elegantly so I'm very open to discussing alternative approaches.

One thing I considered was if there is support for adjusting these values at runtime. I couldn't think of any legitimate reason to do that but, if such a reason exists, I'm sure we could find a solution

When extending Dry::Configurable, the default is to initialize a
Dry::Equalizer without immutability on the hash method

When Dry::View fetches or stores a template, Dry::Core::Cache uses a hash
of the arguments. Since this hashing will occur for every nested
template, it can be quite computationally demanding

In the case of fetching or storing templates, the values being hashed
shouldn't change and so the result can be memoized
@HashNotAdam HashNotAdam requested a review from solnic as a code owner October 9, 2022 18:29
@timriley
Copy link
Member

Thank you so much for this (and #157), Adam!

Am definitely conscious that dry-view (and soon to be, hanami-view) is in need of some performance tuning, and your work here is a great start!

This is also good timing in that I've been doing work in dry-configurable lately. This has mostly been with a view to improving memory usage as much as possible, but it's a good moment to revisit overall performance and make sure I haven't swung the pendulum too far. (It's also doubly good timing in that dry-configurable will probably hit 1.0 in the next ~6 weeks, so we still have a window open for larger changes if required).

To this end, I'd be interested in exploring whether we can do work inside dry-configurable itself to make some of these changes in dry-view unnecessary. I'd like dry-configurable to be a library someone can pick up without having to worry about performance, even if it is used in more performance-sensitive situations like you point out for dry-view.

So: would you mind sharing the benchmark you used for assessing your changes to dry-view? I'd love to be able to give it a run while looking for dry-configuring and compare the results.

@HashNotAdam
Copy link
Author

No worries, @timriley, I've pushed up this repo

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.

2 participants