You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@thrift.apache.org by "kpumuk (via GitHub)" <gi...@apache.org> on 2026/10/23 20:32:49 UTC
[PR] THRIFT-6451: Reduce temporary allocations when reading Ruby JSON strings [thrift]
kpumuk opened a new pull request, #4045:
URL: https://github.com/apache/thrift/pull/4045
<!-- Explain the changes in the pull request below: -->
Reading a large JSON string currently creates a temporary String for nearly every input byte. A 1 MiB ASCII string allocates roughly one million objects.
Use the transport’s `read_byte` API after the first 16 decoded bytes and share frozen escape tables. Transports without `read_byte` retain the `read(1)` path. Regression coverage includes fragmented input, escapes, Unicode, custom readers and transports, size limits, and frame boundaries.
The 0.26.0 README entry documents the compatibility change: custom `read` hooks must keep `read_byte` consistent because `read` alone no longer observes every byte in JSON strings.
Save this as `/tmp/thrift-json-read-alloc.rb`:
```ruby
# frozen_string_literal: true
$LOADED_FEATURES << "thrift_native.rb" if ENV["PURE"] == "1"
require "thrift"
value = "a" * 1_048_576
wire = '"' + value + '"'
Thrift::JsonProtocol.new(Thrift::MemoryBufferTransport.new('"warm"')).read_string
samples = 5.times.map do
transport = Thrift::MemoryBufferTransport.new(wire)
protocol = Thrift::JsonProtocol.new(transport)
GC.start
before = GC.stat(:total_allocated_objects)
started = Process.clock_gettime(Process::CLOCK_MONOTONIC)
actual = protocol.read_string
elapsed = Process.clock_gettime(Process::CLOCK_MONOTONIC) - started
allocated = GC.stat(:total_allocated_objects) - before
abort "incorrect result" unless actual == value && transport.available.zero?
[allocated, elapsed * 1000]
end
puts "bytes=#{value.bytesize} allocations=#{samples.map(&:first).sort[2]}"
puts format("median_ms=%.3f", samples.map(&:last).sort[2])
```
From `lib/rb`, with dependencies installed and the native extension built:
```sh
ruby -W0 -Ilib -Iext /tmp/thrift-json-read-alloc.rb
PURE=1 ruby -W0 -Ilib -Iext /tmp/thrift-json-read-alloc.rb
```
## Benchmarks
Compared master `0a8eb9a5cb8793b6d892327604fb56ea351a1e9f` with this change using Ruby 4.0.7 on aarch64 Linux. The commands above measure a 1 MiB ASCII string with MemoryBufferTransport. Each configuration ran five trials serially, with normal GC enabled and `GC.start` before each trial. Payload and protocol construction are outside the measured interval. Every trial verifies the decoded value and that no input remains.
| Mode | Master allocations | Proposed allocations | Allocation change | Master median ms | Proposed median ms | Time change |
|---|---:|---:|---:|---:|---:|---:|
| Native extension | 1,048,589 | 27 | −99.9974% | 369.425 | 177.967 | −51.83% |
| Pure Ruby | 1,048,604 | 42 | −99.9960% | 388.660 | 228.585 | −41.19% |
These results cover one runtime, architecture, transport, and ASCII workload. Shared-host timings are diagnostic; gains for other transports depend on their `read_byte` implementation.
<!-- We recommend you review the checklist/tips before submitting a pull request. -->
- [x] Did you create an [Apache Jira](https://issues.apache.org/jira/browse/THRIFT-6451) ticket? ([Request account here](https://selfserve.apache.org/jira-account.html), not required for trivial changes)
- [x] If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"?
- [x] Did you squash your changes to a single commit? (not required, but preferred)
- [ ] Did you do your best to avoid breaking changes? If one was needed, did you label the Jira ticket with "Breaking-Change"?
- [ ] If your change does not involve any code, include `[skip ci]` anywhere in the commit message to free up build resources.
<!--
The Contributing Guide at:
https://github.com/apache/thrift/blob/master/CONTRIBUTING.md
has more details and tips for committing properly.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-6451: Reduce temporary allocations when reading Ruby JSON strings [thrift]
Posted by "kpumuk (via GitHub)" <gi...@apache.org>.
kpumuk commented on code in PR #4045:
URL: https://github.com/apache/thrift/pull/4045#discussion_r4364278167
##########
lib/rb/lib/thrift/protocol/json_protocol.rb:
##########
@@ -498,40 +516,46 @@ def read_json_escape_char
# Decodes a JSON string, including unescaping, and returns the string via str
def read_json_string(skipContext = false)
- # This string's characters must match up with the elements in escape_char_vals.
- # JSON permits the optional solidus escape "\/"; see RFC 8259, section 7.
- escape_chars = "\"\\/bfnrt"
-
- # The elements of this array must match up with the sequence of characters in
- # escape_chars
- escape_char_vals = [
- "\"", "\\", "/", "\b", "\f", "\n", "\r", "\t",
- ]
-
if !skipContext
@context.read(@reader)
end
read_json_syntax_char(@@kJSONStringDelimiter)
str = Bytes.empty_byte_buffer
+ ch = @reader.read
+ return decode_json_string(str) if ch == @@kJSONStringDelimiter
+
+ byte_reads = nil
Review Comment:
Kept the three-state flag and documented its values in d0f57fe2589905c62a6f2586366282e7b5387da3: nil means unchecked, false means unavailable, and true means enabled. This lets tiny strings skip capability detection and ensures longer strings check only once. Two booleans would introduce an additional variable and an invalid state without simplifying the transitions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-6451: Reduce temporary allocations when reading Ruby JSON strings [thrift]
Posted by "kpumuk (via GitHub)" <gi...@apache.org>.
kpumuk commented on code in PR #4045:
URL: https://github.com/apache/thrift/pull/4045#discussion_r4364515279
##########
lib/rb/lib/thrift/protocol/json_protocol.rb:
##########
@@ -27,6 +27,18 @@ def initialize(trans)
@data = nil
end
+ def byte_reads?
+ @trans.respond_to?(:read_byte)
+ end
+
+ def read_byte
+ if @hasData
+ read.getbyte(0)
+ else
+ @trans.read_byte
Review Comment:
Tested this with custom transports returning Strings, nil, and out-of-range Integers. The String case does consume through EOF, but these returns violate BaseTransport#read_byte's existing Integer 0..255 contract, which is also documented in the 0.26.0 README entry. CompactProtocol already relies on that same contract. We are intentionally retaining it without adding a type/range check to every JSON byte read; custom transports must implement read_byte consistently or omit the method to use the read(1) fallback.
##########
lib/rb/lib/thrift/protocol/json_protocol.rb:
##########
@@ -498,40 +516,47 @@ def read_json_escape_char
# Decodes a JSON string, including unescaping, and returns the string via str
def read_json_string(skipContext = false)
- # This string's characters must match up with the elements in escape_char_vals.
- # JSON permits the optional solidus escape "\/"; see RFC 8259, section 7.
- escape_chars = "\"\\/bfnrt"
-
- # The elements of this array must match up with the sequence of characters in
- # escape_chars
- escape_char_vals = [
- "\"", "\\", "/", "\b", "\f", "\n", "\r", "\t",
- ]
-
if !skipContext
@context.read(@reader)
end
read_json_syntax_char(@@kJSONStringDelimiter)
str = Bytes.empty_byte_buffer
+ ch = @reader.read
+ return decode_json_string(str) if ch == @@kJSONStringDelimiter
+
+ # nil: unchecked, false: unavailable, true: enabled.
+ byte_reads = nil
+ delimiter = @@kJSONStringDelimiter
+ backslash = @@kJSONBackslash
while true
- ch = @reader.read
- if (ch == @@kJSONStringDelimiter)
- break
- end
- if (ch == @@kJSONBackslash)
+ if (ch == backslash)
ch = @reader.read
if (ch == "u")
ch = read_json_escape_char
else
- pos = escape_chars.index(ch);
+ pos = JSON_ESCAPE_CHARS.index(ch)
if pos.nil? # not found
raise ProtocolException.new(ProtocolException::INVALID_DATA, "Expected control char, got '#{ch}'.")
end
- ch = escape_char_vals[pos]
+ ch = JSON_ESCAPE_VALUES[pos]
end
+ ch = Bytes.force_binary_encoding(ch)
+ elsif !byte_reads
+ ch = Bytes.force_binary_encoding(ch)
Review Comment:
Moved binary normalization into the Unicode-escape branch in b2e73a5e322ceeec825b6ffe3950177c81aaa4aa, so JSON_ESCAPE_VALUES entries now bypass the redundant call. The previous call allocated zero additional objects across 800,000 calls in both modes, but skipping it avoids unnecessary work. Unicode normalization remains necessary: removing it corrupts mixed escaped/raw UTF-8. Verified 60 targeted escape/Unicode-hook cases per mode and confirmed zero normalization calls for constant escapes. Focused specs passed: 149 native and 140 pure-Ruby examples, zero failures; RuboCop passed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-6451: Reduce temporary allocations when reading Ruby JSON strings [thrift]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #4045:
URL: https://github.com/apache/thrift/pull/4045#discussion_r4364117077
##########
lib/rb/spec/json_protocol_spec.rb:
##########
@@ -21,6 +21,177 @@
require "spec_helper"
+describe "JSON input byte ownership" do
+ class FragmentedJsonInput < Thrift::BaseTransport
+ attr_reader :requested
+
+ def initialize(parts)
+ @parts = parts.map(&:b)
+ @requested = []
+ end
+
+ def read(size)
+ @requested << size
+ @parts.shift while @parts.first == ""
+ raise EOFError, "fragmented input exhausted" if @parts.empty?
+
+ @parts.first.slice!(0, size)
+ end
+ end
+
+ it "reads Unicode, escapes, base64 and numbers across every fragment boundary" do
+ wire = <<~'JSON'.chomp.b
+ ["str",3,"raw € 😀\u20ac\uD83D\uDE00\\\"\/\b\f\n\r\t","QUJDRA",-12.5e+2]
+ JSON
+ (1...wire.bytesize).each do |split|
+ input = FragmentedJsonInput.new([wire.byteslice(0, split), wire.byteslice(split..-1)])
+ protocol = Thrift::JsonProtocol.new(Thrift::BufferedTransport.new(input))
+ expect(protocol.read_list_begin).to eq([Thrift::Types::STRING, 3])
+ expect(protocol.read_string).to eq("raw € 😀€😀\\\"/\b\f\n\r\t")
+ expect(protocol.read_binary).to eq("ABCD")
+ expect(protocol.read_double).to eq(-1250.0)
+ protocol.read_list_end
+ end
+ end
+
+ it "leaves the following value in the transport and owns at most one peeked byte" do
+ trans = Thrift::MemoryBufferTransport.new('"first"123,"second"')
+ protocol = Thrift::JsonProtocol.new(trans)
+ expect(protocol.read_string).to eq("first")
+ expect(trans.available).to eq('123,"second"'.bytesize)
+ expect(protocol.read_i32).to eq(123)
+ expect(trans.read(1)).to eq('"') # Only the comma belongs to the reader.
+ reader = protocol.instance_variable_get(:@reader)
+ expect(reader.peek).to eq(",")
+ expect(reader.read).to eq(",")
+ end
+
+ it "falls back to read when the transport has no byte API" do
+ klass = Class.new(Thrift::MemoryBufferTransport) { undef_method :read_byte }
+ value = "v" * 64
+ expect(Thrift::JsonProtocol.new(klass.new('"' + value + '"')).read_string).to eq(value)
+ end
+
+ it "uses the byte API on a duck-typed transport" do
+ value = "v" * 64
+ input = Thrift::MemoryBufferTransport.new('"' + value + '"')
+ transport = double("Transport")
+ allow(transport).to receive(:read).with(1) { input.read(1) }
+ expect(transport).to receive(:read_byte).at_least(:once) { input.read_byte }
+
+ expect(Thrift::JsonProtocol.new(transport).read_string).to eq(value)
+ expect(input.available).to eq(0)
+ end
+
+ it "preserves Unicode decoding hooks" do
+ protocol_class = Class.new(Thrift::JsonProtocol) do
+ def read_json_escape_char
+ super.upcase
+ end
+ end
+ trans = Thrift::MemoryBufferTransport.new('"' + ("v" * 64) + 'a\\u0062"')
+ expect(protocol_class.new(trans).read_string).to eq(("v" * 64) + "aB")
+ end
+
+ it "keeps tiny strings on the read path without inspecting byte-read eligibility" do
+ protocol = Thrift::JsonProtocol.new(Thrift::MemoryBufferTransport.new('"""a"'))
+ reader = protocol.instance_variable_get(:@reader)
+ expect(reader).not_to receive(:byte_reads?)
+ expect(protocol.read_string).to eq("")
+ expect(protocol.read_string).to eq("a")
+ end
+
+ it "selects byte reads once when Unicode escapes cross the small-string threshold" do
+ wire = '"' + ('\\u20ac' * 6) + ("a" * 1024) + '"'
+ protocol = Thrift::JsonProtocol.new(Thrift::MemoryBufferTransport.new(wire))
+ reader = protocol.instance_variable_get(:@reader)
+ expect(reader).to receive(:byte_reads?).once.and_call_original
+ expect(reader).to receive(:read_byte).at_least(:once).and_call_original
+ expect(protocol.read_string).to eq(("€" * 6) + ("a" * 1024))
+ end
+
+ [Thrift::MemoryBufferTransport, Thrift::BufferedTransport].each do |klass|
+ it "uses the byte API when #{klass} customizes it" do
+ original = klass.instance_method(:read_byte)
+ reads = 0
+ klass.define_method(:read_byte) do
+ reads += 1
+ original.bind_call(self)
+ end
+ value = "a" * 64
+ wire = '"' + value + '"'
+ trans = if klass == Thrift::BufferedTransport
+ Thrift::BufferedTransport.new(FragmentedJsonInput.new([wire]))
+ else
+ Thrift::MemoryBufferTransport.new(wire)
+ end
+ expect(Thrift::JsonProtocol.new(trans).read_string).to eq(value)
+ expect(reads).to be > 0
+ ensure
+ klass.define_method(:read_byte, original)
Review Comment:
This spec mutates global transport classes by redefining `read_byte`, which can cause test flakiness under parallel/concurrent spec execution (other examples may observe the temporary override). Prefer using an anonymous subclass per example (e.g., `Class.new(klass)` overriding `read_byte`) and instantiate that subclass, avoiding shared global mutation.
##########
lib/rb/spec/json_protocol_spec.rb:
##########
@@ -21,6 +21,177 @@
require "spec_helper"
+describe "JSON input byte ownership" do
+ class FragmentedJsonInput < Thrift::BaseTransport
+ attr_reader :requested
+
+ def initialize(parts)
+ @parts = parts.map(&:b)
+ @requested = []
+ end
+
+ def read(size)
+ @requested << size
+ @parts.shift while @parts.first == ""
+ raise EOFError, "fragmented input exhausted" if @parts.empty?
+
+ @parts.first.slice!(0, size)
+ end
+ end
+
+ it "reads Unicode, escapes, base64 and numbers across every fragment boundary" do
+ wire = <<~'JSON'.chomp.b
+ ["str",3,"raw € 😀\u20ac\uD83D\uDE00\\\"\/\b\f\n\r\t","QUJDRA",-12.5e+2]
+ JSON
+ (1...wire.bytesize).each do |split|
+ input = FragmentedJsonInput.new([wire.byteslice(0, split), wire.byteslice(split..-1)])
+ protocol = Thrift::JsonProtocol.new(Thrift::BufferedTransport.new(input))
+ expect(protocol.read_list_begin).to eq([Thrift::Types::STRING, 3])
+ expect(protocol.read_string).to eq("raw € 😀€😀\\\"/\b\f\n\r\t")
+ expect(protocol.read_binary).to eq("ABCD")
+ expect(protocol.read_double).to eq(-1250.0)
+ protocol.read_list_end
+ end
+ end
+
+ it "leaves the following value in the transport and owns at most one peeked byte" do
+ trans = Thrift::MemoryBufferTransport.new('"first"123,"second"')
+ protocol = Thrift::JsonProtocol.new(trans)
+ expect(protocol.read_string).to eq("first")
+ expect(trans.available).to eq('123,"second"'.bytesize)
+ expect(protocol.read_i32).to eq(123)
+ expect(trans.read(1)).to eq('"') # Only the comma belongs to the reader.
+ reader = protocol.instance_variable_get(:@reader)
+ expect(reader.peek).to eq(",")
+ expect(reader.read).to eq(",")
+ end
+
+ it "falls back to read when the transport has no byte API" do
+ klass = Class.new(Thrift::MemoryBufferTransport) { undef_method :read_byte }
+ value = "v" * 64
+ expect(Thrift::JsonProtocol.new(klass.new('"' + value + '"')).read_string).to eq(value)
+ end
+
+ it "uses the byte API on a duck-typed transport" do
+ value = "v" * 64
+ input = Thrift::MemoryBufferTransport.new('"' + value + '"')
+ transport = double("Transport")
+ allow(transport).to receive(:read).with(1) { input.read(1) }
+ expect(transport).to receive(:read_byte).at_least(:once) { input.read_byte }
+
+ expect(Thrift::JsonProtocol.new(transport).read_string).to eq(value)
+ expect(input.available).to eq(0)
+ end
+
+ it "preserves Unicode decoding hooks" do
+ protocol_class = Class.new(Thrift::JsonProtocol) do
+ def read_json_escape_char
+ super.upcase
+ end
+ end
+ trans = Thrift::MemoryBufferTransport.new('"' + ("v" * 64) + 'a\\u0062"')
+ expect(protocol_class.new(trans).read_string).to eq(("v" * 64) + "aB")
+ end
+
+ it "keeps tiny strings on the read path without inspecting byte-read eligibility" do
+ protocol = Thrift::JsonProtocol.new(Thrift::MemoryBufferTransport.new('"""a"'))
+ reader = protocol.instance_variable_get(:@reader)
+ expect(reader).not_to receive(:byte_reads?)
+ expect(protocol.read_string).to eq("")
+ expect(protocol.read_string).to eq("a")
+ end
+
+ it "selects byte reads once when Unicode escapes cross the small-string threshold" do
+ wire = '"' + ('\\u20ac' * 6) + ("a" * 1024) + '"'
+ protocol = Thrift::JsonProtocol.new(Thrift::MemoryBufferTransport.new(wire))
+ reader = protocol.instance_variable_get(:@reader)
+ expect(reader).to receive(:byte_reads?).once.and_call_original
+ expect(reader).to receive(:read_byte).at_least(:once).and_call_original
+ expect(protocol.read_string).to eq(("€" * 6) + ("a" * 1024))
+ end
+
+ [Thrift::MemoryBufferTransport, Thrift::BufferedTransport].each do |klass|
+ it "uses the byte API when #{klass} customizes it" do
+ original = klass.instance_method(:read_byte)
+ reads = 0
+ klass.define_method(:read_byte) do
+ reads += 1
+ original.bind_call(self)
Review Comment:
`UnboundMethod#bind_call` is not universally supported across all Ruby implementations/versions that projects sometimes run specs under (e.g., some alternative Ruby runtimes). Using `original.bind(self).call` is more broadly compatible for test code, at the cost of a small performance difference that doesn't matter in specs.
##########
lib/rb/lib/thrift/protocol/json_protocol.rb:
##########
@@ -498,40 +516,46 @@ def read_json_escape_char
# Decodes a JSON string, including unescaping, and returns the string via str
def read_json_string(skipContext = false)
- # This string's characters must match up with the elements in escape_char_vals.
- # JSON permits the optional solidus escape "\/"; see RFC 8259, section 7.
- escape_chars = "\"\\/bfnrt"
-
- # The elements of this array must match up with the sequence of characters in
- # escape_chars
- escape_char_vals = [
- "\"", "\\", "/", "\b", "\f", "\n", "\r", "\t",
- ]
-
if !skipContext
@context.read(@reader)
end
read_json_syntax_char(@@kJSONStringDelimiter)
str = Bytes.empty_byte_buffer
+ ch = @reader.read
+ return decode_json_string(str) if ch == @@kJSONStringDelimiter
+
+ byte_reads = nil
+ delimiter = @@kJSONStringDelimiter
+ backslash = @@kJSONBackslash
while true
- ch = @reader.read
- if (ch == @@kJSONStringDelimiter)
- break
- end
- if (ch == @@kJSONBackslash)
+ if (ch == backslash)
ch = @reader.read
if (ch == "u")
ch = read_json_escape_char
else
- pos = escape_chars.index(ch);
+ pos = JSON_ESCAPE_CHARS.index(ch);
if pos.nil? # not found
raise ProtocolException.new(ProtocolException::INVALID_DATA, "Expected control char, got '#{ch}'.")
end
- ch = escape_char_vals[pos]
+ ch = JSON_ESCAPE_VALUES[pos]
end
+ ch = Bytes.force_binary_encoding(ch)
+ elsif !byte_reads
+ ch = Bytes.force_binary_encoding(ch)
end
- str << Bytes.force_binary_encoding(ch)
- check_string_size(str.bytesize)
+ str << ch
+ size = str.bytesize
+ check_string_size(size)
+ # Avoid byte-read setup for tiny strings; select once per value.
+ if byte_reads.nil? && size >= 16
+ byte_reads = @reader.is_a?(LookaheadReader) && @reader.byte_reads?
+ if byte_reads
+ delimiter = @@kJSONStringDelimiter.getbyte(0)
+ backslash = @@kJSONBackslash.getbyte(0)
+ end
+ end
+ ch = byte_reads ? @reader.read_byte : @reader.read
Review Comment:
The `byte_reads` flag is tri-state (`nil` = unchecked, `false` = checked but unavailable, `true` = enabled), and it’s also used in a boolean context (`elsif !byte_reads`), which makes the control flow harder to reason about. Consider splitting this into two variables (e.g., `byte_reads_checked` + `byte_reads_enabled`) or initializing to `false` and using a separate `checked` flag so the conditions read unambiguously.
##########
lib/rb/lib/thrift/protocol/json_protocol.rb:
##########
@@ -498,40 +516,46 @@ def read_json_escape_char
# Decodes a JSON string, including unescaping, and returns the string via str
def read_json_string(skipContext = false)
- # This string's characters must match up with the elements in escape_char_vals.
- # JSON permits the optional solidus escape "\/"; see RFC 8259, section 7.
- escape_chars = "\"\\/bfnrt"
-
- # The elements of this array must match up with the sequence of characters in
- # escape_chars
- escape_char_vals = [
- "\"", "\\", "/", "\b", "\f", "\n", "\r", "\t",
- ]
-
if !skipContext
@context.read(@reader)
end
read_json_syntax_char(@@kJSONStringDelimiter)
str = Bytes.empty_byte_buffer
+ ch = @reader.read
+ return decode_json_string(str) if ch == @@kJSONStringDelimiter
+
+ byte_reads = nil
+ delimiter = @@kJSONStringDelimiter
+ backslash = @@kJSONBackslash
while true
- ch = @reader.read
- if (ch == @@kJSONStringDelimiter)
- break
- end
- if (ch == @@kJSONBackslash)
+ if (ch == backslash)
ch = @reader.read
if (ch == "u")
ch = read_json_escape_char
else
- pos = escape_chars.index(ch);
+ pos = JSON_ESCAPE_CHARS.index(ch);
if pos.nil? # not found
raise ProtocolException.new(ProtocolException::INVALID_DATA, "Expected control char, got '#{ch}'.")
end
- ch = escape_char_vals[pos]
+ ch = JSON_ESCAPE_VALUES[pos]
end
+ ch = Bytes.force_binary_encoding(ch)
+ elsif !byte_reads
Review Comment:
The `byte_reads` flag is tri-state (`nil` = unchecked, `false` = checked but unavailable, `true` = enabled), and it’s also used in a boolean context (`elsif !byte_reads`), which makes the control flow harder to reason about. Consider splitting this into two variables (e.g., `byte_reads_checked` + `byte_reads_enabled`) or initializing to `false` and using a separate `checked` flag so the conditions read unambiguously.
##########
lib/rb/lib/thrift/protocol/json_protocol.rb:
##########
@@ -498,40 +516,46 @@ def read_json_escape_char
# Decodes a JSON string, including unescaping, and returns the string via str
def read_json_string(skipContext = false)
- # This string's characters must match up with the elements in escape_char_vals.
- # JSON permits the optional solidus escape "\/"; see RFC 8259, section 7.
- escape_chars = "\"\\/bfnrt"
-
- # The elements of this array must match up with the sequence of characters in
- # escape_chars
- escape_char_vals = [
- "\"", "\\", "/", "\b", "\f", "\n", "\r", "\t",
- ]
-
if !skipContext
@context.read(@reader)
end
read_json_syntax_char(@@kJSONStringDelimiter)
str = Bytes.empty_byte_buffer
+ ch = @reader.read
+ return decode_json_string(str) if ch == @@kJSONStringDelimiter
+
+ byte_reads = nil
Review Comment:
The `byte_reads` flag is tri-state (`nil` = unchecked, `false` = checked but unavailable, `true` = enabled), and it’s also used in a boolean context (`elsif !byte_reads`), which makes the control flow harder to reason about. Consider splitting this into two variables (e.g., `byte_reads_checked` + `byte_reads_enabled`) or initializing to `false` and using a separate `checked` flag so the conditions read unambiguously.
##########
lib/rb/lib/thrift/protocol/json_protocol.rb:
##########
@@ -498,40 +516,46 @@ def read_json_escape_char
# Decodes a JSON string, including unescaping, and returns the string via str
def read_json_string(skipContext = false)
- # This string's characters must match up with the elements in escape_char_vals.
- # JSON permits the optional solidus escape "\/"; see RFC 8259, section 7.
- escape_chars = "\"\\/bfnrt"
-
- # The elements of this array must match up with the sequence of characters in
- # escape_chars
- escape_char_vals = [
- "\"", "\\", "/", "\b", "\f", "\n", "\r", "\t",
- ]
-
if !skipContext
@context.read(@reader)
end
read_json_syntax_char(@@kJSONStringDelimiter)
str = Bytes.empty_byte_buffer
+ ch = @reader.read
+ return decode_json_string(str) if ch == @@kJSONStringDelimiter
+
+ byte_reads = nil
+ delimiter = @@kJSONStringDelimiter
+ backslash = @@kJSONBackslash
while true
- ch = @reader.read
- if (ch == @@kJSONStringDelimiter)
- break
- end
- if (ch == @@kJSONBackslash)
+ if (ch == backslash)
ch = @reader.read
if (ch == "u")
ch = read_json_escape_char
else
- pos = escape_chars.index(ch);
+ pos = JSON_ESCAPE_CHARS.index(ch);
Review Comment:
There’s an unnecessary trailing semicolon here, which is uncommon in the surrounding Ruby style and can be removed to match typical Ruby conventions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-6451: Reduce temporary allocations when reading Ruby JSON strings [thrift]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #4045:
URL: https://github.com/apache/thrift/pull/4045#discussion_r4364618182
##########
lib/rb/lib/thrift/protocol/json_protocol.rb:
##########
@@ -27,6 +27,18 @@ def initialize(trans)
@data = nil
end
+ def byte_reads?
+ @trans.respond_to?(:read_byte)
+ end
+
+ def read_byte
+ if @hasData
+ read.getbyte(0)
+ else
+ @trans.read_byte
+ end
Review Comment:
`LookaheadReader#read_byte` assumes `@trans.read_byte` returns an Integer 0..255 (and raises on EOF). If a transport returns `nil` at EOF (common IO-style behavior) or returns a 1-byte String, downstream logic can fail with a confusing `TypeError` or (worse) never match delimiters once `read_json_string` switches to integer delimiter comparisons. Consider normalizing the return value here (e.g., accept Integer, accept 1-byte String by converting via `getbyte(0)`, and turn `nil`/empty into a consistent `EOFError`) so JSON decoding remains correct and failures are diagnosable even when a transport’s `read_byte` isn’t perfectly compliant.
##########
lib/rb/lib/thrift/protocol/json_protocol.rb:
##########
@@ -498,40 +516,46 @@ def read_json_escape_char
# Decodes a JSON string, including unescaping, and returns the string via str
def read_json_string(skipContext = false)
- # This string's characters must match up with the elements in escape_char_vals.
- # JSON permits the optional solidus escape "\/"; see RFC 8259, section 7.
- escape_chars = "\"\\/bfnrt"
-
- # The elements of this array must match up with the sequence of characters in
- # escape_chars
- escape_char_vals = [
- "\"", "\\", "/", "\b", "\f", "\n", "\r", "\t",
- ]
-
if !skipContext
@context.read(@reader)
end
read_json_syntax_char(@@kJSONStringDelimiter)
str = Bytes.empty_byte_buffer
+ ch = @reader.read
+ return decode_json_string(str) if ch == @@kJSONStringDelimiter
+
+ # nil: unchecked, false: unavailable, true: enabled.
+ byte_reads = nil
+ delimiter = @@kJSONStringDelimiter
+ backslash = @@kJSONBackslash
while true
- ch = @reader.read
- if (ch == @@kJSONStringDelimiter)
- break
- end
- if (ch == @@kJSONBackslash)
+ if (ch == backslash)
ch = @reader.read
if (ch == "u")
- ch = read_json_escape_char
+ ch = Bytes.force_binary_encoding(read_json_escape_char)
else
- pos = escape_chars.index(ch);
+ pos = JSON_ESCAPE_CHARS.index(ch)
if pos.nil? # not found
raise ProtocolException.new(ProtocolException::INVALID_DATA, "Expected control char, got '#{ch}'.")
end
- ch = escape_char_vals[pos]
+ ch = JSON_ESCAPE_VALUES[pos]
end
+ elsif !byte_reads
+ ch = Bytes.force_binary_encoding(ch)
end
- str << Bytes.force_binary_encoding(ch)
- check_string_size(str.bytesize)
+ str << ch
+ size = str.bytesize
+ check_string_size(size)
+ # Avoid byte-read setup for tiny strings; select once per value.
+ if byte_reads.nil? && size >= 16
+ byte_reads = @reader.is_a?(LookaheadReader) && @reader.byte_reads?
+ if byte_reads
+ delimiter = @@kJSONStringDelimiter.getbyte(0)
+ backslash = @@kJSONBackslash.getbyte(0)
+ end
+ end
+ ch = byte_reads ? @reader.read_byte : @reader.read
+ break if ch == delimiter
Review Comment:
Once `byte_reads` is enabled, this code path implicitly relies on `@reader.read_byte` returning Integers so delimiter/backslash comparisons work. If a custom transport returns an unexpected type (e.g., String or nil), the resulting error is likely to be opaque (e.g., delimiter never matches, or `String#<<` raises). If you keep the strict contract, consider raising a `Thrift::ProtocolException` with a clear message when `read_byte` returns an invalid type/value, so users overriding `read`/`read_byte` can quickly identify the incompatibility.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-6451: Reduce temporary allocations when reading Ruby JSON strings [thrift]
Posted by "kpumuk (via GitHub)" <gi...@apache.org>.
kpumuk commented on code in PR #4045:
URL: https://github.com/apache/thrift/pull/4045#discussion_r4364282093
##########
lib/rb/spec/json_protocol_spec.rb:
##########
@@ -21,6 +21,177 @@
require "spec_helper"
+describe "JSON input byte ownership" do
+ class FragmentedJsonInput < Thrift::BaseTransport
+ attr_reader :requested
+
+ def initialize(parts)
+ @parts = parts.map(&:b)
+ @requested = []
+ end
+
+ def read(size)
+ @requested << size
+ @parts.shift while @parts.first == ""
+ raise EOFError, "fragmented input exhausted" if @parts.empty?
+
+ @parts.first.slice!(0, size)
+ end
+ end
+
+ it "reads Unicode, escapes, base64 and numbers across every fragment boundary" do
+ wire = <<~'JSON'.chomp.b
+ ["str",3,"raw € 😀\u20ac\uD83D\uDE00\\\"\/\b\f\n\r\t","QUJDRA",-12.5e+2]
+ JSON
+ (1...wire.bytesize).each do |split|
+ input = FragmentedJsonInput.new([wire.byteslice(0, split), wire.byteslice(split..-1)])
+ protocol = Thrift::JsonProtocol.new(Thrift::BufferedTransport.new(input))
+ expect(protocol.read_list_begin).to eq([Thrift::Types::STRING, 3])
+ expect(protocol.read_string).to eq("raw € 😀€😀\\\"/\b\f\n\r\t")
+ expect(protocol.read_binary).to eq("ABCD")
+ expect(protocol.read_double).to eq(-1250.0)
+ protocol.read_list_end
+ end
+ end
+
+ it "leaves the following value in the transport and owns at most one peeked byte" do
+ trans = Thrift::MemoryBufferTransport.new('"first"123,"second"')
+ protocol = Thrift::JsonProtocol.new(trans)
+ expect(protocol.read_string).to eq("first")
+ expect(trans.available).to eq('123,"second"'.bytesize)
+ expect(protocol.read_i32).to eq(123)
+ expect(trans.read(1)).to eq('"') # Only the comma belongs to the reader.
+ reader = protocol.instance_variable_get(:@reader)
+ expect(reader.peek).to eq(",")
+ expect(reader.read).to eq(",")
+ end
+
+ it "falls back to read when the transport has no byte API" do
+ klass = Class.new(Thrift::MemoryBufferTransport) { undef_method :read_byte }
+ value = "v" * 64
+ expect(Thrift::JsonProtocol.new(klass.new('"' + value + '"')).read_string).to eq(value)
+ end
+
+ it "uses the byte API on a duck-typed transport" do
+ value = "v" * 64
+ input = Thrift::MemoryBufferTransport.new('"' + value + '"')
+ transport = double("Transport")
+ allow(transport).to receive(:read).with(1) { input.read(1) }
+ expect(transport).to receive(:read_byte).at_least(:once) { input.read_byte }
+
+ expect(Thrift::JsonProtocol.new(transport).read_string).to eq(value)
+ expect(input.available).to eq(0)
+ end
+
+ it "preserves Unicode decoding hooks" do
+ protocol_class = Class.new(Thrift::JsonProtocol) do
+ def read_json_escape_char
+ super.upcase
+ end
+ end
+ trans = Thrift::MemoryBufferTransport.new('"' + ("v" * 64) + 'a\\u0062"')
+ expect(protocol_class.new(trans).read_string).to eq(("v" * 64) + "aB")
+ end
+
+ it "keeps tiny strings on the read path without inspecting byte-read eligibility" do
+ protocol = Thrift::JsonProtocol.new(Thrift::MemoryBufferTransport.new('"""a"'))
+ reader = protocol.instance_variable_get(:@reader)
+ expect(reader).not_to receive(:byte_reads?)
+ expect(protocol.read_string).to eq("")
+ expect(protocol.read_string).to eq("a")
+ end
+
+ it "selects byte reads once when Unicode escapes cross the small-string threshold" do
+ wire = '"' + ('\\u20ac' * 6) + ("a" * 1024) + '"'
+ protocol = Thrift::JsonProtocol.new(Thrift::MemoryBufferTransport.new(wire))
+ reader = protocol.instance_variable_get(:@reader)
+ expect(reader).to receive(:byte_reads?).once.and_call_original
+ expect(reader).to receive(:read_byte).at_least(:once).and_call_original
+ expect(protocol.read_string).to eq(("€" * 6) + ("a" * 1024))
+ end
+
+ [Thrift::MemoryBufferTransport, Thrift::BufferedTransport].each do |klass|
+ it "uses the byte API when #{klass} customizes it" do
+ original = klass.instance_method(:read_byte)
+ reads = 0
+ klass.define_method(:read_byte) do
+ reads += 1
+ original.bind_call(self)
Review Comment:
Removed this bind_call as part of isolating the spec in d0f57fe2589905c62a6f2586366282e7b5387da3. Each example now uses an anonymous transport subclass and calls super from its read_byte override, so no saved UnboundMethod or restoration is needed.
##########
lib/rb/spec/json_protocol_spec.rb:
##########
@@ -21,6 +21,177 @@
require "spec_helper"
+describe "JSON input byte ownership" do
+ class FragmentedJsonInput < Thrift::BaseTransport
+ attr_reader :requested
+
+ def initialize(parts)
+ @parts = parts.map(&:b)
+ @requested = []
+ end
+
+ def read(size)
+ @requested << size
+ @parts.shift while @parts.first == ""
+ raise EOFError, "fragmented input exhausted" if @parts.empty?
+
+ @parts.first.slice!(0, size)
+ end
+ end
+
+ it "reads Unicode, escapes, base64 and numbers across every fragment boundary" do
+ wire = <<~'JSON'.chomp.b
+ ["str",3,"raw € 😀\u20ac\uD83D\uDE00\\\"\/\b\f\n\r\t","QUJDRA",-12.5e+2]
+ JSON
+ (1...wire.bytesize).each do |split|
+ input = FragmentedJsonInput.new([wire.byteslice(0, split), wire.byteslice(split..-1)])
+ protocol = Thrift::JsonProtocol.new(Thrift::BufferedTransport.new(input))
+ expect(protocol.read_list_begin).to eq([Thrift::Types::STRING, 3])
+ expect(protocol.read_string).to eq("raw € 😀€😀\\\"/\b\f\n\r\t")
+ expect(protocol.read_binary).to eq("ABCD")
+ expect(protocol.read_double).to eq(-1250.0)
+ protocol.read_list_end
+ end
+ end
+
+ it "leaves the following value in the transport and owns at most one peeked byte" do
+ trans = Thrift::MemoryBufferTransport.new('"first"123,"second"')
+ protocol = Thrift::JsonProtocol.new(trans)
+ expect(protocol.read_string).to eq("first")
+ expect(trans.available).to eq('123,"second"'.bytesize)
+ expect(protocol.read_i32).to eq(123)
+ expect(trans.read(1)).to eq('"') # Only the comma belongs to the reader.
+ reader = protocol.instance_variable_get(:@reader)
+ expect(reader.peek).to eq(",")
+ expect(reader.read).to eq(",")
+ end
+
+ it "falls back to read when the transport has no byte API" do
+ klass = Class.new(Thrift::MemoryBufferTransport) { undef_method :read_byte }
+ value = "v" * 64
+ expect(Thrift::JsonProtocol.new(klass.new('"' + value + '"')).read_string).to eq(value)
+ end
+
+ it "uses the byte API on a duck-typed transport" do
+ value = "v" * 64
+ input = Thrift::MemoryBufferTransport.new('"' + value + '"')
+ transport = double("Transport")
+ allow(transport).to receive(:read).with(1) { input.read(1) }
+ expect(transport).to receive(:read_byte).at_least(:once) { input.read_byte }
+
+ expect(Thrift::JsonProtocol.new(transport).read_string).to eq(value)
+ expect(input.available).to eq(0)
+ end
+
+ it "preserves Unicode decoding hooks" do
+ protocol_class = Class.new(Thrift::JsonProtocol) do
+ def read_json_escape_char
+ super.upcase
+ end
+ end
+ trans = Thrift::MemoryBufferTransport.new('"' + ("v" * 64) + 'a\\u0062"')
+ expect(protocol_class.new(trans).read_string).to eq(("v" * 64) + "aB")
+ end
+
+ it "keeps tiny strings on the read path without inspecting byte-read eligibility" do
+ protocol = Thrift::JsonProtocol.new(Thrift::MemoryBufferTransport.new('"""a"'))
+ reader = protocol.instance_variable_get(:@reader)
+ expect(reader).not_to receive(:byte_reads?)
+ expect(protocol.read_string).to eq("")
+ expect(protocol.read_string).to eq("a")
+ end
+
+ it "selects byte reads once when Unicode escapes cross the small-string threshold" do
+ wire = '"' + ('\\u20ac' * 6) + ("a" * 1024) + '"'
+ protocol = Thrift::JsonProtocol.new(Thrift::MemoryBufferTransport.new(wire))
+ reader = protocol.instance_variable_get(:@reader)
+ expect(reader).to receive(:byte_reads?).once.and_call_original
+ expect(reader).to receive(:read_byte).at_least(:once).and_call_original
+ expect(protocol.read_string).to eq(("€" * 6) + ("a" * 1024))
+ end
+
+ [Thrift::MemoryBufferTransport, Thrift::BufferedTransport].each do |klass|
+ it "uses the byte API when #{klass} customizes it" do
+ original = klass.instance_method(:read_byte)
+ reads = 0
+ klass.define_method(:read_byte) do
+ reads += 1
+ original.bind_call(self)
+ end
+ value = "a" * 64
+ wire = '"' + value + '"'
+ trans = if klass == Thrift::BufferedTransport
+ Thrift::BufferedTransport.new(FragmentedJsonInput.new([wire]))
+ else
+ Thrift::MemoryBufferTransport.new(wire)
+ end
+ expect(Thrift::JsonProtocol.new(trans).read_string).to eq(value)
+ expect(reads).to be > 0
+ ensure
+ klass.define_method(:read_byte, original)
Review Comment:
Changed both transport examples to anonymous subclasses with read_byte overrides calling super in d0f57fe2589905c62a6f2586366282e7b5387da3. The specs no longer redefine shared transport classes or need ensure-based restoration. Full suites passed: 1,168 native examples; 1,068 pure-Ruby examples with 18 expected native-only pending; zero failures.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-6451: Reduce temporary allocations when reading Ruby JSON strings [thrift]
Posted by "kpumuk (via GitHub)" <gi...@apache.org>.
kpumuk commented on code in PR #4045:
URL: https://github.com/apache/thrift/pull/4045#discussion_r4364635923
##########
lib/rb/lib/thrift/protocol/json_protocol.rb:
##########
@@ -27,6 +27,18 @@ def initialize(trans)
@data = nil
end
+ def byte_reads?
+ @trans.respond_to?(:read_byte)
+ end
+
+ def read_byte
+ if @hasData
+ read.getbyte(0)
+ else
+ @trans.read_byte
+ end
Review Comment:
A fresh thread does not make this a fresh finding. We already tested these exact noncompliant return values and explained the decision here: https://github.com/apache/thrift/pull/4045#discussion_r4364515279. BaseTransport#read_byte requires an Integer in 0..255 and an exception at EOF; the 0.26.0 README explicitly documents that requirement for custom transports. Accepting Strings or nil would expand the contract we deliberately chose to retain. Resolving as a duplicate; a failing case with a compliant transport would be useful new evidence.
##########
lib/rb/lib/thrift/protocol/json_protocol.rb:
##########
@@ -498,40 +516,46 @@ def read_json_escape_char
# Decodes a JSON string, including unescaping, and returns the string via str
def read_json_string(skipContext = false)
- # This string's characters must match up with the elements in escape_char_vals.
- # JSON permits the optional solidus escape "\/"; see RFC 8259, section 7.
- escape_chars = "\"\\/bfnrt"
-
- # The elements of this array must match up with the sequence of characters in
- # escape_chars
- escape_char_vals = [
- "\"", "\\", "/", "\b", "\f", "\n", "\r", "\t",
- ]
-
if !skipContext
@context.read(@reader)
end
read_json_syntax_char(@@kJSONStringDelimiter)
str = Bytes.empty_byte_buffer
+ ch = @reader.read
+ return decode_json_string(str) if ch == @@kJSONStringDelimiter
+
+ # nil: unchecked, false: unavailable, true: enabled.
+ byte_reads = nil
+ delimiter = @@kJSONStringDelimiter
+ backslash = @@kJSONBackslash
while true
- ch = @reader.read
- if (ch == @@kJSONStringDelimiter)
- break
- end
- if (ch == @@kJSONBackslash)
+ if (ch == backslash)
ch = @reader.read
if (ch == "u")
- ch = read_json_escape_char
+ ch = Bytes.force_binary_encoding(read_json_escape_char)
else
- pos = escape_chars.index(ch);
+ pos = JSON_ESCAPE_CHARS.index(ch)
if pos.nil? # not found
raise ProtocolException.new(ProtocolException::INVALID_DATA, "Expected control char, got '#{ch}'.")
end
- ch = escape_char_vals[pos]
+ ch = JSON_ESCAPE_VALUES[pos]
end
+ elsif !byte_reads
+ ch = Bytes.force_binary_encoding(ch)
end
- str << Bytes.force_binary_encoding(ch)
- check_string_size(str.bytesize)
+ str << ch
+ size = str.bytesize
+ check_string_size(size)
+ # Avoid byte-read setup for tiny strings; select once per value.
+ if byte_reads.nil? && size >= 16
+ byte_reads = @reader.is_a?(LookaheadReader) && @reader.byte_reads?
+ if byte_reads
+ delimiter = @@kJSONStringDelimiter.getbyte(0)
+ backslash = @@kJSONBackslash.getbyte(0)
+ end
+ end
+ ch = byte_reads ? @reader.read_byte : @reader.read
+ break if ch == delimiter
Review Comment:
Same contract, different line number. This repeats https://github.com/apache/thrift/pull/4045#discussion_r4364399315, already answered and resolved at https://github.com/apache/thrift/pull/4045#discussion_r4364515279. We tested String, nil, and out-of-range returns in both native and pure Ruby modes. The decision remains to rely on the documented transport contract without adding validation to every byte read. Resolving this duplicate too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-6451: Reduce temporary allocations when reading Ruby JSON strings [thrift]
Posted by "kpumuk (via GitHub)" <gi...@apache.org>.
kpumuk commented on code in PR #4045:
URL: https://github.com/apache/thrift/pull/4045#discussion_r4364283593
##########
lib/rb/lib/thrift/protocol/json_protocol.rb:
##########
@@ -498,40 +516,46 @@ def read_json_escape_char
# Decodes a JSON string, including unescaping, and returns the string via str
def read_json_string(skipContext = false)
- # This string's characters must match up with the elements in escape_char_vals.
- # JSON permits the optional solidus escape "\/"; see RFC 8259, section 7.
- escape_chars = "\"\\/bfnrt"
-
- # The elements of this array must match up with the sequence of characters in
- # escape_chars
- escape_char_vals = [
- "\"", "\\", "/", "\b", "\f", "\n", "\r", "\t",
- ]
-
if !skipContext
@context.read(@reader)
end
read_json_syntax_char(@@kJSONStringDelimiter)
str = Bytes.empty_byte_buffer
+ ch = @reader.read
+ return decode_json_string(str) if ch == @@kJSONStringDelimiter
+
+ byte_reads = nil
+ delimiter = @@kJSONStringDelimiter
+ backslash = @@kJSONBackslash
while true
- ch = @reader.read
- if (ch == @@kJSONStringDelimiter)
- break
- end
- if (ch == @@kJSONBackslash)
+ if (ch == backslash)
ch = @reader.read
if (ch == "u")
ch = read_json_escape_char
else
- pos = escape_chars.index(ch);
+ pos = JSON_ESCAPE_CHARS.index(ch);
Review Comment:
Removed the semicolon and enabled Style/Semicolon in lib/rb/.rubocop.yml in d0f57fe2589905c62a6f2586366282e7b5387da3. Corrected all 27 existing offenses so the new rule passes across the Ruby tree. Full RuboCop run: 146 files inspected, no offenses.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-6451: Reduce temporary allocations when reading Ruby JSON strings [thrift]
Posted by "kpumuk (via GitHub)" <gi...@apache.org>.
kpumuk merged PR #4045:
URL: https://github.com/apache/thrift/pull/4045
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-6451: Reduce temporary allocations when reading Ruby JSON strings [thrift]
Posted by "kpumuk (via GitHub)" <gi...@apache.org>.
kpumuk commented on code in PR #4045:
URL: https://github.com/apache/thrift/pull/4045#discussion_r4364280571
##########
lib/rb/lib/thrift/protocol/json_protocol.rb:
##########
@@ -498,40 +516,46 @@ def read_json_escape_char
# Decodes a JSON string, including unescaping, and returns the string via str
def read_json_string(skipContext = false)
- # This string's characters must match up with the elements in escape_char_vals.
- # JSON permits the optional solidus escape "\/"; see RFC 8259, section 7.
- escape_chars = "\"\\/bfnrt"
-
- # The elements of this array must match up with the sequence of characters in
- # escape_chars
- escape_char_vals = [
- "\"", "\\", "/", "\b", "\f", "\n", "\r", "\t",
- ]
-
if !skipContext
@context.read(@reader)
end
read_json_syntax_char(@@kJSONStringDelimiter)
str = Bytes.empty_byte_buffer
+ ch = @reader.read
+ return decode_json_string(str) if ch == @@kJSONStringDelimiter
+
+ byte_reads = nil
+ delimiter = @@kJSONStringDelimiter
+ backslash = @@kJSONBackslash
while true
- ch = @reader.read
- if (ch == @@kJSONStringDelimiter)
- break
- end
- if (ch == @@kJSONBackslash)
+ if (ch == backslash)
ch = @reader.read
if (ch == "u")
ch = read_json_escape_char
else
- pos = escape_chars.index(ch);
+ pos = JSON_ESCAPE_CHARS.index(ch);
if pos.nil? # not found
raise ProtocolException.new(ProtocolException::INVALID_DATA, "Expected control char, got '#{ch}'.")
end
- ch = escape_char_vals[pos]
+ ch = JSON_ESCAPE_VALUES[pos]
end
+ ch = Bytes.force_binary_encoding(ch)
+ elsif !byte_reads
Review Comment:
Both nil and false intentionally take the String-read path here; only true means the current character can be an Integer byte. Added a comment documenting the three states in d0f57fe2589905c62a6f2586366282e7b5387da3. Keeping one flag preserves that distinction without adding another variable.
##########
lib/rb/lib/thrift/protocol/json_protocol.rb:
##########
@@ -498,40 +516,46 @@ def read_json_escape_char
# Decodes a JSON string, including unescaping, and returns the string via str
def read_json_string(skipContext = false)
- # This string's characters must match up with the elements in escape_char_vals.
- # JSON permits the optional solidus escape "\/"; see RFC 8259, section 7.
- escape_chars = "\"\\/bfnrt"
-
- # The elements of this array must match up with the sequence of characters in
- # escape_chars
- escape_char_vals = [
- "\"", "\\", "/", "\b", "\f", "\n", "\r", "\t",
- ]
-
if !skipContext
@context.read(@reader)
end
read_json_syntax_char(@@kJSONStringDelimiter)
str = Bytes.empty_byte_buffer
+ ch = @reader.read
+ return decode_json_string(str) if ch == @@kJSONStringDelimiter
+
+ byte_reads = nil
+ delimiter = @@kJSONStringDelimiter
+ backslash = @@kJSONBackslash
while true
- ch = @reader.read
- if (ch == @@kJSONStringDelimiter)
- break
- end
- if (ch == @@kJSONBackslash)
+ if (ch == backslash)
ch = @reader.read
if (ch == "u")
ch = read_json_escape_char
else
- pos = escape_chars.index(ch);
+ pos = JSON_ESCAPE_CHARS.index(ch);
if pos.nil? # not found
raise ProtocolException.new(ProtocolException::INVALID_DATA, "Expected control char, got '#{ch}'.")
end
- ch = escape_char_vals[pos]
+ ch = JSON_ESCAPE_VALUES[pos]
end
+ ch = Bytes.force_binary_encoding(ch)
+ elsif !byte_reads
+ ch = Bytes.force_binary_encoding(ch)
end
- str << Bytes.force_binary_encoding(ch)
- check_string_size(str.bytesize)
+ str << ch
+ size = str.bytesize
+ check_string_size(size)
+ # Avoid byte-read setup for tiny strings; select once per value.
+ if byte_reads.nil? && size >= 16
+ byte_reads = @reader.is_a?(LookaheadReader) && @reader.byte_reads?
+ if byte_reads
+ delimiter = @@kJSONStringDelimiter.getbyte(0)
+ backslash = @@kJSONBackslash.getbyte(0)
+ end
+ end
+ ch = byte_reads ? @reader.read_byte : @reader.read
Review Comment:
The nil guard intentionally distinguishes an unchecked reader from one already checked and found unavailable. It keeps capability detection to once per value after the 16-byte threshold. Documented those three states in d0f57fe2589905c62a6f2586366282e7b5387da3; retaining the flag as discussed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-6451: Reduce temporary allocations when reading Ruby JSON strings [thrift]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #4045:
URL: https://github.com/apache/thrift/pull/4045#discussion_r4364399315
##########
lib/rb/lib/thrift/protocol/json_protocol.rb:
##########
@@ -27,6 +27,18 @@ def initialize(trans)
@data = nil
end
+ def byte_reads?
+ @trans.respond_to?(:read_byte)
+ end
+
+ def read_byte
+ if @hasData
+ read.getbyte(0)
+ else
+ @trans.read_byte
Review Comment:
`LookaheadReader#byte_reads?` enables the optimized path solely based on `respond_to?(:read_byte)`, but the string decoder assumes `read_byte` returns an Integer (0..255). If a custom transport implements `read_byte` but returns a 1-byte String (or another type), the JSON string loop will stop recognizing delimiters/backslashes correctly and can consume input incorrectly until EOF. Consider validating `@trans.read_byte`'s return value in `read_byte` (e.g., ensure it's an Integer 0..255) and raising a `Thrift::ProtocolException::INVALID_DATA` with a clear message when the contract is violated.
##########
lib/rb/lib/thrift/protocol/json_protocol.rb:
##########
@@ -498,40 +516,47 @@ def read_json_escape_char
# Decodes a JSON string, including unescaping, and returns the string via str
def read_json_string(skipContext = false)
- # This string's characters must match up with the elements in escape_char_vals.
- # JSON permits the optional solidus escape "\/"; see RFC 8259, section 7.
- escape_chars = "\"\\/bfnrt"
-
- # The elements of this array must match up with the sequence of characters in
- # escape_chars
- escape_char_vals = [
- "\"", "\\", "/", "\b", "\f", "\n", "\r", "\t",
- ]
-
if !skipContext
@context.read(@reader)
end
read_json_syntax_char(@@kJSONStringDelimiter)
str = Bytes.empty_byte_buffer
+ ch = @reader.read
+ return decode_json_string(str) if ch == @@kJSONStringDelimiter
+
+ # nil: unchecked, false: unavailable, true: enabled.
+ byte_reads = nil
+ delimiter = @@kJSONStringDelimiter
+ backslash = @@kJSONBackslash
while true
- ch = @reader.read
- if (ch == @@kJSONStringDelimiter)
- break
- end
- if (ch == @@kJSONBackslash)
+ if (ch == backslash)
ch = @reader.read
if (ch == "u")
ch = read_json_escape_char
else
- pos = escape_chars.index(ch);
+ pos = JSON_ESCAPE_CHARS.index(ch)
if pos.nil? # not found
raise ProtocolException.new(ProtocolException::INVALID_DATA, "Expected control char, got '#{ch}'.")
end
- ch = escape_char_vals[pos]
+ ch = JSON_ESCAPE_VALUES[pos]
end
+ ch = Bytes.force_binary_encoding(ch)
+ elsif !byte_reads
+ ch = Bytes.force_binary_encoding(ch)
Review Comment:
`JSON_ESCAPE_VALUES` is built from `ch.b.freeze`, so its entries should already be `Encoding::BINARY`. For escaped characters, calling `Bytes.force_binary_encoding(ch)` again is likely redundant and may allocate (depending on `Bytes.force_binary_encoding` implementation), partially undermining the goal of minimizing allocations. Consider skipping the `Bytes.force_binary_encoding` call when `ch` comes from `JSON_ESCAPE_VALUES` (or guarding it with an `encoding` check) so the escape fast-path stays allocation-free.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org