From 402fc9d65f72136bb3e70b1a7e3d0f20443524ad Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Fri, 3 Jan 2025 10:10:16 -0800 Subject: [PATCH] control/controlclient: remove optimization that was more convoluted than useful While working on #13390, I ran across this non-idiomatic pointer-to-view and parallel-sorted-map accounting code that was all just to avoid a sort later. But the sort later when building a new netmap.NetworkMap is already a drop in the bucket of CPU compared to how much work & allocs mapSession.netmap and LocalBackend's spamming of the full netmap (potentially tens of thousands of peers, MBs of JSON) out to IPNBus clients for any tiny little change (node changing online status, etc). Removing the parallel sorted slice let everything be simpler to reason about, so this does that. The sort might take a bit more CPU time now in theory, but in practice for any netmap size for which it'd matter, the quadratic netmap IPN bus spam (which we need to fix soon) will overshadow that little sort. Updates #13390 Updates #1909 Change-Id: I3092d7c67dc10b2a0f141496fe0e7e98ccc07712 Signed-off-by: Brad Fitzpatrick --- control/controlclient/map.go | 68 +++++++++++-------------------- control/controlclient/map_test.go | 13 +++--- 2 files changed, 30 insertions(+), 51 deletions(-) diff --git a/control/controlclient/map.go b/control/controlclient/map.go index 787912222..b20a8e170 100644 --- a/control/controlclient/map.go +++ b/control/controlclient/map.go @@ -14,7 +14,6 @@ "runtime" "runtime/debug" "slices" - "sort" "strconv" "sync" "time" @@ -31,6 +30,7 @@ "tailscale.com/util/clientmetric" "tailscale.com/util/mak" "tailscale.com/util/set" + "tailscale.com/util/slicesx" "tailscale.com/wgengine/filter" ) @@ -75,8 +75,7 @@ type mapSession struct { lastPrintMap time.Time lastNode tailcfg.NodeView lastCapSet set.Set[tailcfg.NodeCapability] - peers map[tailcfg.NodeID]*tailcfg.NodeView // pointer to view (oddly). same pointers as sortedPeers. - sortedPeers []*tailcfg.NodeView // same pointers as peers, but sorted by Node.ID + peers map[tailcfg.NodeID]tailcfg.NodeView lastDNSConfig *tailcfg.DNSConfig lastDERPMap *tailcfg.DERPMap lastUserProfile map[tailcfg.UserID]tailcfg.UserProfile @@ -366,16 +365,11 @@ func (ms *mapSession) updateStateFromResponse(resp *tailcfg.MapResponse) { patchifiedPeerEqual = clientmetric.NewCounter("controlclient_patchified_peer_equal") ) -// updatePeersStateFromResponseres updates ms.peers and ms.sortedPeers from res. It takes ownership of res. +// updatePeersStateFromResponseres updates ms.peers from resp. +// It takes ownership of resp. func (ms *mapSession) updatePeersStateFromResponse(resp *tailcfg.MapResponse) (stats updateStats) { - defer func() { - if stats.removed > 0 || stats.added > 0 { - ms.rebuildSorted() - } - }() - if ms.peers == nil { - ms.peers = make(map[tailcfg.NodeID]*tailcfg.NodeView) + ms.peers = make(map[tailcfg.NodeID]tailcfg.NodeView) } if len(resp.Peers) > 0 { @@ -384,12 +378,12 @@ func (ms *mapSession) updatePeersStateFromResponse(resp *tailcfg.MapResponse) (s keep := make(map[tailcfg.NodeID]bool, len(resp.Peers)) for _, n := range resp.Peers { keep[n.ID] = true - if vp, ok := ms.peers[n.ID]; ok { + lenBefore := len(ms.peers) + ms.peers[n.ID] = n.View() + if len(ms.peers) == lenBefore { stats.changed++ - *vp = n.View() } else { stats.added++ - ms.peers[n.ID] = ptr.To(n.View()) } } for id := range ms.peers { @@ -410,12 +404,12 @@ func (ms *mapSession) updatePeersStateFromResponse(resp *tailcfg.MapResponse) (s } for _, n := range resp.PeersChanged { - if vp, ok := ms.peers[n.ID]; ok { + lenBefore := len(ms.peers) + ms.peers[n.ID] = n.View() + if len(ms.peers) == lenBefore { stats.changed++ - *vp = n.View() } else { stats.added++ - ms.peers[n.ID] = ptr.To(n.View()) } } @@ -427,7 +421,7 @@ func (ms *mapSession) updatePeersStateFromResponse(resp *tailcfg.MapResponse) (s } else { mut.LastSeen = nil } - *vp = mut.View() + ms.peers[nodeID] = mut.View() stats.changed++ } } @@ -436,7 +430,7 @@ func (ms *mapSession) updatePeersStateFromResponse(resp *tailcfg.MapResponse) (s if vp, ok := ms.peers[nodeID]; ok { mut := vp.AsStruct() mut.Online = ptr.To(online) - *vp = mut.View() + ms.peers[nodeID] = mut.View() stats.changed++ } } @@ -488,31 +482,12 @@ func (ms *mapSession) updatePeersStateFromResponse(resp *tailcfg.MapResponse) (s mut.CapMap = v patchCapMap.Add(1) } - *vp = mut.View() + ms.peers[pc.NodeID] = mut.View() } return } -// rebuildSorted rebuilds ms.sortedPeers from ms.peers. It should be called -// after any additions or removals from peers. -func (ms *mapSession) rebuildSorted() { - if ms.sortedPeers == nil { - ms.sortedPeers = make([]*tailcfg.NodeView, 0, len(ms.peers)) - } else { - if len(ms.sortedPeers) > len(ms.peers) { - clear(ms.sortedPeers[len(ms.peers):]) - } - ms.sortedPeers = ms.sortedPeers[:0] - } - for _, p := range ms.peers { - ms.sortedPeers = append(ms.sortedPeers, p) - } - sort.Slice(ms.sortedPeers, func(i, j int) bool { - return ms.sortedPeers[i].ID() < ms.sortedPeers[j].ID() - }) -} - func (ms *mapSession) addUserProfile(nm *netmap.NetworkMap, userID tailcfg.UserID) { if userID == 0 { return @@ -576,7 +551,7 @@ func (ms *mapSession) patchifyPeer(n *tailcfg.Node) (_ *tailcfg.PeerChange, ok b if !ok { return nil, false } - return peerChangeDiff(*was, n) + return peerChangeDiff(was, n) } // peerChangeDiff returns the difference from 'was' to 'n', if possible. @@ -778,14 +753,19 @@ func peerChangeDiff(was tailcfg.NodeView, n *tailcfg.Node) (_ *tailcfg.PeerChang return ret, true } +func (ms *mapSession) sortedPeers() []tailcfg.NodeView { + ret := slicesx.MapValues(ms.peers) + slices.SortFunc(ret, func(a, b tailcfg.NodeView) int { + return cmp.Compare(a.ID(), b.ID()) + }) + return ret +} + // netmap returns a fully populated NetworkMap from the last state seen from // a call to updateStateFromResponse, filling in omitted // information from prior MapResponse values. func (ms *mapSession) netmap() *netmap.NetworkMap { - peerViews := make([]tailcfg.NodeView, len(ms.sortedPeers)) - for i, vp := range ms.sortedPeers { - peerViews[i] = *vp - } + peerViews := ms.sortedPeers() nm := &netmap.NetworkMap{ NodeKey: ms.publicNodeKey, diff --git a/control/controlclient/map_test.go b/control/controlclient/map_test.go index 897036a94..ad8f7dd6e 100644 --- a/control/controlclient/map_test.go +++ b/control/controlclient/map_test.go @@ -340,19 +340,18 @@ func TestUpdatePeersStateFromResponse(t *testing.T) { } ms := newTestMapSession(t, nil) for _, n := range tt.prev { - mak.Set(&ms.peers, n.ID, ptr.To(n.View())) + mak.Set(&ms.peers, n.ID, n.View()) } - ms.rebuildSorted() gotStats := ms.updatePeersStateFromResponse(tt.mapRes) - - got := make([]*tailcfg.Node, len(ms.sortedPeers)) - for i, vp := range ms.sortedPeers { - got[i] = vp.AsStruct() - } if gotStats != tt.wantStats { t.Errorf("got stats = %+v; want %+v", gotStats, tt.wantStats) } + + var got []*tailcfg.Node + for _, vp := range ms.sortedPeers() { + got = append(got, vp.AsStruct()) + } if !reflect.DeepEqual(got, tt.want) { t.Errorf("wrong results\n got: %s\nwant: %s", formatNodes(got), formatNodes(tt.want)) }