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

Memoize Config#hash on finalize #151

Merged
merged 1 commit into from
Oct 12, 2022
Merged

Conversation

timriley
Copy link
Member

@timriley timriley commented Oct 12, 2022

Freezing the hash is beneficial at this point because it saves repeated expensive computation if that hash is to be used later in performance-sensitive situations, such as when serving as a cache key or similar.

The reason we do this here is because Dry::Equalizer's hash implementation is always dynamic, i.e. it will recompute the hash every time from the config's full values. Dry::Equalizer does provide an immutable: true option to memoize the hash, but we can't use that for Config because it is intended to be mutable for much of its lifecycle.

However, there is one point in Config's lifecycle at which we know its values should no longer change, which is #finalize!. By using this as an opportunity to memoize the hash, it will allow users that rely on this hash to trust that it can be used in more performance sensitive situations, such as using it as a cache key.

In terms of testing this, I've brought in rspec-benchmarks and added a test to verify that #hash on a finalized config is significantly faster than on a non-finalized config:

    it "is memoized when the config is finalized", :performance do
      klass.setting :a
      klass.setting :b
      klass.setting :c
      klass.setting :d
      klass.setting :e

      finalized_config = klass.config.dup.finalize!

      expect(finalized_config.hash).to eq klass.config.hash
      expect { finalized_config.hash }.to perform_faster_than { klass.config.hash }.at_least(50).times
    end

This test does take around 300ms to run, but it is the best way I could think of to exercise this behaviour (e.g. it's much more meaningful IMO than simply asserting that a @__hash__ ivar exists).


Sidenote: config being used as a hash key is what actually happens in dry-view/hanami-view, and this PR is an effort to make dry-configurable more useful in those situations. See dry-rb/dry-view#156 as an effort to work around the issue directly inside dry-view. My preferred approach to solve this for dry-view would be to bring the hash memoizing into dry-configurable, where it can be more appropriately managed alongside the rest of the Config logic, and then in dry-view, automatically finalise the config the first time a view instance is initialised.

Freezing the hash is beneficial at this point because it saves repeated expensive
computation if that hash is to be used later in performance-sensitive situations, such as
when serving as a cache key or similar.
@timriley timriley requested a review from solnic as a code owner October 12, 2022 09:46
@timriley timriley requested a review from flash-gordon October 12, 2022 09:46
@flash-gordon
Copy link
Member

In the place where you depend on Config#hash shouldn't you use Config#object_id instead? Otherwise, it seems like you can generate some garbage (presuming you use it for cache or something) until finalization happens. Other than this, 👍

@timriley
Copy link
Member Author

@flash-gordon Thanks for checking this out!

In the place where you depend on Config#hash shouldn't you use Config#object_id instead? Otherwise, it seems like you can generate some garbage (presuming you use it for cache or something) until finalization happens.

Yeah, this is worth considering, though in actual usage I wouldn't expect any caching to occur until after all changes to config are already completed and config is finalised. Anyway, I'll experiment more with some of that over in dry-/hanami-view in a couple months once we're done with this initial hanami 2.0 release.

@timriley timriley merged commit 592fb1c into main Oct 12, 2022
@timriley timriley deleted the memoize-config-hash-on-finalize branch October 12, 2022 20:04
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