Repository navigation
Conversation
cc52d70 to
c92e625
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR optimizes packet transmission by grouping packets by endpoint before sending, reducing the number of send operations while maintaining packet ordering within each endpoint group.
- Introduced
partitionBuffersByEndpointfunction to group packets by destination endpoint while preserving stable ordering within groups - Refactored
SendBuffersto leverage pre-partitioned, contiguous same-endpoint buffer sequences for more efficient batch sending - Fixed resource leak by properly releasing TCElement objects after transferring their contents to outbound elements
Reviewed Changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| polyamide/device/traffic_control.go | Fixed import ordering per Go conventions and added proper cleanup of TCElement resources |
| polyamide/device/send.go | Introduced endpoint partitioning logic to group packets by destination before transmission |
| polyamide/device/peer.go | Optimized SendBuffers to send contiguous same-endpoint packets in batches, replacing map-based grouping |
| .gitignore | Added workdir to ignore list |
| .dockerignore | Added workdir and Dockerfile to ignore list |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if err != nil { | ||
| device.Log.Errorf("%v - Failed to send data packets: %v", peer, err) | ||
| continue | ||
| } |
There was a problem hiding this comment.
The lock acquired at line 526 via elemsContainer.Lock() is not released before the continue statement when partitionBuffersByEndpoint returns an error. This will cause a deadlock. The unlock should happen before continuing, or the cleanup code (lines 547-551) should be executed even on error.
| // If endpoint is nil, it has already been processed | ||
| // as part of an earlier group. |
There was a problem hiding this comment.
The comment "If endpoint is nil, it has already been processed as part of an earlier group" is misleading. At this point in the code, all originally-nil endpoints have been replaced with peer.endpoints.val[0] in the previous loop (lines 451-459). The nil check here is only catching entries that were marked as visited (set to nil) during the current partitioning loop, not originally-nil entries.
| // If endpoint is nil, it has already been processed | |
| // as part of an earlier group. | |
| // If endpoint is nil, it was marked as visited during this loop | |
| // and has already been processed as part of an earlier group. |
| } | ||
| // optimization, if multiple contiguous buffers share the same endpoint, send them in a single batch | ||
| prevIdx := 0 | ||
| prevEp := eps[0] |
There was a problem hiding this comment.
Potential panic if eps slice is empty. Line 149 accesses eps[0] without checking if the slice has any elements. While the current code flow may prevent empty slices from reaching this function, defensive programming would suggest adding a length check or early return for empty inputs.
This PR properly frees TCElement in the hot path, as well as reducing map allocations in the endpoint-based grouping logic.