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

Restore Config value read performance #149

Merged
merged 4 commits into from
Oct 12, 2022
Merged

Conversation

timriley
Copy link
Member

@timriley timriley commented Oct 11, 2022

Restore the performance of Config on value reads back to its state before #143.

Here we capture all values on read to Config's internal _values hash. This means that after first read, all subsequent reads should be fast (This is how it was before #143).

Then to preserve the functionality of #configured? that we introduced in #143, we keep a separate _configured Set that tracks the names of values that have either been assigned, or mutable values that have been read. For the very small amount of memory (one set per config object, since most of the names, as symbols, should already be in memory) this is a worthwhile tradeoff for the best possible performance on read, which should reasonably be expected by our users to be as fast as possible.

Copy link
Member

@solnic solnic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm missing something because it's hard for me to understand why this was needed. Given that we already have _values why we suddenly also need _configured? What's the difference between the two?

@timriley
Copy link
Member Author

timriley commented Oct 12, 2022

@_values is the hash where the config values are kept, regardless of where they came from (i.e. via a setting default or explicit assignment). We keep them all in this hash to ensure that reads remain quick. This is how it was in previous releases of the library.

In #143 I changed how we populated _values so that it could be used to determine whether a user had explicitly assigned a value. But this had some negative performance ramifications. This change was NOT a part of the memory optimisation effort; it was me adding what I believed to be a missing feature, based on places we'd been using dry-configurable.

To restore config read performance, in this PR I'm letting _values be populated again for all values on read. This means we now need a separate place to track whether a user has explicitly assigned a config value, hence _configured.

The reason we need a separate place to track this is because a user may explicitly assign a value that happens to end up equalling the setting's default value. In that case, we do want to return true from #configured?, to make sure we reflect their actual action, so we can't just e.g. do an equality check against the setting default for all values (we have to do it for mutable values only); instead we have to track the keys in _configured.


To put it another way: #configured? could not be implemented with the original way we populated _values. To implement it initially, we changed how we populated values, but that had negative performance consequences. So now we're back to the original way, and now use a small additional structure to preserve #configured?'s behaviour.

@timriley timriley force-pushed the improve-config-to_h-performance branch from 0e30b11 to 4853d71 Compare October 12, 2022 08:13
@timriley timriley requested a review from flash-gordon October 12, 2022 08:15
@timriley
Copy link
Member Author

@solnic @flash-gordon Folks, this is ready to go. Since it is a fix to a performance regression I'm incline to get it in sooner rather than later and ship a patch release. Please let me know if you have any concerns :)

Should be no real difference in performance here, but this feels more direct
This allows us to capture all read values into _values, which makes for much faster regular value reads.
This improves performance, and returning a new object isn't critical here
@solnic solnic self-requested a review October 12, 2022 12:28
Copy link
Member

@solnic solnic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation ❤️ 🚀

@timriley timriley merged commit 8c3abfb into main Oct 12, 2022
@timriley timriley deleted the improve-config-to_h-performance branch October 12, 2022 19:38
timriley added a commit that referenced this pull request Oct 12, 2022
Return to progressively populating the `_values` hash for _all_ values, regardless of cloneable status. This makes config reads as quick as they were before the changes in #143.
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