GODRIVER-3542 use pointers to optimize equalTopologies func

[isopov]

GODRIVER-3542

Optimize equalTopologies

Background & Motivation

In our idle service this method in about 2 hours generates about 1GB of allocations. I want to reduce CPU load on GC from this idle service.

Included benchmark on my machine produces the following results before

BenchmarkEqualTopologies
BenchmarkEqualTopologies-32    	   82350	     13399 ns/op	   90195 B/op	     206 allocs/op
PASS

and after

BenchmarkEqualTopologies
BenchmarkEqualTopologies-32    	  198764	      8329 ns/op	    6992 B/op	       6 allocs/op
PASS

this change.

[isopov]

BTW, changing this string to

Addr:              address.Address("127.0.0." + strconv.Itoa(i)),

turns benchmark results into

BenchmarkEqualTopologies
BenchmarkEqualTopologies-32    	  133854	      8716 ns/op	   35793 B/op	     906 allocs/op
PASS

[isopov]

I've introduced NPD issue in my change, so I fixed it and added another test.

[isopov]

I've understood that we can reuse already computed canonical string for the address from the map key.

Benchmarking (different machine from previous results) produce slight improvement:

• Before

BenchmarkEqualTopologies
BenchmarkEqualTopologies-10    	  281943	      4715 ns/op	    6992 B/op	       6 allocs/op
PASS

• After

BenchmarkEqualTopologies
BenchmarkEqualTopologies-10    	  317918	      4228 ns/op	    6992 B/op	       6 allocs/op
PASS

And with non-canonical address (address.Address("127.0.0." + strconv.Itoa(i)))

• Before

BenchmarkEqualTopologies
BenchmarkEqualTopologies-10    	   66252	     17721 ns/op	   35793 B/op	     906 allocs/op
PASS

• After

BenchmarkEqualTopologies
BenchmarkEqualTopologies-10    	   91142	     12960 ns/op	   26193 B/op	     606 allocs/op
PASS

So we have pretty big additional improvement for free

[mongodb-drivers-pr-bot]

API Change Report

No changes found!

[prestonvasquez]

@isopov Thanks for the PR!

[prestonvasquez]

equalTopologies really isn't on a per-operation hot path. From what I can tell, it's only run in processSRVResults and apply. So under normal conditions this function gets called when heartbeat comes back (default 10s) and when an SRV record is rescanned (default 60s). If allocations are a legitimate concern I suggest we consider opting for a 0-allocation O(n^2) solution:

func equalTopologies(t1, t2 description.Topology) bool {
	if t1.Kind != t2.Kind {
		return false
	}

	if len(t1.Servers) != len(t2.Servers) {
		return false
	}

	for _, s1 := range t1.Servers {
		found := false
		for _, s2 := range t2.Servers {
			if s1.Addr == s2.Addr && driverutil.EqualServers(s1, s2) {
				found = true
				break
			}
		}
		if !found {
			return false
		}
	}

	return true
}

This should lower future maintenance burden concerning pointers, I'm specifically thinking of slice reallocations. What are your thoughts?

Consider updating the benchmark to iterate over three different sizes: 10, 100, 1000 when comparing this solution to the pointer solution.

[isopov]

This method you suggested has a huge optimization

	if len(t1.Servers) != len(t2.Servers) {
		return false
	}

which is applicable to any other implementation, but breaks these now passing tests:

	assert.True(t, equalTopologies(
		description.Topology{
			Servers: []description.Server{{
				Addr: "127.0.0.1:27017",
			}, {
				Addr: "127.0.0.1:27017",
			}},
		}, description.Topology{
			Servers: []description.Server{{
				Addr: "127.0.0.1:27017",
			}},
		},
	))
	assert.True(t, equalTopologies(
		description.Topology{
			Servers: []description.Server{{
				Addr: "127.0.0.1",
			}, {
				Addr: "127.0.0.1:27017",
			}},
		}, description.Topology{
			Servers: []description.Server{{
				Addr: "127.0.0.1:27017",
			}},
		},
	))

Also your method does no canonical addresses via String() that seems to be the most expensive operation in this method.
So this test fails:

	assert.True(t, equalTopologies(
		description.Topology{
			Servers: []description.Server{{
				Addr: "127.0.0.1",
			}},
		}, description.Topology{
			Servers: []description.Server{{
				Addr: "127.0.0.1:27017",
			}},
		},
	))

With all of the above your method gives me on the current benchmark:

BenchmarkEqualTopologies
BenchmarkEqualTopologies-32    	  605614	      1888 ns/op	       0 B/op	       0 allocs/op

For 100 distinct servers in two equal topologies.

[isopov]

If zero-allocation effect should be tested it can be done this way:

func BenchmarkEqualTopologies(b *testing.B) {
	for _, bsize := range []int{10, 100, 1000} {
		b.Run(strconv.Itoa(bsize), benchmarkEqualTopologiesWithSize(bsize))
	}
}

func benchmarkEqualTopologiesWithSize(size int) func(b *testing.B) {
	return func(b *testing.B) {
		servers := make([]description.Server, size)
		for i := 0; i < len(servers); i++ {
			servers[i] = description.Server{
				Addr:              address.Address("127.0.0." + strconv.Itoa(i) + ":27017"),
				HeartbeatInterval: time.Duration(10) * time.Second,
				LastWriteTime:     time.Date(2017, 2, 11, 14, 0, 0, 0, time.UTC),
				LastUpdateTime:    time.Date(2017, 2, 11, 14, 0, 2, 0, time.UTC),
				Kind:              description.ServerKindMongos,
				WireVersion:       &description.VersionRange{Min: 6, Max: 21},
			}
		}
		desc := description.Topology{
			Servers: servers,
		}

		b.ResetTimer()
		b.RunParallel(func(p *testing.PB) {
			b.ReportAllocs()
			for p.Next() {
				_ = equalTopologies(desc, desc)
			}
		})
	}
}

func TestEqualTopologiesZeroAllocations(t *testing.T) {
	result := testing.Benchmark(benchmarkEqualTopologiesWithSize(10))
	assert.EqualValues(t, 0, result.AllocsPerOp())
}

[prestonvasquez]

Good catch, I think the O(n^2) solution is still valid. Here is a polished version:

func equalTopologies(t1, t2 description.Topology) bool {
	if t1.Kind != t2.Kind {
		return false
	}

	// Every server in t1 has to appear in t2:
	for _, s1 := range t1.Servers {
		addr1 := s1.Addr.String()
		found := false
		for _, s2 := range t2.Servers {
			if addr1 == s2.Addr.String() && driverutil.EqualServers(s1, s2) {
				found = true
				break
			}
		}
		if !found {
			return false
		}
	}

	// Every server in t2 has to appear in t1:
	for _, s2 := range t2.Servers {
		addr2 := s2.Addr.String()
		found := false
		for _, s1 := range t1.Servers {
			if s1.Addr.String() == addr2 && driverutil.EqualServers(s1, s2) {
				found = true
				break
			}
		}
		if !found {
			return false
		}
	}

	return true
}

We are over 10x the ns/op in the benchmark as a trade-off for 0 allocations:

BenchmarkEqualTopologies-10        21643             55066 ns/op               3 B/op          0 allocs/op
PASS
ok      go.mongodb.org/mongo-driver/v2/x/mongo/driver/topology  32.328s

@qingyang-hu , @isopov What are your thoughts? Again, I only expect this function to be called when a heartbeat comes back (default 10s) and when an SRV record is rescanned (default 60s).

[qingyang-hu]

I agree that we don't have to be aggressive because this function is not on a hot path.

As a tradeoff, we can reduce the allocations by half from @isopov's solution once we determine the number of duplicates in topo2.
For example:

func equalTopologies(topo1, topo2 description.Topology) bool {
	if topo1.Kind != topo2.Kind {
		return false
	}

	topoServers := make(map[string]uint, len(topo1.Servers)) // Use a unit map value for index in case it exceeds the limit with offset.
	for i := range topo1.Servers {
		topoServers[topo1.Servers[i].Addr.String()] = uint(i)
	}

	dupCnt := 0
	offset := uint(len(topo1.Servers))
	for i := 0; i < len(topo2.Servers); i++ {
		addr := topo2.Servers[i].Addr.String()
		serverIdx, ok := topoServers[addr]
		if !ok {
			return false
		}

		if serverIdx >= offset {
			dupCnt++
		} else {
			topoServers[addr] += offset // Add an offset to mark a duplicate.
			if !driverutil.EqualServers(topo1.Servers[serverIdx], topo2.Servers[i]) {
				return false
			}
		}
	}
	return len(topoServers) == len(topo2.Servers)-dupCnt
}

[prestonvasquez]

@isopov? O(n^2) or uint are the team preferences. Do you have thoughts? Here are the current results:

Pointer:

BenchmarkEqualTopologies/10-10   2732804               412.5 ns/op           840 B/op          2 allocs/op
BenchmarkEqualTopologies/100-10                   308890              4178 ns/op            8362 B/op          4 allocs/op
BenchmarkEqualTopologies/1000-10                   28926             41944 ns/op          114739 B/op          4 allocs/op
PASS
ok      go.mongodb.org/mongo-driver/v2/x/mongo/driver/topology  35.483s

O(n^2):

BenchmarkEqualTopologies/10-10   1641102               730.9 ns/op             0 B/op          0 allocs/op
BenchmarkEqualTopologies/100-10                    21530             54448 ns/op               2 B/op          0 allocs/op
BenchmarkEqualTopologies/1000-10                     193           5718903 ns/op             304 B/op          2 allocs/op
PASS
ok      go.mongodb.org/mongo-driver/v2/x/mongo/driver/topology  34.935s

uint:

BenchmarkEqualTopologies/10-10   4112641               291.5 ns/op           420 B/op          1 allocs/op
BenchmarkEqualTopologies/100-10                   412269              2842 ns/op            4181 B/op          2 allocs/op
BenchmarkEqualTopologies/1000-10                   40996             29112 ns/op           57369 B/op          2 allocs/op
PASS
ok      go.mongodb.org/mongo-driver/v2/x/mongo/driver/topology  24.280s

[prestonvasquez]

We should extend this test with the tests noted in the comment above:

	assert.True(t, equalTopologies(
		description.Topology{
			Servers: []description.Server{{
				Addr: "127.0.0.1:27017",
			}, {
				Addr: "127.0.0.1:27017",
			}},
		}, description.Topology{
			Servers: []description.Server{{
				Addr: "127.0.0.1:27017",
			}},
		},
	))
	assert.True(t, equalTopologies(
		description.Topology{
			Servers: []description.Server{{
				Addr: "127.0.0.1",
			}, {
				Addr: "127.0.0.1:27017",
			}},
		}, description.Topology{
			Servers: []description.Server{{
				Addr: "127.0.0.1:27017",
			}},
		},
	))

Optionally, we should parameterize this test:

func TestEqualTopologies(t *testing.T) {
	tests := []struct{
		name string 
		t1, t2 description.Topology
		wantEqual bool
	}{
		// TODO
	}

	for _, test := range tests {
		t.Run(test.name, func(t *testing.T) {
			assert.Equal(t, test.wantEqual, equalTopologies(test.t1, test.t2))
		})
	}
}