do not copy ConnectionStats until we have to #28173
Conversation
brycekahle
added
changelog/no-changelog
team/networks
qa/done
| @@ -302,6 +302,17 @@ func (c ConnectionStats) IsExpired(now uint64, timeout uint64) bool { | |||
| return c.LastUpdateEpoch+timeout <= now | |||
| } | |||
|
|
|||
| // IsEmpty returns whether the connection has any statistics | |||
| func (c ConnectionStats) IsEmpty() bool { | |||
| // TODO why does this not include TCPEstablished and TCPClosed? | |||
There was a problem hiding this comment.
I have no objection with adding those fields here, do you @hmahmood?
There was a problem hiding this comment.
happy to do that in a different PR though
There was a problem hiding this comment.
actually we may set the closed flags on otherwise totally empty connections in kernelspace... so this could end up returning false a lot more
There was a problem hiding this comment.
This function was mostly checking for whether the connection exchanged any data, and almost exclusively for code that is storing closed connections in state.go. As opposed to something like IsZero which does check for those other two fields.
|
/merge |
|
🚂 MergeQueue: pull request added to the queue The median merge time in Use |
What does this PR do?
Uses
*ConnectionStatsarguments rather than forcing copies. Creates a copy only when storing for "long-term" usage (outside of the data pipeline for closed connections).Motivation
Reduced allocations, because the
ConnectionStatsobject is way too large to fit into a register.Additional Notes
Extracted from changes in #28040
Possible Drawbacks / Trade-offs
Describe how to test/QA your changes