perf: adjust read/write timeouts for redir #561
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Pull Request Overview
This PR optimizes network operations by implementing conditional deadline setting for read/write operations to reduce syscall overhead. The optimization only updates TCP connection deadlines when necessary, rather than on every operation.
- Adds read/write timeout constants and conditional deadline setting logic
- Implements deadline tracking with mutex protection to avoid unnecessary syscalls
- Resets deadline tracking state when connections are closed
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| pkg/wsman/client/wsman_tcp.go | Implements conditional deadline setting methods and integrates them into Send/Receive operations |
| pkg/wsman/client/wsman.go | Adds deadline tracking fields to the Target struct |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| now := time.Now() | ||
| // Only set deadline if it's been more than half the timeout period since last set | ||
| // or if it's never been set (zero value) | ||
| const deadlineRefreshInterval = defaultReadTimeout / 2 |
There was a problem hiding this comment.
The magic number division by 2 should be extracted to a named constant to improve code clarity and maintainability.
| const deadlineRefreshInterval = defaultReadTimeout / 2 | |
| const deadlineRefreshInterval = defaultReadTimeout / deadlineRefreshDivisor |
Copilot uses AI. Check for mistakes.
| now := time.Now() | ||
| // Only set deadline if it's been more than half the timeout period since last set | ||
| // or if it's never been set (zero value) | ||
| const deadlineRefreshInterval = defaultWriteTimeout / 2 |
There was a problem hiding this comment.
This duplicates the same magic number division by 2 from the read method. Consider extracting a shared constant like deadlineRefreshRatio = 0.5 to avoid duplication.
| const deadlineRefreshInterval = defaultWriteTimeout / 2 | |
| const deadlineRefreshInterval = time.Duration(float64(defaultWriteTimeout) * deadlineRefreshRatio) |
Copilot uses AI. Check for mistakes.
| } | ||
|
|
||
| // setReadDeadlineIfNeeded sets read deadline only when needed to reduce syscall overhead | ||
| func (t *Target) setReadDeadlineIfNeeded() error { |
There was a problem hiding this comment.
The deadline optimization could create a race condition where multiple goroutines might call Send/Receive simultaneously. Consider if the current mutex protection is sufficient for all concurrent access patterns.
Copilot uses AI. Check for mistakes.
| // or if it's never been set (zero value) | ||
| const deadlineRefreshInterval = defaultReadTimeout / 2 | ||
|
|
||
| if t.lastReadDeadlineSet.IsZero() || now.Sub(t.lastReadDeadlineSet) > deadlineRefreshInterval { |
There was a problem hiding this comment.
Calling time.Now() on every Send/Receive operation may impact performance. Consider caching the current time or using a less precise but faster time source for the deadline refresh logic.
Copilot uses AI. Check for mistakes.
No description provided.