MemStore Copying Message Content Necessary?

[stefanLeo]

I just saw that in memstore.go's storeRawMsg NATS is always executing a byte copy of the msg payload

// Make copies
	// TODO(dlc) - Maybe be smarter here.
	if len(msg) > 0 {
		msg = copyBytes(msg)
	}
	if len(hdr) > 0 {
		hdr = copyBytes(hdr)
	}

My question is: Is this really needed? It won't generate a direct performance impact, but will increase GC load imho.
While I think I understood the basic idea behind it - I also could not find anything tampering with the content of msg in the calling code. I also think this should anyhow be discouraged for the broker?
It also comes with the "todo" on top which I could not make too much sense of as either copying is requried or not, but doing it smarter?

Any feedback on that is highly appreciated.

[neilalexander]

I think the answer is to make sure that the msg or hdr are not causing us to hold onto a larger underlying array which belongs to the client buffer pools which would get recycled and reused, causing a data race.

The first path via processInboundJetStreamMsg has already copied both msg and hdr so the memory store is probably doing it a second time unnecessarily in that particular case.

However, the second path via processCatchupMsg hasn't, so maybe that copy wants to be moved from the memstore out to processCatchupMsg or processSnapshot. That way both paths have only copy msg and hdr once.