Refactor receive logic to reuse message reading sequence #334
Conversation
| // receive will drain the receive buffer of the provided netlink connection | ||
| // and return all received messages, along with the first error encountered, | ||
| // if any. | ||
| func (cc *Conn) receive(conn *netlink.Conn) ([]netlink.Message, error) { |
There was a problem hiding this comment.
The idea is to make the transition from receiveAckAware gradual in the parts of the codebase that use it. Ideally, they would use the iterator directly when deserializing, which would help avoid creating intermediate slice copies and possibnly reduce memory usage especially for large dump responses.
That said, I’m not sure we’d actually see those memory savings in practice, since mdlayher/netlink seems to collect all dump messages into a single slice that’s received through one conn.Receive call.
conn.go
Outdated
| var firstError error | ||
|
|
||
| for msg, err := range cc.receiveSeq(conn) { | ||
| if err != nil && firstError == nil { |
There was a problem hiding this comment.
receiveAckAware returns the first receive error, flush joins all receive errors. Perhaps we could join them here too for consistency
There was a problem hiding this comment.
I’m in favor of consistency, but I wonder if we should go the other way and make Flush also only return the first error? I’m not sure how useful the non-first errors are…
There was a problem hiding this comment.
I agree, the first error should be enough and doesn't change anything on the caller side when handling.
conn.go
Outdated
| var firstError error | ||
|
|
||
| for msg, err := range cc.receiveSeq(conn) { | ||
| if err != nil && firstError == nil { |
There was a problem hiding this comment.
I’m in favor of consistency, but I wonder if we should go the other way and make Flush also only return the first error? I’m not sure how useful the non-first errors are…
Introduce a receiveSeq iterator to simplify reading messages from the
socket without copying them into a temporary slice. It yields only
messages relevant to nftables operations (i.e., those containing
nftables attributes).
Also refactor:
- receiveAckAware into a reusable receive method, which, like flush,
uses the same receiveSeq iterator for consistency.
- flush to return the first error during the receive sequence for
consistency with the receive method.
nickgarlis
force-pushed
the
refactor-receiveAckAware
branch
from
78e23cf to
7934eb0
Compare
|
I've now amended my changes so that |
Since google#330 and google#334 were merged, we no longer need to request an acknowledge for every message we send. Skipping ACK requests improves runtime and memory usage for large Flush batches, as it reduces the number of required `recvmsg` calls. The first error will still be reported via an acknowledge even if none are requested. This also matches the behavior of nft userspace.
Since google#330 and google#334 were merged, we no longer need to request an acknowledge for every message we send. Skipping ACK requests improves runtime and memory usage for large Flush batches, as it reduces the number of required `recvmsg` calls. The first error will still be reported via an acknowledge even if none are requested. This also matches the behavior of nft userspace.
Since #330 and #334 were merged, we no longer need to request an acknowledge for every message we send. Skipping ACK requests improves runtime and memory usage for large Flush batches, as it reduces the number of required `recvmsg` calls. The first error will still be reported via an acknowledge even if none are requested. This also matches the behavior of nft userspace.
2 participants
Introduce a
receiveSeqiterator to simplify reading messages from the socket without copying them into a temporary slice. It yields only messages relevant to nftables operations (i.e., those containing nftables attributes).Also refactor
receiveAckAwareinto a reusablereceivemethod, which, likeflush, uses the samereceiveSeqiterator for consistency.