Can't share buffers like that! #484
Conversation
| type Buffer struct { | ||
| type Buffer interface { | ||
| io.Reader | ||
| io.Writer |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
| func badRequest(r *http.Request, w http.ResponseWriter, err error) { | ||
| http.Error(w, err.Error(), http.StatusBadRequest) | ||
| log.Printf("Error procressing request for %s: %v", r.URL.Path, err) | ||
| } |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
|
@tomwilkie in #470 I thought I understood what you were doing with the buffers but after reading this PR a couple times it's clear that I don't. Specifically, I don't understand why multiple report publishers are sharing the same buffer instance, nor the need for the manual reference counting. Can you walk me through the "why" part of this change (I understand the ultimate goal is to reduce GC pressure)? Happy to Skype if that would be easier. |
|
Or @paulbellamy if you've got a handle on it happy to take a lesson from you... |
|
The whole point of the original change was to only serialise and compress the report once per publish cycle, instead of once-per-upstream-app-per-publish cycle. This accounted for about 1/4 of the probes CPU usage. We could have created a fresh buffer to serialise into, and then shared that buffer (via NewByteBuffers) with the publisher, but the next biggest hit for the probe was GC, and these buffers are large. So I introduce this mechanism for publishers to all read form the same buffer, and when they are done with it, return it to the pool, for reuse in the next cycle. The reference counting was needed as the publishers all publish in parallel, and can all take different lengths of time to publish (especially if the app is not reachable - it take a ~30s timeout to return the buffer). |
|
Closing in favour of #492 |
As part of #470 I introduces reference counted, shared buffers such that we only serialise and compress a report once. I got it a bit wrong: byte.Buffers contain a cursor, and therefore can only be used 'once'.
This change adapts that code such that a 'new' byte buffer is created for each report publisher, but they share the underlying []bytes, and use the reference counting to track the usage of that.
This manifested itself as a panic in the app as it couldn't decompress reports (as they were empty) - the panic was fixed in #481 without identifying the underly cause.
This change also adds error reporting on the app for why requests fail.
Fixes #471