Repository navigation
Cut TimeoutRecoveryReader CPU and syscalls for get_multi of small values - #88
Conversation
- Pass binary separators to byteindex so it no longer scans the buffer's coderange after every fill. - Append with String#append_as_bytes (Ruby 3.4+), which skips the coderange scan String#<< does, and copy the buffered start of a value with unpack1 so the buffer never shares its memory with a value and gets copied on the next read. - Grow buffer reads from 8 KiB to 64 KiB once a reply outgrows 8 KiB, and read large values through the same buffer instead of a separate 64 KiB scratch string. - Wait before a reply's first read on plain sockets instead of reading, getting :wait_readable, and then waiting. - Add a 40 x 4 KB get_multi to the async_get benchmark target.
mrattle
left a comment
mrattle
left a comment
There was a problem hiding this comment.
This looks good to me and will be a performance improvement as-is, but my robot friend flagged a bit more juice that you could squeeze here. Might be worth checking against your benchmarks before finalizing this.
| data = String.new(capacity: count, encoding: Encoding::BINARY) | ||
| data << @buffer.byteslice(@offset, available) if available.positive? | ||
| # A byteslice that runs to the end of @buffer shares its memory, and the next fill would then | ||
| # copy the whole buffer before overwriting it. unpack1 copies just the bytes asked for. |
There was a problem hiding this comment.
Robot suggestion 1:
A value that ends exactly at the buffer's end shares the 64 KiB buffer.
consume → byteslice shares memory with @buffer whenever the slice runs to the buffer's end. Two things follow:
- The returned value keeps the whole 64 KiB buffer alive for as long as the caller holds it.
- The next
read_nonblockinto@bufferfirst copies the whole buffer.
This already happens on main, but with an 8 KiB buffer. This PR makes it 64 KiB.
Measured on Ruby 4.0.7 with 6,400 raw values kept, 400 of which ended on a read boundary. RSS grew by 56 MiB with this PR's existing reader and by 27 MiB with the fix. On a reply built so that every read ends at a value's end, the fix was 12.9% faster, and it made no measurable difference on get_multi of 40 × 4 KB or 400 × 100 B.
Using > sends the exact-end case through the existing unpack1 copy path. It adds no work on the common path:
def read(count)
available = @buffer.bytesize - @offset
- return consume(count) if available >= count
+ # A slice that runs to the end of @buffer shares it; see below.
+ return consume(count) if available > count
data = String.new(capacity: count, encoding: Encoding::BINARY)The comment at :56-57 already explains why, so it could move up here instead of adding a new one.
There was a problem hiding this comment.
Applied in 084e7d1, in consume rather than with >: when a slice would run to the end of @buffer, consume returns tail, which copies it with unpack1 and consumes it. That covers read and gets, and copies the value once instead of through a temp and then into a presized string. read uses the same tail for a value's buffered start, and the comment moved there.
| def fill | ||
| partial = @buffer.byteslice(@offset, @buffer.bytesize - @offset) if pending? | ||
| filled = !read_partial(CHUNK_SIZE, @buffer).nil? | ||
| filled = !read_partial(@read_size, @buffer).nil? | ||
| @read_size = LARGE_READ_SIZE if @buffer.bytesize == @read_size | ||
| @buffer.clear unless filled | ||
| @offset = 0 | ||
| @buffer.prepend(partial) if partial | ||
| filled | ||
| end |
There was a problem hiding this comment.
Robot suggestion 2:
The leftover partial line makes the next read copy the whole buffer.
When a line is split across a read, fill saves the leftover with a slice that runs to the buffer's end. For leftovers of about 20 bytes or more, that slice shares @buffer. read_nonblock then allocates and copies up to 64 KiB before reading into the buffer. This is the same str_make_independent_expand cost the PR removes from read. With long keys, header lines are long enough to hit it.
A probe showed the buffer shared before 4 of 5 reads with this PR's code, and before 0 of 5 with the fix. On a reply built so that every read splits a header line, the fix was 15.6% faster, with no difference on ordinary replies.
def fill
- partial = @buffer.byteslice(@offset, @buffer.bytesize - @offset) if pending?
+ partial = @buffer.unpack1('a*', offset: @offset) if pending?
filled = !read_partial(@read_size, @buffer).nil?The comment above fill (:109-111) could say why the copy is needed, for example: "unpack1 copies the leftover; a byteslice running to the end would share @buffer and make the read copy all of it."
There was a problem hiding this comment.
Applied in 084e7d1: fill copies the leftover with unpack1, and the comment above fill now points at tail for why.
| fails after `socket_timeout`. Writes are unchanged, and so is the pipelined `get_multi` path used with multiple | ||
| servers or a block. The buffer is about 8 KiB per connection, is allocated only when a request on that connection | ||
| opts in, and is released when the connection closes. The `exp_` prefix marks the option as experimental. | ||
| servers or a block. The buffer is 8 KiB per connection and grows to 64 KiB once a reply on that connection |
There was a problem hiding this comment.
The buffer is 8 KiB per connection and grows to 64 KiB
The existing change can apparently keep the 64KiB buffer alive indefinitely (which isn't a huge deal) but this becomes accurate if robot suggestion 1 is applied.
There was a problem hiding this comment.
Agreed. With 084e7d1 no returned string shares the buffer, so the README's "grows to 64 KiB" holds. Left the text as is.
| @@ -548,12 +537,11 @@ def run_ops(test_case) | |||
| return :stalled | |||
| end | |||
There was a problem hiding this comment.
The fuzz test never calls expect_reply. Each fuzz case uses one reader for all its ops, so the new wait-before-read path only runs on the first op of each case. When the buffer is empty, randomly marking a reply boundary would put that path under the slow-scheduler and spurious-wakeup faults on every simulated request.
There was a problem hiding this comment.
Added in 084e7d1: before each op, with nothing buffered, the fuzz calls expect_reply half the time (STATS[:expected_replies], ~830 per 24-iteration run). The wait before the first read now runs under the starved waits, spurious EAGAINs, and stalls. 3 × FUZZ_ITERATIONS=24 runs pass on Linux.
There was a problem hiding this comment.
Suggested regression test for robot suggestion 1:
it 'returns a value that ends at the end of a read without sharing the buffer' do
with_reader do |reader, peer|
peer.write("VA 1000 k\r\n", 'v' * 1000)
sleep(0.01)
assert_equal "VA 1000 k\r\n", reader.gets("\r\n")
value = reader.read(1000)
peer.write("\r\n")
assert_equal "\r\n", reader.read(2)
refute_includes ObjectSpace.dump(value), '"shared":true'
end
endThere was a problem hiding this comment.
Added as is in 084e7d1. It fails on the previous commit ("shared":true) and passes now.
- Values and leftovers that run to the end of the buffer are copied with unpack1, so they neither keep the 64 KiB buffer alive nor make the next read copy all of it. - The fuzz test marks a new reply at random when nothing is buffered, so the wait before a reply's first read runs under the injected faults.
|
Thanks @mrattle. Applied all five in 084e7d1. Benchmarked against the previous commit: median of 3 alternating runs, Linux, Ruby 3.4 + YJIT, glibc, UNIX socket, recovery path, CPU µs per op.
The 40 × 4 KB |
2 participants
Problem
With
exp_async_timeout_recovery, aget_multiof many small values costs more CPU than the default read path. #87 put this down to Ruby-level line parsing. Theasync_getbenchmark target shows it over TCP (40 × 4 KB, Ruby 3.4, YJIT, underSync):mainThe
perfprofile points elsewhere. Without YJIT, for 40 × 12 KB values, three costs grow with the bytes read, not with the number of values:coderange_scan, 7.2% of samples. 3.9% comes frombyteindex.ConnectionManagerpasses UTF-8 separator literals and the buffer is binary, sorb_enc_checkscans the whole buffer's coderange after every fill. The other 3.3% comes fromString#<<, which scans each appended chunk.str_make_independent_expand, 3.2% (memcpy).data << @buffer.byteslice(@offset, available)takes a slice that runs to the end of@buffer. Such a slice shares@buffer's memory, so the nextread_nonblockinto@bufferfirst copies the whole buffer.A reply's first read also always returns
:wait_readable, because it comes straight after the request's write. A 1 KB get therefore takes 4 syscalls (recvfrom,ppoll,recvfrom,write) where the default path takes 3.Change
ConnectionManagerpassesTimeoutRecoveryReader::LFand::CRLF, which are binary, sobyteindexskips the scan.appendusesString#append_as_byteson Ruby 3.4+, which skips the coderange scan and keeps the capacity the value string was created with. Older Rubies keep<<. The buffered start of a value is copied withunpack1('a*', offset:), which copies only those bytes and never shares@buffer. I tried the five-argumentbytesplicefirst. It avoids both costs too, but it reallocates the destination to its exact new size on every call, which threw away the presized capacity and made large gets ~25% slower.ConnectionManager#start_request!callsreader.expect_replyfor flagged requests. On plain sockets the first read of the reply waits first, through the samewait_for(a scheduler wait with the socket timeout, then the zero-timeout poll). TLS sockets still read first, because OpenSSL can hold decrypted bytes that the socket's readiness doesn't show. Later reads in a reply still read first.Large values now land in the buffer, so a read can take bytes past the end of its value: the trailing
\r\nand the next lines of the same reply.pending?still counts exactly the bytes taken but not returned. A reply can't contain bytes past its own end, sofinish_request!'s check is unaffected. I dropped the fuzz test's "large read took bytes past its end" assertion and its scratch-size check, and kept thepending?consistency check.Results
Linux arm64 (Docker on Apple Silicon), Ruby 3.4.11 with YJIT, glibc malloc, memcached 1.6.38 on a UNIX socket, async 2.46 / io-event 1.22 with EPoll. Raw values,
socket_timeout: 0.05. Each figure is the median of 3 runs that alternate betweenmainand this branch, and each run is the median of 3 × 3000 iterations.CPU µs per op (wall µs in parentheses):
mainUnder
Sync(EPoll):mainSyscalls per op, counted with
perf stat -e raw_syscalls:sys_enter(2000 iterations minus 0, divided by 2000). I didn't usestracehere: it slows the client enough that replies are always waiting, which hides the read that returns:wait_readable.mainSync: default /main/ this PRBENCH_TARGET=async_get bin/benchmark(TCP, now with a 40 × 4 KBget_multi):mainmainCaveats:
unpack1). What differs is the allocation pattern around the per-value string, and so how often glibc trims and regrows its heap. A loop of 512 KB gets underSynccosts this branch ~10–20% more CPU thanmainwith glibc's defaults (~40 page faults per get against ~9). WithMALLOC_TRIM_THRESHOLD_=268435456it costs ~35% less (maingoes to ~128 faults per get). Under jemalloc the two are within ±3% for 512 KB and 2 MB. Theget_multigains hold under both glibc and jemalloc (−17% to −24% CPU at 40 × 4 KB under jemalloc).get_multiof 40 × 3 KB / 40 × 12 KB costs 15% / 27% less CPU thanmain. A 1 KB get costs ~1.5 µs more (10 µs against 8.5 µs), although it makes one syscall fewer. I couldn't trace syscalls there to find out why.<<'s coderange scan, becauseappend_as_bytesdoesn't exist there. They get the other changes.Tests
mainwith 21.expect_reply, the first read of a delayed reply never returns:wait_readable. This fails onmain.LARGE_READ_SIZEafter a 1 MB read. This replaces the 8 KiB bound.pending?consistency check.Runs:
test_rack_session.rbwas excluded becausecgi/cookieis gone in Ruby 4.0.FUZZ_ITERATIONS=24under EPoll and URing.<<fallback): reader, fuzz, recovery integration, andConnectionManagertests pass.