Separate setting definitions from config values #138

timriley · 2022-08-27T06:48:40Z

Overview

In this PR we separate setting definitions (i.e. the objects you create when you call setting :my_setting) from config, which now maintains its values in a separate hash, and stores only a reference to the setting definitions.

This allows settings objects to be shared across all config classes, which, along with some other changes here, reduces memory usage considerably.

According benchmarks/memory_profile.rb for 10,000 subclasses, this branch now allocates only 9.5 MB, down from 21.9 MB on the main branch.

User-facing changes

Unless you're an advanced dry-configurable user here, and reaching deeply into our Setting objects, the user-facing changes here are minimal! Ordinary usages of dry-configurable will continue to work as before.

The one real behavioural change is that for instance-level configurable, new settings defined make themselves available even on previously initialized objects. This is in fact an incidental change to the rest of this PR and I could choose to wind that back, too, if we wanted.

Implementation details

Setting moves to being a strict definition only. It no longer plays double duty as a realisation of an associated value within a config object. Specifically, it no longer has an input or value. Because of this, it no longer needs to be finalized or return a pristine copy or other such config-related responsibilities.
Setting::Nested and Setting#nested have been removed. These no longer seemed necessary, and we now just provide a simple array of children: when initialising the setting.
In turn Settings can lose config-related responsibilities such as #finalize! and #pristine.
I renamed the internal Settings collection from elements to settings since it feels nicer.
At the same time, this internal collection is no longer a Concurrent::Map, instead just a regular hash. This netted us some decent memory savings. I am keen to hear feedback about this. Why did it need to be a concurrent map in the first place? Are we worried e.g. about settings being defined in concurrent threads? What material downside would there be from this being a hash? And how does this compare with how we do things in e.g. Dry::Core::ClassAttributes, where there's no specific thread safety measures. Is calling setting here really all that different to defines from ClassAttributes?
Config is now initialized with a settings instance, which it uses to determine which values it should be storing. These values are kept internally in a simple name=>value hash.
When configurable classes are inherited, we create a dup of the _settings collection to the subclass. This dups its internal settings array only, so we don't copy all of the settings themselves, thereby preserving memory.
Likewise, we create a copy of the config into the subclass using a new Config#dup_for_settings method, passing the new subclass settings collection to ensure the config object sees any new settings defined in the subclass. When we #dup_for_settings, we also dup any cloneable values that the config holds (i.e. values likely to be mutated and therefore ideally not shared), but we do not dup any of the other simple values, since they are safer to be shared.

Next steps

In the shortest term, I'm going to let @solnic merge #136 when he is ready, and then I'll make sure I update this PR to work with those changes. I expect it should only need minimal adjustment, mostly just moving of code.

I also expect to follow this up in the next week with another proposal for further reducing dry-configurable memory usage, but this PR is already a good step in a positive direction, and I think it also helpfully clarifies the code at the same time.

timriley · 2022-09-13T13:35:21Z

lib/dry/configurable/class_methods.rb

+        # Only classes **extending** Dry::Configurable have class-level config. When
+        # Dry::Configurable is **included**, the class-level config method is undefined because it
+        # resides at the instance-level instead (see `Configurable.included`).


I left this note here because I remember being confused in the past about why we may not respond to config despite it being defined right here in this module. Hopefully this helps alleviate any confusion from readers in the future.

timriley · 2022-09-13T13:35:47Z

lib/dry/configurable/class_methods.rb

-        @settings ||= Set[*_settings.map(&:name)]
+        Set[*_settings.map(&:name)]


We don't want to memoize this because it'll capture stale data if called before all settings are defined.

@timriley I would remove this, we have _settings and this one should be exposed as settings IMO. This Set is mostly here for historical reasons, it was just always like that but I'm not sure if this is a valuable thing to keep.

@solnic Oh great, I'm glad you think this! The settings/_settings disparity has always bothered me (especially since the more useful of the two objects is the one with the ugly underscore prefix).

I'll be happy to update it, but I'll do it in a separate PR (once this is merged) for better visibility.

@timriley the only reason why I introduced _settings was because I didn't want to break backward compatibility by changing settings from a set of symbol names to an array of setting objects.

timriley · 2022-09-13T13:36:50Z

lib/dry/configurable/compiler.rb

-        visit(parent).nested(call(children))
+        name, opts = parent[1]
+
+        Setting.new(name, **opts, children: Settings.new(call(children)))


To save ourselves from creating a throwaway object (i.e. a Setting that we initialize and then have to re-initialize with the children), we build a single Setting instance here directly rather than running its part of the AST back through the compiler.

timriley · 2022-09-13T13:37:44Z

lib/dry/configurable/config.rb

-        if setting.writer?(meth)
-          raise FrozenConfig, "Cannot modify frozen config" if frozen?
-
-          _settings << setting.with(input: args[0])
+        if name.end_with?("=")
+          self[setting_name] = args[0]


Clarified reading and writing of values here by moving all such logic into the #[] and #[]= methods, rather than it being spread between them and #method_missing.

timriley · 2022-09-13T13:47:05Z

Reduce memory usage of Hanami::Action instances

solnic · 2022-09-13T17:57:04Z

custom config support was merged to main but I'm not sure how it's going to work here given that crucial construction logic was moved to the default Config class?

timriley · 2022-09-14T20:49:47Z

custom config support was merged to main but I'm not sure how it's going to work here given that crucial construction logic was moved to the default Config class?

I've rebased over the main branch now (and tidied my commit history) and everything is working fine! (👋🏼 @solnic)

If you'd like to review it, the Config construction logic is in these places:

ClassMethods#__config_build__ (same as your recent PR)
Setting#to_value (changed for this PR, this used to be in the now-removed Setting::Nested class)

Overall, this PR is ready for review and merge once we're all good with it.

timriley · 2022-09-14T20:52:17Z

lib/dry/configurable/class_methods.rb

+        # Dry::Configurable is **included**, the class-level config method is undefined because it
+        # resides at the instance-level instead (see `Configurable.included`).
+        if respond_to?(:config)
+          subclass.instance_variable_set(:@config, config.dup_for_settings(new_settings))


Interestingly, until now we had been setting :@_config, when the actual memoized instance variable (used further below in the #config method) is @config. Setting this to the memoized ivar is in fact the desired behaviour here, so I think before now things must have been working accidentally.

@timriley it'd be good to decide on just one approach for naming private ivars, in hanami I noticed we have _foo, in dry-rb we have __foo__. I'm just mentioning it here for future consideration.

timriley · 2022-09-14T20:55:10Z

lib/dry/configurable/dsl.rb

@@ -8,16 +8,12 @@ module Configurable
    class DSL
      VALID_NAME = /\A[a-z_]\w*\z/i.freeze

-      # @api private


This whole class is marked as # @api private so these lines aren't needed.

This reduces memory usage considerably, and improves separation of concerns.

This saves us a reasonable amount of memory.

Now that settings and config are separated, we don't need to dup the settings anymore. This allows the config to use settings defined even after instances of the configurable class are made.

solnic

Looking good overall, I left some suggestions that may improve performance a bit.

solnic · 2022-09-15T11:16:02Z

lib/dry/configurable/class_methods.rb

+        # Dry::Configurable is **included**, the class-level config method is undefined because it
+        # resides at the instance-level instead (see `Configurable.included`).
+        if respond_to?(:config)
+          subclass.instance_variable_set(:@config, config.dup_for_settings(new_settings))


@timriley it'd be good to decide on just one approach for naming private ivars, in hanami I noticed we have _foo, in dry-rb we have __foo__. I'm just mentioning it here for future consideration.

solnic · 2022-09-15T11:16:50Z

lib/dry/configurable/class_methods.rb

-        @settings ||= Set[*_settings.map(&:name)]
+        Set[*_settings.map(&:name)]


@timriley the only reason why I introduced _settings was because I didn't want to break backward compatibility by changing settings from a set of symbol names to an array of setting objects.

lib/dry/configurable/config.rb

lib/dry/configurable/setting.rb

lib/dry/configurable/settings.rb

timriley · 2022-09-15T20:18:15Z

Thanks @solnic! I'll incorporate all of your suggestions :)

And just to make sure, are you and @flash-gordon both fine with the replacement of the Concurrent::Map with an ordinary Hash for containing the setting definitions (inside Settings)?

solnic · 2022-09-16T08:37:23Z

And just to make sure, are you and @flash-gordon both fine with the replacement of the Concurrent::Map with an ordinary Hash for containing the setting definitions (inside Settings)?

@timriley yes this is fine. If thread-safety is an issue here, some external mechanism should be incorporated to ensure the safety. Doing so at each library level turned out to be a bad idea, as we've learned 🙂

flash-gordon · 2022-09-19T08:21:26Z

And just to make sure, are you and @flash-gordon both fine with the replacement of the Concurrent::Map with an ordinary Hash for containing the setting definitions (inside Settings)?

I don't think this can lead to problems, practically speaking. Your reasoning is the same as mine, why would you have an environment where you define settings concurrently? AFAIK, require is thread-safe. There are possible other use cases but I agree with @solnic that they should be synchronized externally.

timriley · 2022-09-19T20:02:07Z

Thank you all! Merging this one now.

@name

… config_class (#392) Take advantage of the recent memory usage improvements in dry-configurable (see dry-rb/dry-configurable#138) by converting the previously custom `Configuration` class into settings defined on the `Hanam::Action` class. To preserve some of the existing custom configuration methods, provide a custom `Hanami::Action::Config` config class to `Dry::Configurable` and keep those methods there. This custom config class also comes in handy as the place to keep the documentation for our settings, which I've converted into yard `@!attribute` macros for concision. We also move two existing bits of state into new dry-configurable settings: - Convert `before_callbacks` and `after_callbacks` from class attributes, since Hanami-utils' class attributes implementation is very inefficient with memory. - Move `accepted_formats` from being an class-level ivar, since this was the only remaining class-level state and it made sense to keep all this together via the class-level config object. As a consequence of the last change, we're also able to change `Hanami::Action` so it has a conventional `#initialize` method again, rather than having a custom `.new` method that has to `allocate` the object and assign some ivars directly, which is a big win for ease of understanding the code :) Lastly, while we're here, we get rid of the @name ivar for actions, since it's no longer used anywhere. Using the newly added `benchmarks/memory_profile_action.rb` (which creates 100 `Action` subclasses by default) we can see this branch allocates 0.338 MB of memory, versus 1.859 MB for the main branch.

ojab

TBH no idea how this supposed to be used after reading the specs.

ojab · 2022-10-19T16:06:07Z

spec/unit/dry/configurable/dsl_spec.rb

  end

  it "compiles but deprecates giving a default as positional argument", :collect_deprecations do
    setting = dsl.setting :user, "root"

    expect(setting.name).to be(:user)
-    expect(setting.value).to eql("root")
+    expect(setting.default).to eq("root")
+    # expect(setting.value).to eql("root")


Should be removed or uncommented (and below)?

timriley · 2022-10-19T19:36:51Z

TBH no idea how this supposed to be used after reading the specs.

@ojab What in particular are you looking to find out?

That the specs didn't change much indicates that overall usage remains the same, especially if you're just using setting and config.

The biggest difference is that _settings now returns an array of Dry::Configurable::Setting objects that contain the details of the setting definition only; the details about the configured values of the settings are kept in Dry::Configurable::Config now, separated from the Setting instances.

timriley · 2022-10-19T19:38:41Z

@ojab Ah, if your comment was meant specifically about whether to leave or remove these lines from spec/unit/configurable/dsl_spec.rb:

# expect(setting.value).to eql("root")

Then the answer is that they should be removed :) Setting instances no longer hold values.

Looks like I cleaned them up in 22ab1fa, anyway.

ojab · 2022-10-22T13:19:49Z

Thanks, I was trying to understand if there should be any user-visible changes, now I see that it's the internal change.

This comment was marked as outdated.

Sign in to view

timriley mentioned this pull request Sep 13, 2022

Support for custom config classes #136

Merged

timriley requested a review from flash-gordon September 13, 2022 13:32

timriley marked this pull request as ready for review September 13, 2022 13:32

timriley requested a review from solnic as a code owner September 13, 2022 13:32

timriley force-pushed the separate-setting-definitions branch from c2e9e9f to f6107f8 Compare September 13, 2022 13:35

timriley commented Sep 13, 2022

View reviewed changes

timriley force-pushed the separate-setting-definitions branch 4 times, most recently from 8e514e4 to 79cfb72 Compare September 14, 2022 20:44

timriley commented Sep 14, 2022

View reviewed changes

timriley added 3 commits September 15, 2022 06:55

Separate setting definitions from config values

87cbfc3

This reduces memory usage considerably, and improves separation of concerns.

Use simple hash instead of Concurrent::Map

bc32258

This saves us a reasonable amount of memory.

Use original settings object for included config

10d4af7

Now that settings and config are separated, we don't need to dup the settings anymore. This allows the config to use settings defined even after instances of the configurable class are made.

timriley force-pushed the separate-setting-definitions branch from 79cfb72 to 10d4af7 Compare September 14, 2022 20:55

timriley requested a review from jodosha September 15, 2022 05:37

solnic approved these changes Sep 15, 2022

View reviewed changes

jodosha approved these changes Sep 16, 2022

View reviewed changes

Apply fedback from solnic

554d19f

timriley merged commit 35f34b2 into main Sep 19, 2022

timriley deleted the separate-setting-definitions branch September 19, 2022 20:02

This was referenced Oct 2, 2022

Reduce memory usage in Hanami::Action via dry-configurable and custom config_class hanami/controller#392

Merged

Add Config#configured? #143

Merged

ojab reviewed Oct 19, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Separate setting definitions from config values #138

Separate setting definitions from config values #138

timriley commented Aug 27, 2022 •

edited

Loading

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

timriley Sep 13, 2022

timriley Sep 13, 2022

solnic Sep 13, 2022

timriley Sep 14, 2022 •

edited

Loading

solnic Sep 15, 2022

timriley Sep 13, 2022 •

edited

Loading

timriley Sep 13, 2022

timriley commented Sep 13, 2022

solnic commented Sep 13, 2022

timriley commented Sep 14, 2022

timriley Sep 14, 2022 •

edited

Loading

solnic Sep 15, 2022

timriley Sep 14, 2022

solnic left a comment

solnic Sep 15, 2022

solnic Sep 15, 2022

timriley commented Sep 15, 2022

solnic commented Sep 16, 2022

flash-gordon commented Sep 19, 2022

timriley commented Sep 19, 2022

ojab left a comment

ojab Oct 19, 2022

timriley commented Oct 19, 2022

timriley commented Oct 19, 2022 •

edited

Loading

ojab commented Oct 22, 2022

		@settings \|\|= Set[*_settings.map(&:name)]
		Set[*_settings.map(&:name)]

Separate setting definitions from config values #138

Separate setting definitions from config values #138

Conversation

timriley commented Aug 27, 2022 • edited Loading

Overview

User-facing changes

Implementation details

Next steps

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timriley Sep 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timriley Sep 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timriley commented Sep 13, 2022

solnic commented Sep 13, 2022

timriley commented Sep 14, 2022

timriley Sep 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

solnic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timriley commented Sep 15, 2022

solnic commented Sep 16, 2022

flash-gordon commented Sep 19, 2022

timriley commented Sep 19, 2022

ojab left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timriley commented Oct 19, 2022

timriley commented Oct 19, 2022 • edited Loading

ojab commented Oct 22, 2022

timriley commented Aug 27, 2022 •

edited

Loading

timriley Sep 14, 2022 •

edited

Loading

timriley Sep 13, 2022 •

edited

Loading

timriley Sep 14, 2022 •

edited

Loading

timriley commented Oct 19, 2022 •

edited

Loading