Skip to content

Duplex routing unnecessarily clones RoutingNetworkUsage#68

Open
periodically-makes-puns wants to merge 6 commits intomockingbirdnest:masterfrom
periodically-makes-puns:why_clone
Open

Duplex routing unnecessarily clones RoutingNetworkUsage#68
periodically-makes-puns wants to merge 6 commits intomockingbirdnest:masterfrom
periodically-makes-puns:why_clone

Conversation

@periodically-makes-puns
Copy link
Contributor

The core issue is in this line in FindCircuit:

var usage_with_forward_channel = new RoutingNetworkUsage(this, usage);

This clones the entire NetworkUsage object passed to it. We use this to temporarily add the links from the forward route to the usage, which lets us route the backwards route. However, the cost of doing this increases the larger current_network_usage_ becomes, and is essentially quadratic in the number of connections (since we are allocating an entire new Dictionary with all of the contents of the old one every duplex connection we route!).

Therefore, I opt to add "fake" usages onto the network, which exploits the fact that we cache the total power/spectrum usage of each antenna separately from the actual SourcedLink[] usages. To that end, UseLinks and its dependencies now support two additional optional arguments, multiplier and fake. multiplier multiplies the power/spectrum added to the total, which we can invert for undoing a "fake" link. fake will cause the SourcedLink[] usages array to not be added, which prevents having to clean it up later when we restore the original state.

FindCircuit now operates on the passed NetworkUsage directly. It adds "fake" usages corresponding to the forward links (which increase the reported power/spectrum use properly without adding the links), before routing the backward Channel. It receives its answer, then cleans up after itself by adding "fake" usages with multiplier -1 corresponding to the forward links, which undoes their effects.

However, we must take care to support FindCircuitInIsolation as well. If it is passed NetworkUsage.None, then FindCircuit will default to cloning the network usage as normal, so that the forward links can be checked against the backwards links properly.

Also, there is hypothetically a floating-point precision error possible here, since we are not generally guaranteed that x + y - y == x for any given double x, y. Addressing this could be done by adding an additional field to PowerBreakdown and SpectrumBreakdown that tracks the "fake" power/spectrum added directly.

Finally, since we add quite a lot of single links with no opportunity for broadcast, I spun off a Linq-free version of UseLinks that optimises for adding a single link to the NetworkUsage. This makes it so we can skip the awkward iteration and group-by checks when there is only one link to add. This version is in UseLinkNoBroadcast.

@eggrobin
Copy link
Member

Also, there is hypothetically a floating-point precision error possible here, since we are not generally guaranteed that x + y - y == x for any given double x, y. Addressing this could be done by adding an additional field to PowerBreakdown and SpectrumBreakdown that tracks the "fake" power/spectrum added directly.

Please do that, regardless of the floating-point issue it seems much cleaner to maintain these separately than to have to adjust the values as part of cleanup.

@periodically-makes-puns
Copy link
Contributor Author

Done. As a bonus that removes the need for the multiplier argument, so it's gone now.

@periodically-makes-puns
Copy link
Contributor Author

Fixed a very dumb error where UseLinkNoBroadcast would skip Rx spectrum consumption if the Tx station was multiple-tracking, because I pulled that code directly from UseTxPower. Whoops.

Also, reverted the changes to the Linq version of UseLinks, because I don't think it ended up being any faster. I'm still not a fan of calling First() on the enumerables produced by GroupBy, but I don't have concrete numbers to justify if/why that would be an issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants