Remove method_missing usage in Idv::Session#11857
Merged
zachmargolis merged 2 commits intomainfrom Feb 7, 2025
Merged
Conversation
- 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
matthinz
approved these changes
Feb 7, 2025
zachmargolis
commented
Feb 7, 2025
| it 'allows supported attributes' do | ||
| it 'allows using supported setters and getters' do | ||
| Idv::Session::VALID_SESSION_ATTRIBUTES.each do |attr| | ||
| subject.send attr, 'foo' |
Contributor
Author
There was a problem hiding this comment.
So before calling idv_session.address_edited('foo') would act as a setter.
That is very uncommon! I made a call to remove that, now it will throw an argument error because now reader and writer are two separate methods.
Open to changing this back if we are worried, but I expect our test suite would catch any lingering use of that style
Contributor
There was a problem hiding this comment.
I noticed this too, glad to see a fix
Contributor
Author
There was a problem hiding this comment.
fixed two sneaky-setter usages in f2426a2
Contributor
mitchellhenke
left a comment
mitchellhenke
left a comment
There was a problem hiding this comment.
This is really incredible, glad to see some much simpler refactoring of the less than common patterns used previously!
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
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?
NB, I have not performance tested this myself to confirm the hypothesis, I am open to checking that before merging. The unit test coverage is solid so this should still work