fix memory leak
perMetricByteBuffers leaks both its keys and its values. The leak happens as follows:
- A new metric is added.
- Skipping some plumbing,
PcpMmvWriter.stop()is called, which clearsPcpMmvWriter.metricData, which is a map ofPcpValueInfoobjects that describe all of the metrics. - All of the metrics are re-added with calls to
PcpMmvWriter.addMetricInfo(), which creates newPcpValueInfoinstances to repoplulatePcpMmvWriter.metricData. PcpMmvWriter.start()is called, which creates a newByteBuffer, and populates slices of the buffer intoPcpMmvWriter.perMetricByteBuffers. This map is keyed off the newPcpValueInfoinstances, and this is where it's leaking.
Because PcpValueInfo does not implement hashCode(), old entries in perMetricByteBuffers are never removed. This leaks both the PcpValueInfo keys as well as the ByteBuffer.
The fix here implements PcpValueInfo.hashCode(), based only on the metric name, and ensures that the metric's old entry is explicitely removed from perMetricByteBuffers. This is necessary, rather than just relying on put() to overwrite, because the PcpValueInfo objects are different, and we want the old object to be GC'd.
An alternative, simpler, solution would be to just call perMetricByteBuffers.clear() in PcpMmvWriter.reset(), though I have not run this solution.
@pwinckles I haven't heard from any of the Parfait developers recently, but let's give them a little more time. If we don't get a response though would you be interested in becoming a direct contributor to this git repo?
@natoscott
If we don't get a response though would you be interested in becoming a direct contributor to this git repo?
What does that entail?
I tested the alternative solution that simply clears perMetricByteBuffers, and it works. I'll update the PR later.
@pwinckles I'll send you an invite to join the pcp github org - should be an email arriving shortly. There's a #parfait team on PCP slack channel I encourage you to join - see https://pcp.io/community.html - it's very quiet but some of the original Parfait developers may still be lurking there if you have further questions.
Thanks!
I did a force push on this branch to change it to the clear() solution.
@natoscott I'm not in a particular rush or anything, as we're already using a forked version, but, since you added me to the github group, I'm uncertain as to what your process expectations are here. Shall I just ping the slack and wait and see for a few more weeks?
@pwinckles I think since noone's commented by now, go ahead and merge.