cloudfoundry / gorouter

CF Router
Apache License 2.0
441 stars 226 forks source link

Improve pool.Endpoint.Equals() peformance #375

Closed peanball closed 11 months ago

peanball commented 11 months ago

The change improves the Endpoint.Equal() function to use less time and no allocations during comparison. In profiler runs, @domdom82 identified that a lot of time is spent on the two fmt.Sprintf calls for comparison of the tags maps.

Route registration messages that happen on route refresh are compared with the existing endpoints in the route registry for every received router.register message. This happens for every route by default every 20 seconds. In our scenarios we have up to 70k routes (on each gorouter) and such comparisons add up.

Functionality is not changed. A benchmark has been added to show the changes.

Route registration (and re-registration) should be a little faster. This will have measurable effect for large numbers of routes and route registration messages only.

Route registration takes about 8% of the processing time in Gorouter in our profiling examples. See https://github.com/cloudfoundry/routing-release/issues/366.

peanball commented 11 months ago

Some benchmark results:

There is a new benchmark, pool_bench_test.go:

BenchmarkEndpointEquals-16                695605          1597 ns/op         480 B/op         16 allocs/op <- fmt.Sprintf
BenchmarkEndpointEquals-16              13326138            83.46 ns/op        0 B/op          0 allocs/op <- maps.Equal

While this is a microbenchmark and the difference between those two ops looks very stark, the pool endpoint insertion is a small part of the overall processing. Eliminating allocations should be a significant benefit however for this often-called function in terms of cache coherency, reducing memory access, etc.

Finally, the results of registry_benchmark_test.go:

old version
BenchmarkRegisterWith100KRoutes-16        939470          1712 ns/op        1546 B/op         11 allocs/op
BenchmarkRegisterWithOneRoute-16          977769          1208 ns/op        1546 B/op         11 allocs/op

new version
BenchmarkRegisterWith100KRoutes-16       1000000          1312 ns/op        1440 B/op          7 allocs/op
BenchmarkRegisterWithOneRoute-16         1281951           839.3 ns/op      1440 B/op          7 allocs/op

There are fewer allocations and generally less time per iteration with the new comparator.

The semantics of Endpoint.Equals() remain identical.

maxmoehl commented 11 months ago

When merging this into routing-release, please remember to bump the go directive to at least 1.21.0.

peanball commented 11 months ago

Good point. We might need a separate PR to prepare routing release. The gorouter PR will just be pulled in before the respective RR PR by default.

ameowlia commented 11 months ago

https://github.com/cloudfoundry/routing-release/commit/da3bb317a517178a86575bc8ff0e6b8601504532 - merged into routing release

ameowlia commented 11 months ago

Oh no! This is failing go vet...

go vet ./...
# code.cloudfoundry.org/gorouter/route_test
gorouter/route/pool_bench_test.go:35:14: variable declaration copies lock value to endpoint3: code.cloudfoundry.org/gorouter/route.Endpoint contains sync.RWMutex

I'm reverting the commits so our pipeline isn't red. Let me know when you fix it and I will merge again.

domdom82 commented 11 months ago

Neat!