Issue #22186 has been updated by himura467 (Akito Shitara). byroot (Jean Boussier) wrote in #note-6:
This isn't always a good reflection of performance in larger benchmark / real programs.
Agreed, I looked for real libraries with the same shape as the fluentd case. `Net::HTTPResponse#read_headers` is one: https://github.com/ruby/ruby/blob/master/lib/net/http/response.rb#L188 The source is not frozen, so as with fluentd each value that takes the shared path allocates a frozen root that copying avoids. How many values that reaches depends on real header lengths, so I collected response headers from five sites. The last two columns are how many of the shared values each cutoff would copy instead: | site | values | embedded on master | shared on master | copied at 128 | copied at 256 | | --- | ---: | ---: | ---: | ---: | ---: | | https://www.ruby-lang.org | 20 | 12 | 8 | 8 | 8 | | https://bugs.ruby-lang.org | 21 | 9 | 12 | 8 | 11 | | https://github.com | 18 | 6 | 12 | 7 | 10 | | https://www.wikipedia.org | 22 | 7 | 15 | 12 | 15 | | https://www.cloudflare.com | 28 | 10 | 18 | 9 | 11 | | total | 109 | 44 | 65 | 44 | 55 | A 128 cutoff covers 44 of the 65 shared values and a 256 cutoff covers 55. The 11 in between include six `Set-Cookie`. Falcon is a case that gets nothing. `protocol-http1` is pure Ruby and the values do move from shared to embedded, but the allocation count is unchanged, because it parses with a regex: https://github.com/socketry/protocol-http1/blob/v0.39.0/lib/protocol/http1/c... `line.match` already allocates a frozen copy of the line for the MatchData, so the source is frozen by the time the capture is sliced and sharing costs one allocation, the same as copying. The saving here is the frozen root, and regex parsing has already paid for it. So the benefit tracks the parser rather than the format. fluentd's LTSV parser and `Net::HTTP` both split a string that is not frozen, while Falcon matches a regex. Either cutoff seems defensible to me on this evidence. 256 covers 55 of the 65 shared values against 44, while 128 would halve the 6.4x worst case retained memory I reported in #note-5. I do not have a strong argument for one over the other. For Falcon I ran `protocol-http1`'s `HEADER` regex and read loop directly rather than starting a server. ---------------------------------------- Feature #22186: Increase the embeddable size limit for substrings created by `str_subseq()` https://bugs.ruby-lang.org/issues/22186#change-118138 * Author: himura467 (Akito Shitara) * Status: Open ---------------------------------------- ## Summary `str_subseq()` unnecessarily limits how large a sharable middle substring can be while still being embedded into the new `RString`. Substrings roughly 24 to 1000 bytes long (on a 64-bit build) always take the shared path, even though variable width allocation (VWA, Feature #18239) already supports embedding objects of that size. I propose sizing the allocation to the actual substring length via `STR_EMBEDDABLE_P()` / `str_alloc_embed()`, the same pattern used by other allocation sites in `string.c`, instead of allocating through `str_alloc_heap()` and checking whether the substring fits in the smallest heap slot class. ## Background For a sharable substring (per `SHARABLE_SUBSTRING_P`), `str_subseq()` either embeds the bytes directly into the new `RString`, or shares the parent's heap buffer, which keeps the parent alive. Since [commit 132f097149](https://github.com/ruby/ruby/commit/132f097149af36cb77308d9fe1c1a94940ab2089), the decision has been: ```c str2 = str_alloc_heap(rb_cString); if (str_embed_capa(str2) >= len + termlen) { // embed } else { // share } ``` `str_alloc_heap()` always allocates the default `sizeof(struct RString)` slot regardless of `len`, so `str_embed_capa(str2)` is capped at roughly 23 bytes. Meanwhile the default GC's size pools go up to 1024 bytes on 64-bit builds, and other allocation sites (`str_enc_new` behind `rb_str_new`, `rb_str_buf_new`, `str_new_frozen_buffer`, `rb_str_times`, ...) already pick the right-sized slot via `STR_EMBEDDABLE_P()` / `str_alloc_embed()`. ## Proposed change Draft PR: https://github.com/ruby/ruby/pull/17723 Check `STR_EMBEDDABLE_P(len, termlen)` up front and allocate with `str_alloc_embed()`; only allocate the `STR_NOEMBED` heap object on the sharing branch. Behavior is unchanged for substrings that aren't sharable or that reach the end of the parent's buffer. Only the threshold at which a sharable middle substring switches from embedding to sharing grows. ## Benchmark results Using [ruby-bench](https://github.com/ruby/ruby-bench), this branch vs current master (`arm64-darwin25`, `+PRISM`). Full results are attached as `output_001.txt`, with a per-benchmark ratio chart in `output_001-ratio-current.png`. Top 3 and bottom 3 by ratio (master/branch, higher is better): | bench | master (ms) | current (ms) | ratio | | ---- | ---- | ---- | ----- | | fluentd | 258.6 | 230.8 | 1.120 | | ruby-json | 154.1 | 150.2 | 1.026 | | liquid-c | 31.2 | 30.6 | 1.022 | | graphql-native | 172.0 | 174.9 | 0.983 | | activerecord | 148.7 | 153.0 | 0.972 | | psych-load | 1244.9 | 1291.4 | 0.964 | Note that most except fluentd varies between runs. The fluentd improvement reproduces across runs, while the entries in the bottom 3 do not. So the few 2-4% slowdowns may be noise, but I'd like to verify them with more runs and machines before treating this as risk-free. ## Relation to Feature #19315 #19315 (sharable middle substrings) addresses the same underlying problem: middle substrings copy unnecessarily. The branch benchmarked in https://bugs.ruby-lang.org/issues/19315#note-52 actually included this embed size change as one of its commits, in addition to enabling `SHARABLE_MIDDLE_SUBSTRING`. To isolate the two, I benchmarked a variant of that branch with only the embed size change reverted ([rstring-raw-ptr-reverted](https://github.com/himura467/ruby/tree/rstring-raw-ptr-reverted)), alongside this proposal, against the same master. Full results are attached as `output_002.txt`: | bench | master (ms) | this proposal (ms) | #19315 without this change (ms) | | ---- | ---- | ---- | ---- | | fluentd | 256.9 | 207.2 (x1.240) | 255.6 (x1.005) | The clear `fluentd` improvement seen in note-52 reproduces with this change alone, and disappears when this change is reverted from the #19315 branch. So the practical win observed on ruby-bench so far is attributable to the larger embed threshold rather than to `SHARABLE_MIDDLE_SUBSTRING` itself. Beyond that, this change is much narrower: - A self-contained fix to one function, reusing the VWA embedding infrastructure already battle-tested elsewhere in `string.c`. - No API/ABI implications; `RSTRING_PTR()`'s `\0`-termination guarantee is untouched. - No memory-retention risk, since embedding copies the bytes instead of referencing the parent. The two are orthogonal (#19315 still helps for slices too large to embed, as its micro-benchmarks show), but this smaller fix seems worth landing first on its own merits. ---Files-------------------------------- output_001.txt (7.76 KB) output_001-ratio-current.png (184 KB) output_002.txt (14.9 KB) string_subseq.yml (418 Bytes) str_subseq_memory.rb (1.25 KB) -- https://bugs.ruby-lang.org/