-
Notifications
You must be signed in to change notification settings - Fork 532
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
resource identity comparable #1430
Conversation
add tests for ResourceIdentity, including that comparison does not allocate memory
@@ -41,7 +43,16 @@ def hash | |||
end | |||
|
|||
def <=>(other_identity) | |||
self.id <=> other_identity.id | |||
return nil unless other_identity.is_a?(ResourceIdentity) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comparsble includes some of the above methods. Is the override here intended?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the idea is we have a composite key and need to ensure we sort consistently even when given a polymorphic list of ResourceIdentities. This logic places grouping by type as a higher priority than by id. I chose that logic since it will seem more natural when ids are either UUIDs or integers.
Comparable uses <=> to implement the conventional comparison operators (<, <=, ==, >=, and >) and the method between?.
I could remove the override of ==
as well, which might be more performant than the hash
call. I'm going to test this and see what makes the most sense. I wish I had focused on including Comparable earlier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My gut was wrong and it like I should keep the override of ==
that uses hash
as it's about twice as fast in the most common case where the resource klasses are the same:
>> require 'benchmark'
=> false
>> # Using hash method
>> rid1 = JSONAPI::ResourceIdentity.new(Image, 13); rid2 = JSONAPI::ResourceIdentity.new(Image, 13); puts Benchmark.measure { 1_000_000.times { rid1 == rid2 } }
0.194055 0.000052 0.194107 ( 0.195783)
>> # Using new <=> logic prioritizing resource_klass - Case where they are equal
>> rid1 = JSONAPI::ResourceIdentity.new(Image, 13); rid2 = JSONAPI::ResourceIdentity.new(Image, 13); puts Benchmark.measure { 1_000_000.times { rid1 <=> rid2 } }
0.398467 0.000156 0.398623 ( 0.401471)
>> # Using new <=> logic prioritizing resource_klass - Case where resource klasses differ, which should only do one comparison (best case)
>> rid1 = JSONAPI::ResourceIdentity.new(Image, 13); rid2 = JSONAPI::ResourceIdentity.new(User, 13); puts Benchmark.measure { 1_000_000.times { rid1 <=> rid2 } }
0.330682 0.000103 0.330785 ( 0.332534)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also tested removing the check on other_identity.is_a?(ResourceIdentity)
and it only saved a small amount of time:
>> rid1 = JSONAPI::ResourceIdentity.new(Image, 13); rid2 = JSONAPI::ResourceIdentity.new(Image, 13); puts Benchmark.measure { 1_000_000.times { rid1 <=> rid2 } }
0.379685 0.000151 0.379836 ( 0.383279)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job testing!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good
add tests for ResourceIdentity, including that comparison does not allocate memory
Fixes #1429
add tests for ResourceIdentity, including that comparison does not allocate memory
All Submissions:
New Feature Submissions:
Bug fixes and Changes to Core Features:
Test Plan:
Reviewer Checklist: