-
Notifications
You must be signed in to change notification settings - Fork 4.5k
resolver: Make EndpointMap's Get, Set and Delete operations O(1) #8179
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
Conversation
78c0b88
to
a24bee3
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #8179 +/- ##
==========================================
- Coverage 82.42% 82.00% -0.43%
==========================================
Files 393 410 +17
Lines 39162 40233 +1071
==========================================
+ Hits 32280 32993 +713
- Misses 5571 5878 +307
- Partials 1311 1362 +51
🚀 New features to boost your workflow:
|
en := toEndpointNode(e) | ||
if endpoint := em.find(en); endpoint != nil { | ||
return em.endpoints[endpoint], true | ||
val, found := em.endpoints[encodeEndpoint(e)] |
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.
This is still O(n) in the number of addresses, no?
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.
encodeEndpoint
is O(n) where n
is the number of addresses within a single endpoint. Practically a single endpoint should contain around 2 addresses or so.
em.endpoints[x]
is O(n) where n
is also the number of addresses within a single endpoint. This is independent of the number of endpoints in the map, which can be much larger than n
.
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.
Do you think it makes sense to checkin some/all of the benchmark code that you have for posterity?
resolver/map.go
Outdated
} | ||
return nil, false | ||
return nil, found |
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.
Nit: return nil, false
here to be explicit about the second return value.
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.
Done.
Added the micro benchmark mentioned in the PR description. |
Fixes: #8173
This PR changes only the internal implementation of
EndpointMap
to use a map keyed by an unordered list of addresses. This avoids the need to iterate over the entire map to check the existence of a single element. This brings down the average case time complexity for Get, Set and Delete from O(n) to O(1) per operation, wheren
is the number of endpoints in the map.Benchmarks
Code:
Without any changes
With base64 encoding the address list
With JSON encoding the address list
RELEASE NOTES: