Reduce memory usage in Hanami::Action via dry-configurable and custom config_class #392
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
timriley
force-pushed
the
use-class-level-settings
branch
4 times, most recently
from
8d5e47e
to
78dd1e4
Compare
timriley
changed the title
…ize, avoid unneeded allocations for accepted_mime_types
timriley
force-pushed
the
use-class-level-settings
branch
from
78dd1e4
to
f8f7de2
Compare
|
FYI, it seems we had a test failure in a previous run of this (https://github.com/hanami/controller/actions/runs/3166800527/jobs/5156785405) — I'll look into this and address it before merging. |
solnic
approved these changes
Oct 2, 2022
|
@solnic It's so good! Couldn't have done it without you :) |
FYI, I looked at this test and couldn't replicate locally (same Ruby version, RSpec command and seed), plus it doesn't seem related to my changes here, so I'm not following it up any further at this point. |
🥹 |
timriley
added a commit
to hanami/hanami
that referenced
this pull request
Oct 7, 2022
Use the config now accessible directly on the `Hanami::Action` class as `Hanami::Action.config` (changed in hanami/controller#392). Also update usage of dry-configurable with `Undefined`: with the new `default_undefined` extension option, `Undefined` takes a special meaning inside dry-configurable. Avoid passing it in our case to make sure our existing behavior is preserved.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR takes advantage of the recent memory usage improvements in dry-configurable (see dry-rb/dry-configurable#138) by converting the previously custom
Configurationclass into settings defined on theHanam::Actionclass.To preserve some of the existing custom configuration methods, we provide a custom config class to
Dry::Configurableand 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
@!attributemacros for concision.We also move two existing bits of state into new dry-configurable settings:
before_callbacksandafter_callbacksfrom class attributes, since Hanami-utils' class attributes implementation is very inefficient with memory.accepted_formatsfrom 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::Actionso it has a conventional#initializemethod again, rather than having a custom.newmethod that has toallocatethe 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 100Actionsubclasses by default) we can see this branch allocates 0.338 MB of memory, versus 1.859 MB for the main branch.Even putting aside the memory improvements, I'd paint this as a useful refactor, because it reduces code, better leverages a library we're already using (dry-configurable), as well as centralises all the action class-level state, making it easier for us to manage.
🙋🏼 I've tidied up the commit history here so you should be able to review things commit by commit.