Redis cache: use binary wire format with pooled buffers #473
Conversation
folbricht
left a comment
folbricht
left a comment
There was a problem hiding this comment.
There are a few inconsistencies and some unnecessary complexity in this. Since you mentioned you're not comfortable writing Go, would you prefer I make a PR against yours, or just take this code and fix it up?
cache-redis.go
Outdated
| if len(valueBytes) >= 1 && valueBytes[0] == '{' { | ||
| if jsonErr := json.Unmarshal(valueBytes, &a); jsonErr != nil { | ||
| Log.Error("failed to decode cache record from redis (binary and JSON)", "binary_error", err, "json_error", jsonErr) | ||
| return nil, false, false | ||
| } | ||
| } else { | ||
| // Try JSON anyway | ||
| if jsonErr := json.Unmarshal(valueBytes, &a); jsonErr != nil { | ||
| Log.Error("failed to decode cache record from redis", "error", err) | ||
| return nil, false, false | ||
| } | ||
| } |
There was a problem hiding this comment.
The conditional here looks unnecessary, it'll unmarshal with JSON regardless, if it starts with { or not.
There was a problem hiding this comment.
I have removed it. After removing, a lot of these error messages do appear in the log. Even after I have flushed the Redis DB.
level=ERROR msg="failed to decode cache record from redis" binary_error="failed to unpack DNS message: bad question name: dns: bad rdata" json_error="invalid character '\\x01' looking for beginning of value"
There was a problem hiding this comment.
@emlimap Do you still see this issue in the latest version? The error suggests the binary decode is failing (the json failure is just it trying to decode the binary format so that's expected to fail, shouldn't even try that)
cache-redis.go
Outdated
| var _ CacheBackend = (*redisBackend)(nil) | ||
|
|
||
| // Buffer pool for dns.Msg.PackBuffer to minimize allocations. | ||
| // Pool stores []byte directly (not pointers) and returns the grown buffer |
There was a problem hiding this comment.
Using a pointer is actually slightly more efficient, though that isn't really that important.
cache-redis.go
Outdated
| binaryFormatVersion = 1 | ||
| headerSize = 10 | ||
| flagPrefetchBit = 1 << 0 | ||
| maxPooledBufCap = 32 << 10 // 32 KiB - cap pooled buffers to prevent unbounded growth |
There was a problem hiding this comment.
Not sure the limiter is necessary
cache-redis.go
Outdated
| packed, err := item.Msg.PackBuffer(buf) | ||
| if err != nil { | ||
| // Return buffer to pool even on error | ||
| packBufPool.Put(buf[:0]) |
There was a problem hiding this comment.
Using defer is more common, no need to cover every code-path that returns
cache-redis.go
Outdated
| } | ||
|
|
||
| // Allocate result buffer: header (10 bytes) + packed message | ||
| result := make([]byte, headerSize+len(packed)) |
There was a problem hiding this comment.
Basically all of the advantages of using the pool are wasted here since this allocates a new buffer, then later copies the whole message into it.
|
@folbricht I have got AI to make changes based on your comment. Please feel free to make your own PR or take this code and fix it. |
|
@folbricht The failed to decode messages was because the format being written to Redis was corrupted. This is now fixed. Got Claude code to take your PR comments into account when making the changes. It also added tests for encoding and decoding. Not sure if it is useful for you. If not, feel free to remove it. |
|
Thank you. The handling of the buffer pool can definitely be improved, so this could be made a little more efficient. I should be able to take care of that in January. |
2 participants
Store Redis cache entries in compact binary wire format to reduce payload size and CPU usage. Uses
dns.Msg.PackBufferwith adaptivesync.Poolto minimize allocations.Changes
Benefits