parfait icon indicating copy to clipboard operation
parfait copied to clipboard

fix memory leak

Open pwinckles opened this issue 1 year ago • 4 comments

perMetricByteBuffers leaks both its keys and its values. The leak happens as follows:

  1. A new metric is added.
  2. Skipping some plumbing, PcpMmvWriter.stop() is called, which clears PcpMmvWriter.metricData, which is a map of PcpValueInfo objects that describe all of the metrics.
  3. All of the metrics are re-added with calls to PcpMmvWriter.addMetricInfo(), which creates new PcpValueInfo instances to repoplulate PcpMmvWriter.metricData.
  4. PcpMmvWriter.start() is called, which creates a new ByteBuffer, and populates slices of the buffer into PcpMmvWriter.perMetricByteBuffers. This map is keyed off the new PcpValueInfo instances, 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.

pwinckles avatar Jul 29 '24 17:07 pwinckles

An alternative, simpler, solution would be to just call perMetricByteBuffers.clear() in PcpMmvWriter.reset(), though I have not run this solution.

pwinckles avatar Jul 29 '24 21:07 pwinckles

@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 avatar Aug 03 '24 02:08 natoscott

@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 avatar Aug 05 '24 19:08 pwinckles

@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.

natoscott avatar Aug 05 '24 22:08 natoscott

Thanks!

I did a force push on this branch to change it to the clear() solution.

pwinckles avatar Aug 06 '24 00:08 pwinckles

@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 avatar Aug 19 '24 14:08 pwinckles

@pwinckles I think since noone's commented by now, go ahead and merge.

natoscott avatar Aug 19 '24 23:08 natoscott