Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Benchmark is not (only) measuring the routing cost #24

Closed
julienschmidt opened this issue Nov 1, 2018 · 7 comments
Closed

Benchmark is not (only) measuring the routing cost #24

julienschmidt opened this issue Nov 1, 2018 · 7 comments

Comments

@julienschmidt
Copy link

Currently your benchmark code does the following:

func benchRoutes(b *testing.B, router http.Handler, routes []route) {
	b.N = 10000
	b.ReportAllocs()
	b.ResetTimer()

	for i := 0; i < b.N; i++ {
		for _, route := range routes {
			router.ServeHTTP(httptest.NewRecorder(), httptest.NewRequest(route.method, route.path, nil))
		}
	}
}

The critical part is the router.ServeHTTP(httptest.NewRecorder(), httptest.NewRequest(route.method, route.path, nil)) within the loop, meaning that you allocate and initialize a new Recorder and a Request in every loop iteration.
Unfortunately that is likely much more expensive than the actual routing, meaning that both the measured memory cost and the execution time are completely meaningless, since there is a huge baseline cost.

@xujiajun
Copy link
Owner

xujiajun commented Nov 2, 2018

@julienschmidt thanks your suggest. i try to modify my benchmark code does the following:

func benchRoutes(b *testing.B, router http.Handler, routes []route) {
	b.N = 10000
	b.ReportAllocs()
	b.ResetTimer()

	w := httptest.NewRecorder()
	r, _ := http.NewRequest("GET", "/", nil)

	for i := 0; i < b.N; i++ {
		for _, route := range routes {
			r.Method = route.method
			r.RequestURI = route.path
			router.ServeHTTP(w, r)
		}
	}
}

benchmark result:

➜  gorouter git:(master) ✗ go test -bench=.
GithubAPI Routes: 203
GithubAPI2 Routes: 203
   HttpRouter: 37464 Bytes
   GoRouter: 83616 Bytes
   trie-mux: 134280 Bytes
   MuxRouter: 1324192 Bytes
goos: darwin
goarch: amd64
pkg: github.com/xujiajun/gorouter
BenchmarkTrieMuxRouter-8   	   10000	    119290 ns/op	   38044 B/op	    1218 allocs/op
BenchmarkHttpRouter-8      	   10000	    101041 ns/op	   18450 B/op	     609 allocs/op
BenchmarkGoRouter-8        	   10000	    105853 ns/op	   29820 B/op	    1218 allocs/op
BenchmarkMuxRouter-8       	   10000	   4259585 ns/op	   28229 B/op	     812 allocs/op
PASS
ok  	github.com/xujiajun/gorouter	45.904s

right now? Could you give me any other advice?

@julienschmidt
Copy link
Author

Take a look at https://github.com/julienschmidt/go-http-routing-benchmark/blob/2b136956a56bc65dddfa4bdaf7d1728ae2c90d50/bench_test.go#L76
As far as I remember, it was important to set these other attributes properly too.

Btw: I don't mind if you copy some of the benchmark code, but please add proper attribution.

The b.ResetTimer() should come directly before the loop, when all preparation is done. You want to measure the execution, not the preparation.

@xujiajun
Copy link
Owner

xujiajun commented Nov 2, 2018

@julienschmidt thanks your suggest.you are awesome . i try to modify my benchmark code again:

// referred to https://github.com/julienschmidt/go-http-routing-benchmark/blob/2b136956a56bc65dddfa4bdaf7d1728ae2c90d50/bench_test.go#L76
func benchRoutes(b *testing.B, router http.Handler, routes []route) {
	w := httptest.NewRecorder()
	r, _ := http.NewRequest("GET", "/", nil)
	u := r.URL
	rq := u.RawQuery

	b.N = 10000
	b.ReportAllocs()
	b.ResetTimer()

	for i := 0; i < b.N; i++ {
		for _, route := range routes {
			r.Method = route.method
			r.RequestURI = route.path

			u.Path = route.path
			u.RawQuery = rq

			router.ServeHTTP(w, r)
		}
	}
}

benchmark result:

➜  gorouter git:(master) ✗ go test -bench=.
GithubAPI Routes: 203
GithubAPI2 Routes: 203
   HttpRouter: 37464 Bytes
   GoRouter: 83616 Bytes
   trie-mux: 134280 Bytes
   MuxRouter: 1324192 Bytes
goos: darwin
goarch: amd64
pkg: github.com/xujiajun/gorouter
BenchmarkTrieMuxRouter-8   	   10000	    127334 ns/op	   65856 B/op	     537 allocs/op
BenchmarkHttpRouter-8      	   10000	     34353 ns/op	   13792 B/op	     167 allocs/op
BenchmarkGoRouter-8        	   10000	     66331 ns/op	   13832 B/op	     406 allocs/op
BenchmarkMuxRouter-8       	   10000	   5814194 ns/op	  207171 B/op	    1760 allocs/op
PASS
ok  	github.com/xujiajun/gorouter	84.822s

BTW why should set these other attributes properly too. could you tell me the reason?

@negbie
Copy link

negbie commented Nov 2, 2018

@xujiajun Mby it would be nice to include github.com/husobee/vestigo since it's a stand alone url router which is std lib compatible like your young project. Mby you can get some inspiration from it.

@xujiajun
Copy link
Owner

xujiajun commented Nov 3, 2018

@negbie thanks your suggest.

@xujiajun
Copy link
Owner

xujiajun commented Nov 3, 2018

@julienschmidt i have updated the REDME about benchmark: https://github.com/xujiajun/gorouter#benchmarks, any other advice?

@julienschmidt
Copy link
Author

julienschmidt commented Nov 5, 2018

I think the benchmark looks fine now.
However, the bench_test.go seems to be copied mostly from go-http-routing-benchmark. It should state so in the file and attribute the copyright to the original author. Strictly speaking, it would also have to be published under a compatible license, but I'm fine with the MIT License (in this case).

I'm closing this issue, as the benchmark is fixed now.

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

No branches or pull requests

3 participants