Remove frequently unnecessary hash allocation#11854
Conversation
changelog: Internal, Performance, Remove fetch to avoid unnecessary hash allocation
mitchellhenke
deleted the
mitchellhenke/remove-frequently-unneccessary-hash-allocation
branch
|
|
||
| def session | ||
| user_session.fetch(:idv, {}) | ||
| user_session[:idv] || {} |
There was a problem hiding this comment.
As a further micro-optimization, I wonder if it could be useful to have a frozen empty hash constant reference somewhere we could use to further avoid allocations in these cases?
Like a...
EMPTY_HASH = {}.freezePoking around the Rails source, I did find a couple references to constants used for this purpose, though I dunno that we'd want to use them directly:
ActiveRecord::QueryMethods::FROZEN_EMPTY_HASHActionController::Parameters::EMPTY_HASH
There was a problem hiding this comment.
Does the default get instantiated and persisted in the session? Like would we want something like this to preserve behavior?
| user_session[:idv] || {} | |
| user_session[:idv] ||= {} |
There was a problem hiding this comment.
As a further micro-optimization, I wonder if it could be useful to have a frozen empty hash constant reference somewhere we could use to further avoid allocations in these cases?
I assumed it was likely that the hash is mutated, so I'm less sure if a frozen constant would be possible here.
Does the default get instantiated and persisted in the session? Like would we want something like this to preserve behavior?
Not in the current implementation, the result of this is used in instantiation here. I think it's in part due to the class being kind of unique in that it uses method_missing to allow for calling idv_session.applicant = 'abc' to set values in the hash. I'd like to refactor that since the mutation of the method here also seems to be a bit of a hotspot, but that's a bigger effort.
- Per comment on #11854, the dynamic access and string manipulation makes these methods a hotspot - Dynamically defining these methods one should hopefully let the JIT optimize this a little better, and lets us use the default implementations for #method_missing and #respond_to_missing? changelog: Internal, Source code, Optimize dynamic method access
* Remove method_missing usage in Idv::Session - Per comment on #11854, the dynamic access and string manipulation makes these methods a hotspot - Dynamically defining these methods one should hopefully let the JIT optimize this a little better, and lets us use the default implementations for #method_missing and #respond_to_missing? * Use explicit setter methods in specs changelog: Internal, Source code, Optimize dynamic method access
4 participants
🛠 Summary of changes
Sort of a continuation of #11841 and #11844, this PR removes a frequently unnecessary hash allocation.
#fetch(key, default)will always instantiate the default even if the key exists. The behavior is not identical in that anilvalue in the hash will be returned if it is there and this will fall through and return the empty hash. We should never be storingnilhere as we use delete when clearing the session value.We could alternatively use the block form
#fetch(key) { |key| ... }as a "lazy" instantiation of the hash, but it didn't feel necessary in this case.The pattern of a default empty hash value appears in other places in the codebase, but
sessionis called quite frequently and seems to contribute much more significantly.