Fix memory leak of retained HTTP write payloads #7832
Conversation
This leak seems to have been introduced in 8aa224b, present in 1.1.0 and 1.1.1. When points were parsed from HTTP payloads, their tags and fields referred to subslices of the request body; if any tag set introduced a new series, then those tags then were stored in the in-memory series index objects, preventing the HTTP body from being garbage collected. If there were no new series in the payload, then the request body would be garbage collected as usual. Now, we clone the tags before we store them in the index. This is an imperfect fix because the Point still holds references to the original tags, and the Point's field iterator also refers to the payload buffer. However, the current write code path does not retain references to the Point or its fields; and this change will likely be obsoleted when TSI is introduced. This change likely fixes #7827, #7810, #7778, and perhaps others.
|
If anyone is running into this issue now and needs a workaround until this is merged or released, the best option is to keep the HTTP payload as small as possible for writes that introduce a new series. Writes that only affect existing series should not contribute to the leak. Once new series have been written, I believe the only way to release the memory of the associated HTTP payloads is to restart the server. |
|
If we're concerned about total allocations, this will create a lot of smaller allocations. We could instead just perform one large allocations for cloning |
|
@joelegasse I see what you mean. We have a couple options for a more efficient
Ultimately it probably would be best if |
| // If the tags were created by models.ParsePointsWithPrecision, | ||
| // they refer to subslices of the buffer containing line protocol. | ||
| // To ensure we don't refer to that buffer, preventing that buffer from being garbage collected, clone the tags. | ||
| tags = tags.Clone() |
There was a problem hiding this comment.
This is going to cause unnecessary allocations a lot of the time because CreateSeriesIndexIfNotExists only holds onto the subsliced tags if a new series needs to be created.
I think it would be better to push tags through to NewSeries as-is, and then clone the tags inside CreateSeriesIndexIfNotExists only when AddSeries is called. Maybe you could just do something like:
series.Tags = series.Tags.Clone()
m.AddSeries(series)| // | ||
| // Tags associated with a Point created by ParsePointsWithPrecision will hold references to the byte slice that was parsed. | ||
| // Use Clone to create Tags with new byte slices that do not refer to the argument to ParsePointsWithPrecision. | ||
| func (a Tags) Clone() Tags { |
There was a problem hiding this comment.
Just a suggestion: a is not at risk of holding onto anything, if we remove all the references it contains. Therefore I don't think we need to allocate a new Tags value here. We can just do a[i] = a[i].Clone() and it will probably make this method significantly faster.
Whether or not it's still appropriate to call this Clone with that implementation I'm not sure about. 😄
This change delays Tag cloning until a new series is found, and will only clone Tags acquired from `ParsePoints...` and not those referencing the mmap-ed files (TSM) that are created on startup.
joelegasse
force-pushed
the
mr-fix-http-write-memory-leak
branch
from
f6578af
to
cd00085
Compare
This leak seems to have been introduced in 8aa224b,
present in 1.1.0 and 1.1.1.
When points were parsed from HTTP payloads, their tags and fields
referred to subslices of the request body; if any tag set introduced a
new series, then those tags then were stored in the in-memory series
index objects, preventing the HTTP body from being garbage collected. If
there were no new series in the payload, then the request body would be
garbage collected as usual.
Now, we clone the tags before we store them in the index. This is an
imperfect fix because the Point still holds references to the original
tags, and the Point's field iterator also refers to the payload buffer.
However, the current write code path does not retain references to the
Point or its fields; and this change will likely be obsoleted when TSI
is introduced.
This change likely fixes #7827, #7810, #7778, and perhaps others.