control/controlclient: rewrite, test NetworkMap.ConciseDiffFrom

It stood out a lot in hello.ipn.dev's profiles for generating a lot of
garbage (and thus GC CPU).

control/controlclient/netmap.go

@@ -30,7 +30,7 @@ type NetworkMap struct {
 	Addresses     []wgcfg.CIDR
 	LocalPort     uint16 // used for debugging
 	MachineStatus tailcfg.MachineStatus
-	Peers         []*tailcfg.Node
+	Peers         []*tailcfg.Node // sorted by Node.ID
 	DNS           []wgcfg.IP
 	DNSDomains    []string
 	Hostinfo      tailcfg.Hostinfo
@@ -56,6 +56,13 @@ type NetworkMap struct {
 }
 
 func (n *NetworkMap) Equal(n2 *NetworkMap) bool {
+	if n == nil && n2 == nil {
+		return true
+	}
+	if n == nil || n2 == nil {
+		return false
+	}
+
 	// TODO(crawshaw): this is crude, but is an easy way to avoid bugs.
 	b, err := json.Marshal(n)
 	if err != nil {
@@ -74,6 +81,19 @@ func (nm NetworkMap) String() string {
 
 func (nm *NetworkMap) Concise() string {
 	buf := new(strings.Builder)
+
+	nm.printConciseHeader(buf)
+	for _, p := range nm.Peers {
+		printPeerConcise(buf, p)
+	}
+	return buf.String()
+}
+
+// printConciseHeader prints a concise header line representing nm to buf.
+//
+// If this function is changed to access different fields of nm, keep
+// in equalConciseHeader in sync.
+func (nm *NetworkMap) printConciseHeader(buf *strings.Builder) {
 	fmt.Fprintf(buf, "netmap: self: %v auth=%v",
 		nm.NodeKey.ShortString(), nm.MachineStatus)
 	if nm.LocalPort != 0 {
@@ -85,72 +105,116 @@ func (nm *NetworkMap) Concise() string {
 	}
 	fmt.Fprintf(buf, " %v", nm.Addresses)
 	buf.WriteByte('\n')
-	for _, p := range nm.Peers {
+}
-		aip := make([]string, len(p.AllowedIPs))
-		for i, a := range p.AllowedIPs {
-			s := strings.TrimSuffix(fmt.Sprint(a), "/32")
-			aip[i] = s
-		}
 
-		ep := make([]string, len(p.Endpoints))
+// equalConciseHeader reports whether a and b are equal for the fields
-		for i, e := range p.Endpoints {
+// used by printConciseHeader.
-			// Align vertically on the ':' between IP and port
+func (a *NetworkMap) equalConciseHeader(b *NetworkMap) bool {
-			colon := strings.IndexByte(e, ':')
+	if a.NodeKey != b.NodeKey ||
-			spaces := 0
+		a.MachineStatus != b.MachineStatus ||
-			for colon > 0 && len(e)+spaces-colon < 6 {
+		a.LocalPort != b.LocalPort ||
-				spaces++
+		len(a.Addresses) != len(b.Addresses) {
-				colon--
+		return false
-			}
-			ep[i] = fmt.Sprintf("%21v", e+strings.Repeat(" ", spaces))
-		}
-
-		derp := p.DERP
-		const derpPrefix = "127.3.3.40:"
-		if strings.HasPrefix(derp, derpPrefix) {
-			derp = "D" + derp[len(derpPrefix):]
-		}
-
-		// Most of the time, aip is just one element, so format the
-		// table to look good in that case. This will also make multi-
-		// subnet nodes stand out visually.
-		fmt.Fprintf(buf, " %v %-2v %-15v : %v\n",
-			p.Key.ShortString(), derp,
-			strings.Join(aip, " "),
-			strings.Join(ep, " "))
 	}
-	return buf.String()
+	for i, a := range a.Addresses {
+		if b.Addresses[i] != a {
+			return false
+		}
+	}
+	return (a.Debug == nil && b.Debug == nil) || reflect.DeepEqual(a.Debug, b.Debug)
+}
+
+// printPeerConcise appends to buf a line repsenting the peer p.
+//
+// If this function is changed to access different fields of p, keep
+// in nodeConciseEqual in sync.
+func printPeerConcise(buf *strings.Builder, p *tailcfg.Node) {
+	aip := make([]string, len(p.AllowedIPs))
+	for i, a := range p.AllowedIPs {
+		s := strings.TrimSuffix(fmt.Sprint(a), "/32")
+		aip[i] = s
+	}
+
+	ep := make([]string, len(p.Endpoints))
+	for i, e := range p.Endpoints {
+		// Align vertically on the ':' between IP and port
+		colon := strings.IndexByte(e, ':')
+		spaces := 0
+		for colon > 0 && len(e)+spaces-colon < 6 {
+			spaces++
+			colon--
+		}
+		ep[i] = fmt.Sprintf("%21v", e+strings.Repeat(" ", spaces))
+	}
+
+	derp := p.DERP
+	const derpPrefix = "127.3.3.40:"
+	if strings.HasPrefix(derp, derpPrefix) {
+		derp = "D" + derp[len(derpPrefix):]
+	}
+
+	// Most of the time, aip is just one element, so format the
+	// table to look good in that case. This will also make multi-
+	// subnet nodes stand out visually.
+	fmt.Fprintf(buf, " %v %-2v %-15v : %v\n",
+		p.Key.ShortString(), derp,
+		strings.Join(aip, " "),
+		strings.Join(ep, " "))
+}
+
+// nodeConciseEqual reports whether a and b are equal for the fields accessed by printPeerConcise.
+func nodeConciseEqual(a, b *tailcfg.Node) bool {
+	return a.Key == b.Key &&
+		a.DERP == b.DERP &&
+		eqCIDRsIgnoreNil(a.AllowedIPs, b.AllowedIPs) &&
+		eqStringsIgnoreNil(a.Endpoints, b.Endpoints)
 }
 
 func (b *NetworkMap) ConciseDiffFrom(a *NetworkMap) string {
-	if reflect.DeepEqual(a, b) {
+	var diff strings.Builder
-		// Fast path that only does one allocation.
-		return ""
-	}
-	out := []string{}
-	ra := strings.Split(a.Concise(), "\n")
-	rb := strings.Split(b.Concise(), "\n")
 
-	ma := map[string]struct{}{}
+	// See if header (non-peers, "bare") part of the network map changed.
-	for _, s := range ra {
+	// If so, print its diff lines first.
-		ma[s] = struct{}{}
+	if !a.equalConciseHeader(b) {
+		diff.WriteByte('-')
+		a.printConciseHeader(&diff)
+		diff.WriteByte('+')
+		b.printConciseHeader(&diff)
 	}
 
-	mb := map[string]struct{}{}
+	aps, bps := a.Peers, b.Peers
-	for _, s := range rb {
+	for len(aps) > 0 && len(bps) > 0 {
-		mb[s] = struct{}{}
+		pa, pb := aps[0], bps[0]
-	}
+		switch {
-
+		case pa.ID == pb.ID:
-	for _, s := range ra {
+			if !nodeConciseEqual(pa, pb) {
-		if _, ok := mb[s]; !ok {
+				diff.WriteByte('-')
-			out = append(out, "-"+s)
+				printPeerConcise(&diff, pa)
+				diff.WriteByte('+')
+				printPeerConcise(&diff, pb)
+			}
+			aps, bps = aps[1:], bps[1:]
+		case pa.ID > pb.ID:
+			// New peer in b.
+			diff.WriteByte('+')
+			printPeerConcise(&diff, pb)
+			bps = bps[1:]
+		case pb.ID > pa.ID:
+			// Deleted peer in b.
+			diff.WriteByte('-')
+			printPeerConcise(&diff, pa)
+			aps = aps[1:]
 		}
 	}
-	for _, s := range rb {
+	for _, pa := range aps {
-		if _, ok := ma[s]; !ok {
+		diff.WriteByte('-')
-			out = append(out, "+"+s)
+		printPeerConcise(&diff, pa)
-		}
 	}
-	return strings.Join(out, "\n")
+	for _, pb := range bps {
+		diff.WriteByte('+')
+		printPeerConcise(&diff, pb)
+	}
+	return diff.String()
 }
 
 func (nm *NetworkMap) JSON() string {
@@ -271,3 +335,31 @@ func appendEndpoint(peer *wgcfg.Peer, epStr string) error {
 	peer.Endpoints = append(peer.Endpoints, wgcfg.Endpoint{Host: host, Port: uint16(port16)})
 	return nil
 }
+
+// eqStringsIgnoreNil reports whether a and b have the same length and
+// contents, but ignore whether a or b are nil.
+func eqStringsIgnoreNil(a, b []string) bool {
+	if len(a) != len(b) {
+		return false
+	}
+	for i, v := range a {
+		if v != b[i] {
+			return false
+		}
+	}
+	return true
+}
+
+// eqCIDRsIgnoreNil reports whether a and b have the same length and
+// contents, but ignore whether a or b are nil.
+func eqCIDRsIgnoreNil(a, b []wgcfg.CIDR) bool {
+	if len(a) != len(b) {
+		return false
+	}
+	for i, v := range a {
+		if v != b[i] {
+			return false
+		}
+	}
+	return true
+}

control/controlclient/netmap_test.go

@@ -10,13 +10,14 @@ import (
 	"tailscale.com/tailcfg"
 )
 
-func TestNetworkMapConcise(t *testing.T) {
+func testNodeKey(b byte) (ret tailcfg.NodeKey) {
-	nodekey := func(b byte) (ret tailcfg.NodeKey) {
+	for i := range ret {
-		for i := range ret {
+		ret[i] = b
-			ret[i] = b
-		}
-		return
 	}
+	return
+}
+
+func TestNetworkMapConcise(t *testing.T) {
 	for _, tt := range []struct {
 		name string
 		nm   *NetworkMap
@@ -25,15 +26,15 @@ func TestNetworkMapConcise(t *testing.T) {
 		{
 			name: "basic",
 			nm: &NetworkMap{
-				NodeKey: nodekey(1),
+				NodeKey: testNodeKey(1),
 				Peers: []*tailcfg.Node{
 					{
-						Key:       nodekey(2),
+						Key:       testNodeKey(2),
 						DERP:      "127.3.3.40:2",
 						Endpoints: []string{"192.168.0.100:12", "192.168.0.100:12354"},
 					},
 					{
-						Key:       nodekey(3),
+						Key:       testNodeKey(3),
 						DERP:      "127.3.3.40:4",
 						Endpoints: []string{"10.2.0.100:12", "10.1.0.100:12345"},
 					},
@@ -44,7 +45,7 @@ func TestNetworkMapConcise(t *testing.T) {
 		{
 			name: "debug_non_nil",
 			nm: &NetworkMap{
-				NodeKey: nodekey(1),
+				NodeKey: testNodeKey(1),
 				Debug:   &tailcfg.Debug{},
 			},
 			want: "netmap: self: [AQEBA] auth=machine-unknown debug={} []\n",
@@ -52,7 +53,7 @@ func TestNetworkMapConcise(t *testing.T) {
 		{
 			name: "debug_values",
 			nm: &NetworkMap{
-				NodeKey: nodekey(1),
+				NodeKey: testNodeKey(1),
 				Debug:   &tailcfg.Debug{LogHeapPprof: true},
 			},
 			want: "netmap: self: [AQEBA] auth=machine-unknown debug={\"LogHeapPprof\":true} []\n",
@@ -70,3 +71,147 @@ func TestNetworkMapConcise(t *testing.T) {
 		})
 	}
 }
+
+func TestConciseDiffFrom(t *testing.T) {
+	for _, tt := range []struct {
+		name string
+		a, b *NetworkMap
+		want string
+	}{
+		{
+			name: "no_change",
+			a: &NetworkMap{
+				NodeKey: testNodeKey(1),
+				Peers: []*tailcfg.Node{
+					{
+						Key:       testNodeKey(2),
+						DERP:      "127.3.3.40:2",
+						Endpoints: []string{"192.168.0.100:12", "192.168.0.100:12354"},
+					},
+				},
+			},
+			b: &NetworkMap{
+				NodeKey: testNodeKey(1),
+				Peers: []*tailcfg.Node{
+					{
+						Key:       testNodeKey(2),
+						DERP:      "127.3.3.40:2",
+						Endpoints: []string{"192.168.0.100:12", "192.168.0.100:12354"},
+					},
+				},
+			},
+			want: "",
+		},
+		{
+			name: "header_change",
+			a: &NetworkMap{
+				NodeKey: testNodeKey(1),
+				Peers: []*tailcfg.Node{
+					{
+						Key:       testNodeKey(2),
+						DERP:      "127.3.3.40:2",
+						Endpoints: []string{"192.168.0.100:12", "192.168.0.100:12354"},
+					},
+				},
+			},
+			b: &NetworkMap{
+				NodeKey: testNodeKey(2),
+				Peers: []*tailcfg.Node{
+					{
+						Key:       testNodeKey(2),
+						DERP:      "127.3.3.40:2",
+						Endpoints: []string{"192.168.0.100:12", "192.168.0.100:12354"},
+					},
+				},
+			},
+			want: "-netmap: self: [AQEBA] auth=machine-unknown []\n+netmap: self: [AgICA] auth=machine-unknown []\n",
+		},
+		{
+			name: "peer_add",
+			a: &NetworkMap{
+				NodeKey: testNodeKey(1),
+				Peers: []*tailcfg.Node{
+					{
+						ID:        2,
+						Key:       testNodeKey(2),
+						DERP:      "127.3.3.40:2",
+						Endpoints: []string{"192.168.0.100:12", "192.168.0.100:12354"},
+					},
+				},
+			},
+			b: &NetworkMap{
+				NodeKey: testNodeKey(1),
+				Peers: []*tailcfg.Node{
+					{
+						ID:        1,
+						Key:       testNodeKey(1),
+						DERP:      "127.3.3.40:1",
+						Endpoints: []string{"192.168.0.100:12", "192.168.0.100:12354"},
+					},
+					{
+						ID:        2,
+						Key:       testNodeKey(2),
+						DERP:      "127.3.3.40:2",
+						Endpoints: []string{"192.168.0.100:12", "192.168.0.100:12354"},
+					},
+					{
+						ID:        3,
+						Key:       testNodeKey(3),
+						DERP:      "127.3.3.40:3",
+						Endpoints: []string{"192.168.0.100:12", "192.168.0.100:12354"},
+					},
+				},
+			},
+			want: "+ [AQEBA] D1                 :    192.168.0.100:12     192.168.0.100:12354\n+ [AwMDA] D3                 :    192.168.0.100:12     192.168.0.100:12354\n",
+		},
+		{
+			name: "peer_remove",
+			a: &NetworkMap{
+				NodeKey: testNodeKey(1),
+				Peers: []*tailcfg.Node{
+					{
+						ID:        1,
+						Key:       testNodeKey(1),
+						DERP:      "127.3.3.40:1",
+						Endpoints: []string{"192.168.0.100:12", "192.168.0.100:12354"},
+					},
+					{
+						ID:        2,
+						Key:       testNodeKey(2),
+						DERP:      "127.3.3.40:2",
+						Endpoints: []string{"192.168.0.100:12", "192.168.0.100:12354"},
+					},
+					{
+						ID:        3,
+						Key:       testNodeKey(3),
+						DERP:      "127.3.3.40:3",
+						Endpoints: []string{"192.168.0.100:12", "192.168.0.100:12354"},
+					},
+				},
+			},
+			b: &NetworkMap{
+				NodeKey: testNodeKey(1),
+				Peers: []*tailcfg.Node{
+					{
+						ID:        2,
+						Key:       testNodeKey(2),
+						DERP:      "127.3.3.40:2",
+						Endpoints: []string{"192.168.0.100:12", "192.168.0.100:12354"},
+					},
+				},
+			},
+			want: "- [AQEBA] D1                 :    192.168.0.100:12     192.168.0.100:12354\n- [AwMDA] D3                 :    192.168.0.100:12     192.168.0.100:12354\n",
+		},
+	} {
+		t.Run(tt.name, func(t *testing.T) {
+			var got string
+			n := int(testing.AllocsPerRun(50, func() {
+				got = tt.b.ConciseDiffFrom(tt.a)
+			}))
+			t.Logf("Allocs = %d", n)
+			if got != tt.want {
+				t.Errorf("Wrong output\n Got: %q\nWant: %q\n## Got (unescaped):\n%s\n## Want (unescaped):\n%s\n", got, tt.want, got, tt.want)
+			}
+		})
+	}
+}