groupcache: enable H2C + add benchmarks #5068
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add h2c benchmarks. Somehow h2c is slowed than regular HTTP locally for me. Probably because these aren't real life benchmarks i.e. establishing TCP connections locally has no cost. And if there is only one operation happening at any time then h2 has to do more work hence more CPU usage. With 500 parallel requests going on at the same time, h2 becomes faster than (or is at the same level as) h1 while using a minimal number of TCP connections. Thus, it will work much faster in real life situations. So, let's enable it. ``` name time/op GroupcacheRetrieval/h2c/seq-16 227µs ± 1% GroupcacheRetrieval/h2c/parallel=500-16 54.8µs ±20% GroupcacheRetrieval/h1,_max_one_TCP_connection/seq-16 150µs ± 2% GroupcacheRetrieval/h1,_max_one_TCP_connection/parallel=500-16 146µs ± 1% GroupcacheRetrieval/h1,_unlimited_TCP_connections/seq-16 145µs ± 3% GroupcacheRetrieval/h1,_unlimited_TCP_connections/parallel=500-16 52.8µs ± 9% name alloc/op GroupcacheRetrieval/h2c/seq-16 183kB ± 0% GroupcacheRetrieval/h2c/parallel=500-16 143kB ± 1% GroupcacheRetrieval/h1,_max_one_TCP_connection/seq-16 161kB ± 0% GroupcacheRetrieval/h1,_max_one_TCP_connection/parallel=500-16 162kB ± 0% GroupcacheRetrieval/h1,_unlimited_TCP_connections/seq-16 161kB ± 0% GroupcacheRetrieval/h1,_unlimited_TCP_connections/parallel=500-16 116kB ± 2% name allocs/op GroupcacheRetrieval/h2c/seq-16 283 ± 0% GroupcacheRetrieval/h2c/parallel=500-16 256 ± 1% GroupcacheRetrieval/h1,_max_one_TCP_connection/seq-16 260 ± 0% GroupcacheRetrieval/h1,_max_one_TCP_connection/parallel=500-16 262 ± 0% GroupcacheRetrieval/h1,_unlimited_TCP_connections/seq-16 260 ± 0% GroupcacheRetrieval/h1,_unlimited_TCP_connections/parallel=500-16 279 ± 1% ``` Signed-off-by: Giedrius Statkevičius <[email protected]>
GiedriusS
commented
Jan 18, 2022
| @@ -93,6 +96,12 @@ func NewGroupcacheWithConfig(logger log.Logger, reg prometheus.Registerer, conf | |||
| cfg *CachingBucketConfig) (*Groupcache, error) { | |||
| httpProto := galaxyhttp.NewHTTPFetchProtocol(&galaxyhttp.HTTPOptions{ | |||
| BasePath: basepath, | |||
| Transport: &http2.Transport{ | |||
| AllowHTTP: true, | |||
| DialTLS: func(network, addr string, cfg *tls.Config) (net.Conn, error) { | |||
There was a problem hiding this comment.
For now, let's disable TLS but in the future, we can come back to this.
There was a problem hiding this comment.
Shall we open an issue for it?
There was a problem hiding this comment.
I will add it to #5037
7 tasks
marijus-ravickas
previously approved these changes
Jan 18, 2022
kakkoyun
previously approved these changes
Jan 18, 2022
| @@ -93,6 +96,12 @@ func NewGroupcacheWithConfig(logger log.Logger, reg prometheus.Registerer, conf | |||
| cfg *CachingBucketConfig) (*Groupcache, error) { | |||
| httpProto := galaxyhttp.NewHTTPFetchProtocol(&galaxyhttp.HTTPOptions{ | |||
| BasePath: basepath, | |||
| Transport: &http2.Transport{ | |||
| AllowHTTP: true, | |||
| DialTLS: func(network, addr string, cfg *tls.Config) (net.Conn, error) { | |||
There was a problem hiding this comment.
Shall we open an issue for it?
Signed-off-by: Giedrius Statkevičius <[email protected]>
GiedriusS
dismissed stale reviews from kakkoyun and marijus-ravickas
via
72f80bf
onprem
reviewed
Jan 19, 2022
|
|
||
| go func() { | ||
| if err = httpServer.ListenAndServe(); err != nil { | ||
| fmt.Printf("failed to listen: %s\n", err.Error()) |
There was a problem hiding this comment.
We should do an os.Exit(1) here as well. If we fail to start the http server, we should fail the test here instead of continuing with it.
| }() | ||
| go func() { | ||
| if err = httpServerH2C.ListenAndServe(); err != nil { | ||
| fmt.Printf("failed to listen: %s\n", err.Error()) |
GiedriusS
added a commit
to GiedriusS/thanos
that referenced
this pull request
Jan 20, 2022
Add fixes according to the suggestions here thanos-io#5068 (comment). Signed-off-by: Giedrius Statkevičius <[email protected]>
GiedriusS
added a commit
to GiedriusS/thanos
that referenced
this pull request
Jan 20, 2022
Add fixes according to the suggestions here thanos-io#5068 (comment). Signed-off-by: Giedrius Statkevičius <[email protected]>
GiedriusS
added a commit
that referenced
this pull request
Jan 26, 2022
Add fixes according to the suggestions here #5068 (comment). Signed-off-by: Giedrius Statkevičius <[email protected]>
Nicholaswang
pushed a commit
to Nicholaswang/thanos
that referenced
this pull request
Mar 6, 2022
Add fixes according to the suggestions here thanos-io#5068 (comment). Signed-off-by: Giedrius Statkevičius <[email protected]> Signed-off-by: Nicholaswang <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add h2c benchmarks. Somehow h2c is slower than regular HTTP locally for
me. Probably because these aren't real life benchmarks i.e. establishing
TCP connections locally have no cost. And if there is only one operation
happening at any time then h2 has to do more work hence more CPU usage.
With 500 parallel requests going on at the same time, h2 becomes faster
than (or is at the same level as) h1 while using a minimal number of TCP
connections. Thus, it will work much faster in real life situations. So,
let's enable it.
Signed-off-by: Giedrius Statkevičius [email protected]