Possible early filtering feature to support higher novel-packet bandwidth

[timothyfroehlich]

Overview & Major Changes

This is a possible change to increase the number of novel broadcasts that coretools can pass through. The idea is to filter out duplicate packets at the earliest possible moment, before any processing time is spent on it. I went through the code and identified everything that should identify a bled packet as a broadcast packet and wrote a hardcoded check for whether the packet that was just read from the bled112 dongle is a broadcast. If so, and if the read queue has 25 packets in it already (chosen somewhat arbitarily) it'll check to see if the packet matches the last broadcast from that mac address. If so then it drops it.

I'm running some tests on it now but so far I haven't seen anything that shows it negatively impacting performance and it potentially doubles the number of packets downstream systems see, when the average pod updates it's data no more than once a second (packets include the Pod's timestamp, which increments once a second, so at least one packet a second will get through per Pod.

I haven't cleaned it up yet or added a config variable to enable / disable it yet, but I'd like an early look at it to see if anyone has any problems with this approach.

Edit: The code in this draft PR was added to show an example of what this change would look like, to make it more clear what this change would entail.

[mattrunchey]

(I edited my earlier comment -a lot- so I'm re-pasting it to make it more clear for those who are only reading email notifications)

(read your note after commenting earlier, sorry)

This deduplication needs to be locked behind a configurable flag. We don't want to always deduplicate. I don't know how you're adding that because it's not currently in the code. (I know you call this out in the note, but that's the main thing I want to re-emphasize that the design of that is going to be critical here).

General note / my opinion on reviews like this: It's easier to review your design and approach in an issue laying out what changes you're intending to make, rather than to review partial code that's in an in-progress state, and then reviewing again when it's complete. I will try but it's challenging because we don't know what the final product is going to look like, and it's more likely that we'll request changes that require more rework rather than to get the kinks ironed out at the design phase. In general I think it's tough enough to jockey for time from others for reviews as it is, so minimizing a reviewer's burden will get you more effective reviews overall.

If you're at the point where you're writing code, I'm concerned if we're evaluating code for the right approach. It's more effective to agree on the approach before code gets written, and then in the review we're evaluating mostly for small bugs, and we're all on the same page with the bigger swathes of things. But this is just my opinion on things in general. This PR looks small enough that it's not a big deal, so this is more of a note pointed at bigger changes.

[timothyfroehlich]

Reading your note.

This deduplication needs to be locked behind a configurable flag. We don't want to always deduplicate. I don't know if you're adding that because it's not currently in the code. (I know you call this out in the note, but that's the main thing I want to re-emphasize that the design of that is going to be critical here).

This was mentioned in the initial comment.

General note / opinion on reviews like this: It's easier to review your design and approach in an issue laying out what changes you're intending to make, rather than to review partial code that's in an in-progress state. I can try but it's challenging because we don't know what the final product is going to look like, and it's more likely that we'll request changes that require more rework rather than to get the kinks ironed out at the design phase.

This was described in the first paragraph of the initial comment. To add to it, this was something that was relatively easy for me to add for quick testing, and I was unsure if it would even be useful at all. I felt it was helpful to include the current test code to help with discussion, vs having to explain it in text that could be misunderstood.

If you're at the point where you're writing code, I'm concerned if we're evaluating code for the right approach. It's more effective to agree on the approach before code gets written, and then in the review we're evaluating mostly for small bugs, and we're all on the same page with the bigger swathes of things. But this is just my opinion on things in general. This PR looks small enough that it's not a big deal, so this is more of a note pointed at bigger changes.

This is a draft review, not a final one and is a useful tool for showing code differences. I've only modified one file and added about fifteen lines of functional code.

This was an approach that I was unsure would even be worth having a discussion about, because I was unsure if the changes I had in mind would make a measurable improvement or even hurt performance. This was experimentation and I'm asking for discussion about the approch.

[mattrunchey]

I see one thing in the design that maybe could be simplified without changing what the goal of this is:

If so, and if the read queue has 25 packets in it already (chosen somewhat arbitarily) it'll check to see if the packet matches the last broadcast from that mac address. If so then it drops it.

If we are deduplicating in the first place, do we want to permit building up a backlog of 25 packets before checking? It might be useful to compare this approach to doing an all-or-none deduplication check. I am gathering that the reason for this is to not incur performance hits unless we're queued up with more than 25 packets, but if the overall change here has low performance impact anyways, would the cost of always checking be noticeable? Removing this check reduces code complexity and the outcome could end up the same for all intents and purposes. Given that this check is primarily designed for high traffic environments, it feels like the application a-priori is self identifying that it anticipates a queue backlog anyways.

Additionally, it looks like the system would be a touch confusing may result in downstream ambiguity. It's only checking for duplicates if there's a backlog, so sometimes we'll get duplicates (in low traffic) and other times we wont. This creates some ambiguity for downstream systems that can be problematic - coretools as a plug-and-play application for end users would set this flag like --purge-duplicates would indicate that you never get duplicates, vs. having to explain a more ambiguous flag like --purge-duplicates-if-overloaded and might require application developers to consider the traffic patterns they'd observe (that they otherwise wouldn't have to consider). Something to think about.