Conversation
a461b44 to
5dc45d8
Compare
5dc45d8 to
3537a70
Compare
lib/dalli/socket.rb
Outdated
|
|
||
| # Returns true if host is an IP address (v4 or v6) rather than a hostname. | ||
| def self.ip_address?(host) | ||
| host.match?(/\A\d{1,3}(\.\d{1,3}){3}\z/) || host.include?(':') |
There was a problem hiding this comment.
So Resolv::IPv4 and Ipv6 have Regexes, might be useful?
host.match?(Resolv::IPv4::Regex) || host.match?(Resolv::IPv6::Regex)There was a problem hiding this comment.
Good idea
host.include?(':') already captures IPv6 format, but is looser and will capture edge cases that Resolv::IPv6::Regex will miss (e.g. a scoped address like fe80::1%eth0)
Gonna move forward with
host.match?(Resolv::IPv4::Regex) || host.include?(':')
| sock.options = { path: path }.merge(options) | ||
| sock.server = server | ||
| sock | ||
| rescue |
There was a problem hiding this comment.
should we rescue => e so that we at least only match StdErrors? this sounds too catch-allish
There was a problem hiding this comment.
iirc, rescue and rescue => e are the same, with the latter capturing the exception to a var e
|
|
||
| dns = Resolv::DNS.new | ||
| dns.timeouts = timeout | ||
| resolver = Resolv.new([Resolv::Hosts.new, dns]) |
There was a problem hiding this comment.
idk how costly it is but i generally like the idea of caching resolved hosts for a while instead of resolving on every conn
There was a problem hiding this comment.
At Braze, we shouldn't ever get down to this line because we use IP addresses to connect to memcached and will return on the guard clause. Also connection creation is not a hot path, so optimizing here is not so important.
Also also, this bit from Claude: if we were to use DNS, there is a risk of caching and using stale hosts if K8s should rotate DNS records to redirect traffic. Leaving as-is for now
radixdev
left a comment
There was a problem hiding this comment.
wouldn't call myself an expert on this, but lgtm
This PR reduces Global VM Lock (GVL) contention in the Dalli memcached client by switching socket operations to non-blocking I/O with explicit
IO.selectwaits. When the socket would have previously blocked, the thread now yields the GVL instead of holding it across blocking syscalls, improving concurrency under load.More details involving the changes, motivation, and benchmarking results provided by @radixdev - https://gist.github.com/radixdev/d601cf8a9676d10774463b63748fffe6
benchmark_async.rb
Benchmark Results
Benchmarked on:
Latency injected via a forked TCP proxy process (separate GVL) that adds a
configurable
sleepper data relay in each direction.Localhost (no added latency)
On localhost, the kernel socket buffer absorbs writes instantly and
IO.selectinreadfullalready releases the GVL during reads. The asyncpath adds a small extra-syscall overhead that shows up at high thread counts
with small payloads. Performance is effectively equivalent.
1ms RTT (same-rack)
4ms RTT (same-datacenter)
At realistic datacenter latency, the async approach shows consistent 5–9%
improvements in multi-threaded throughput. The fixed overhead of the extra
non-blocking syscall becomes negligible relative to the per-operation network
time.
10ms RTT (cross-datacenter)
At high latency, operations are dominated by network round-trip time. Both
approaches spend nearly all time in
IO.select(which releases the GVLregardless), so throughput converges.