implement containsRoute check in AbstractRib, tests #590
Conversation
| currentAddressBit = bits.get(nextUnmatchedBit); | ||
| currentRightAddressBit = rightAddressBits.get(nextUnmatchedBit); | ||
| if (currentRightAddressBit != currentAddressBit) { | ||
| break; |
There was a problem hiding this comment.
Trying to see how to simplify this code a bit, if possible.
- It looks to me like the
_rightand_leftbits are identical except for whether you use_rightor_left. Maybe extract a helper, and then this function could be simplified to (basically)
if (currentBit) {
doIt(_right);
} else {
doIt(_left);
}- Can the
breakon this line just bereturn false, and then drop theifafter thefor?
There was a problem hiding this comment.
(p.s. I'm doubtful on 2. ; but see the comment above for why.)
There was a problem hiding this comment.
As mentioned above, I'd prefer to defer. Helpers were avoided due to criticality of this code. IMO any proposed changes that execute more code should be justified with profiling. However early exits are uncontroversial.
| boolean currentAddressBit = false; | ||
| boolean currentRightAddressBit; | ||
| for (nextUnmatchedBit = firstUnmatchedBitIndex + 1; | ||
| nextUnmatchedBit < rightPrefixLength && nextUnmatchedBit < prefixLength; |
There was a problem hiding this comment.
I'm not quite understanding how the case is handled when prefixLength > rightPrefixLength. It looks like it will always return false, because even if everything matches we'll exit the for loop, fail the check at L114, and then return false.
If this is desired, just have that return earlier?
I am likely missing something.
There was a problem hiding this comment.
This code was trivially modified from the mergeRoute code which modifies the underlying DDNF-like structure containing the routes. This code itself was itself somewhat brutishly modified from the original trie-maintaining code before the structure was changed. There is likely quite a bit of room for improvement via careful analysis and tests in both mergeRoute and containsRoute.
I will say that at the time I wanted to avoid further function calls (helpers) because all this code is in a critical section (though that might have been overly paranoid).
Anyway the return false on L117 would in mergeRoute instead be code that performs modification. That modification depends on how far we get along rightAddressBits, regardless of whether prefixLength is larger than rightPrefixLength. However in this case since we don't care how far we get when prefixLength is larger than rightPrefixLength, the code can be simplified.
Due to the large amount of optimization we can do on this class, I'd personally prefer to just file a ticket and defer to a later PR.
No description provided.