Support nested prefixes in the LcTrie class#2614
Support nested prefixes in the LcTrie class#2614alyssawilk merged 5 commits intoenvoyproxy:masterfrom brian-pane:nested-prefix
Conversation
Signed-off-by: Brian Pane <bpane@pinterest.com>
|
Please merge master to pick up #2613, this should fix CI TSAN. |
Signed-off-by: Brian Pane <bpane@pinterest.com>
ccaraman
left a comment
ccaraman
left a comment
There was a problem hiding this comment.
Thank you for implementing this! Looks good, just a few comments/questions.
| // prefix, the lookup will scan the nested prefixes to see if any of them match, | ||
| // too. This situation is rare, so to save memory in the common case the | ||
| // nested_prefixes field is a pointer to a vector rather than an inline vector. | ||
| std::shared_ptr<std::vector<IpPrefix>> nested_prefixes_; |
There was a problem hiding this comment.
is this meant to be a unique_ptr? addNestedPrefix is calling make_unique
There was a problem hiding this comment.
Thanks for catching that. It started out as unique_ptr but then I found that the existing code assumed copyability of the containing struct. So I changed to shared_ptr, but I forgot to change addNestedPrefix. I’ll fix that today.
| IpType bitmask = ip_prefixes_[address].ip_ ^ ip_address; | ||
| if (extractBits<IpType, address_size>(0, ip_prefixes_[address].length_, bitmask) == 0) { | ||
| return ip_prefixes_[address].tag_; | ||
| // The prefix table entry ip_prefixes_[address] contains either a single prefix or |
There was a problem hiding this comment.
Can you add a todo assigned to me to look into if having a better way to handle /0 is ideal.
There was a problem hiding this comment.
Ok, added! With this PR, the code is relatively clean, because the /0 special case is encapsulated in the extractBits function, but the drawback is that the extra conditional in that function will make the common case (prefixes of nonzero length) a bit slower. I'll defer to you on whether to optimize for code simplicity or runtime speed.
Signed-off-by: Brian Pane <bpane@pinterest.com>
|
@alyssawilk for senior maintainer review |
alyssawilk
left a comment
alyssawilk
left a comment
There was a problem hiding this comment.
Mind doing a merge now that #2612 has landed, and I'll take a look this afternoon?? I think you'll need to update the tests per the latest push there.
Signed-off-by: Brian Pane <bpane@pinterest.com>
source/common/network/lc_trie.h
Outdated
| @@ -155,15 +133,36 @@ class LcTrie { | |||
| extractBits<IpType, address_size>(0, length_, other.ip_))); | |||
There was a problem hiding this comment.
migrate over to using contains()?
|
|
||
| if (tag_data[i - 1] != tag_data[i]) { | ||
| if (ip_prefixes_[ip_prefixes_.size() - 1].isPrefix(tag_data[i])) { | ||
| ip_prefixes_[ip_prefixes_.size() - 1].addNestedPrefix(tag_data[i]); |
There was a problem hiding this comment.
Ok, question time, if we're trying to build a trie, isn't this actually building a vector of maybe vectors? I'd expect from the class name if we had nested prefixes those could have their own nested prefix rather than being bundled together at depth 1. Is this just infrequent enough we do it the inefficient way and if so, mind commenting it up?
There was a problem hiding this comment.
Yes, it is building a vector of nested prefixes. From talking to @mattklein123, nested prefixes aren't that common and linearly traversing a sorted vector in these cases should be good enough for now. If we do notice there being an issue, we can always swap this to have tries for the nested prefix cases.
There was a problem hiding this comment.
I'll add a TODO comment about how more general-purpose usage will require using sub-tries for the nested prefix cases.
There was a problem hiding this comment.
Another solution, I think, would be to change the construction of the base LC Trie to allow nested prefixes. As far as I can tell, the data structure should be able to handle nested prefixes in a single trie; it's just the trie construction algorithm from the original paper that doesn't support nesting. For now, though, I just added the TODO comment.
… `contains` Signed-off-by: Brian Pane <bpane@pinterest.com>
| // too. This situation is rare, so to save memory in the common case the | ||
| // nested_prefixes field is a pointer to a vector rather than an inline vector. | ||
| // TODO(brian-pane) switch to a trie of nested prefixes, to ensure sublinear-time | ||
| // searching even in situations where there are a lot of nested prefixes. |
There was a problem hiding this comment.
FWIW if we think nested prefixes are rare I'm totally fine just commenting "this is a vector rather than a trie of nested prefixes because it's rare and not worth optimizing for" but a TODO is fine if you think we'll fix eventually :-)
|
Actually I can go ahead and do that. Mergable! :-) |
5 participants
Description:
Support nested prefixes such as
127.0.0.0/24and127.0.0.1/32in the LcTrie class. An address comparison against an LcTrie will now return the union of all matching prefixes' tags.Risk Level: Low
Testing: Updated unit tests included
Docs Changes: N/A
Release Notes: N/A
Partially Fixes: #1001
Signed-off-by: Brian Pane bpane@pinterest.com