Use sync.Pool for gzipWriter #481

glefloch · 2018-10-17T12:04:44Z

This PR aims to close issue #366 by adding a sync.Pool for gzipWriter.
As mentionned by @dswarbrick, the NYTime gzipHandler did not improve performance compared to a sync.Pool.

Signed-off-by: glefloch glfloch@gmail.com

Signed-off-by: glefloch <glfloch@gmail.com>

dswarbrick · 2018-10-17T12:17:24Z

Is there any performance (or GC) difference having the gzipHandler as an anonymous function, vs. declared normally?

beorn7 · 2018-10-17T12:48:59Z

prometheus/promhttp/http.go

-		defer giveBuf(buf)
-		writer, encoding := decorateWriter(req, buf, opts.DisableCompression)
-		enc := expfmt.NewEncoder(writer, contentType)
+		buf := &bytes.Buffer{}


Why did you remove the sync.Pool for buf? This will create a lot of allocations in high-scrape-rate scenarios.

Or to put it another way: For the same reason we want the gzip writer in a sync.Pool, we want this intermediate buffer in a sync.Pool.

beorn7 · 2018-10-17T13:58:53Z

prometheus/promhttp/http.go

 		if lastErr != nil && buf.Len() == 0 {
 			http.Error(w, "No metrics encoded, last error:\n\n"+lastErr.Error(), http.StatusInternalServerError)
 			return
 		}
-		header := w.Header()
-		header.Set(contentTypeHeader, string(contentType))
-		header.Set(contentLengthHeader, fmt.Sprint(buf.Len()))


This commit removes this header (which SHOULD be used as per https://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.13 ). That's probably OK for gzip'd case because we avoid another buffering layer, but it could still be set in the cases where we don't compress. This is complicated here because of the double-wrapping, see comment below.

beorn7 · 2018-10-17T13:59:37Z

prometheus/promhttp/http.go

 		if _, err := w.Write(buf.Bytes()); err != nil && opts.ErrorLog != nil {
 			opts.ErrorLog.Println("error while sending encoded metrics:", err)
 		}
 		// TODO(beorn7): Consider streaming serving of metrics.
 	})

+	gzipHandler := http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {


It is helpful to have two http.HandlerFuncs here where one just wraps the other? Couldn't this just be inlined?

Nested handlers is the normal way to implement gzip middleware in Go, especially if you want it to be optional / settable by some flag.

But this is not really nesting of handlers. The code is all enclosed in the very large HandlerFor function. And we always go through the local gzipHandler function, even if compression is disabled. We kind of get the worst of both worlds: No access to the content length anymore, but still no modularization.

I'm not a fan of it being an anonymous function either, as I suspect this will cause a lot of allocations / GC. The NYTimes implementation is worth studying. And bear in mind that the content-length header should be the length of the gzipped output, not the original input.

WRT content length: Yes, I had that in mind. That's why I suggested it's probably OK to not add a content length when gzipping. But we should (or even SHOULD :-) still add the content length when not gzipping. And that's not possible with the approach here where the decision is made within the gzipHandler local function but the uncompressed content length is only available in the h local function. In this specific case, where we don't want to provide a generically usable gzip handler, I'd probably just inline everything into one local function. At least for now. We can still refactor into something nicer once we have something that works.

beorn7 · 2018-10-17T14:16:00Z

Thanks for doing this. The idea to gzip the completely rendered response instead of the decorated writer approach so far looks quite neat.

I left a few comments we need to clarify.

glefloch · 2018-10-18T06:37:00Z

@beorn7 Thanks for the comment, I will update the code.

Signed-off-by: glefloch <glfloch@gmail.com>

beorn7

I think this looks good in general. Just a few minor comments, which don't change anything fundamentally.
(With the exception of the content-length-header...)

beorn7 · 2018-10-19T13:02:22Z

prometheus/promhttp/http.go

+}
+
+// gzipHandler return a http.HandlerFunc in charge of compressing the content
+// of the given http.HandlerFunc


Doc comment is outdated now.

beorn7 · 2018-10-19T13:04:44Z

prometheus/promhttp/http.go

-		if closer, ok := writer.(io.Closer); ok {
-			closer.Close()
-		}
+
 		if lastErr != nil && buf.Len() == 0 {
 			http.Error(w, "No metrics encoded, last error:\n\n"+lastErr.Error(), http.StatusInternalServerError)
 			return
 		}
 		header := w.Header()
 		header.Set(contentTypeHeader, string(contentType))
 		header.Set(contentLengthHeader, fmt.Sprint(buf.Len()))


Setting the content length must only happen if we do not gzip-compress. See suggestion below.

beorn7 · 2018-10-19T13:05:38Z

prometheus/promhttp/http.go

+		buf.Reset()
+		enc := expfmt.NewEncoder(buf, contentType)
+
+		defer bufPool.Put(buf)


Is there a reason to not do this right after the bugPool.Get() above?

beorn7 · 2018-10-19T13:22:10Z

prometheus/promhttp/http.go

 		}
+		writeResult(w, buf, opts)


I think now that this is inlined, we can even drop the gzipResponseWriter and the writeResult. Suggestion for everything from L163 to here:

var writer io.Writer = w if !opts.DisableCompression && gzipAccepted(req.Header) { header.Set(contentEncodingHeader, "gzip") gz := gzipPool.Get().(*gzip.Writer) defer gzipPool.Put(gz) gz.Reset(w) defer gz.Close() writer = gz } else { header.Set(contentLengthHeader, fmt.Sprint(buf.Len())) } if _, err := writer.Write(buf.Bytes()); err != nil && opts.ErrorLog != nil { opts.ErrorLog.Println("error while sending encoded metrics:", err) }

Would that work?

For nicer naming, perhaps then rename w into hw and writer into w.

beorn7 · 2018-10-19T22:20:04Z

Hmm, thinking about the need (or not-need) of content length, the changes proposed in your PR, and the current performance testing I'm doing for kube-state-metrics, I'm wondering if we should perhaps ditch that intermediate buffer and do streaming exposition, at least for the encoding part.

I'll play with it and create a PR based on yours.

beorn7 · 2018-10-19T23:30:34Z

See #482

Use sync.Pool for gzipWriter

fdf4cbc

Signed-off-by: glefloch <glfloch@gmail.com>

beorn7 reviewed Oct 17, 2018

View reviewed changes

Fix PR comments

c2c6fd2

Signed-off-by: glefloch <glfloch@gmail.com>

beorn7 requested changes Oct 19, 2018

View reviewed changes

glefloch closed this Oct 22, 2018

glefloch deleted the use-gzip-pool branch October 22, 2018 09:44

beorn7 mentioned this pull request Oct 22, 2018

Use gzip from a pool and stream while encoding metrics #482

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use sync.Pool for gzipWriter #481

Use sync.Pool for gzipWriter #481

glefloch commented Oct 17, 2018

dswarbrick commented Oct 17, 2018

beorn7 Oct 17, 2018

beorn7 Oct 17, 2018

beorn7 Oct 17, 2018

beorn7 Oct 17, 2018

dswarbrick Oct 17, 2018

beorn7 Oct 17, 2018

dswarbrick Oct 17, 2018

beorn7 Oct 17, 2018

beorn7 commented Oct 17, 2018

glefloch commented Oct 18, 2018

beorn7 left a comment •

edited

Loading

beorn7 Oct 19, 2018

beorn7 Oct 19, 2018

beorn7 Oct 19, 2018

beorn7 Oct 19, 2018

beorn7 commented Oct 19, 2018

beorn7 commented Oct 19, 2018

Use sync.Pool for gzipWriter #481

Use sync.Pool for gzipWriter #481

Conversation

glefloch commented Oct 17, 2018

dswarbrick commented Oct 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beorn7 commented Oct 17, 2018

glefloch commented Oct 18, 2018

beorn7 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beorn7 commented Oct 19, 2018

beorn7 commented Oct 19, 2018

beorn7 left a comment •

edited

Loading