guelfey
commented
1 year ago I see 3 separate things here:
IsValidhas no need to actually use a real buffer, but can just useDiscard: easy fix.IsValidneeds to perform a full encode to check for validity: not easy to work around since e.g. the maximum message size limit can't really be checked without just trying to encode the message. You could do some optimizations here to e.g. avoid copying strings and just adding their length, but not sure if that's worth the complexity.- But: indeed basically all uses of
IsValidfrom inside the package are unnecessary because they're either followed directly by a "real" encode (which just needs some tweaked error handling to have the same effect), or they only really check a subset of the message after decoding (see #353). I will address the first part separately.
I was tracking down a memory leak in an app of mine and, thanks to pprof, I found that calls to
(*Message).IsValid()are both expensive and very allocation heavy. It's not documented, but the method performs an entire encoding of the message to a throwaway buffer and immediately just discards the result, only returning any errors that it encounters.Could this be feasibly changed? Is the full encode to find errors really necessary? Couldn't the same code that finds those errors be refactored out so that only it can be called without needed to actually perform an encoding?
Or, if that's not possible, could the buffer size be estimated and pre-allocated, and maybe even use a globalWhat am I talking about... Why not just write tosync.Poolto reduce the allocations from multiple calls?io.Discardand avoid the problem completely? The data's being thrown away anyways.As it stands, the path of the code is such that
IsValid()is called followed immediately by a call to(*Conn).SendMessage(), meaning that every message is being encoded twice, with one of those being into a buffer that's just immediately thrown away.