From 344a06c6982a6f6db7a0374082cce5e8ef9fb703 Mon Sep 17 00:00:00 2001 From: Dmytro Shteflyuk Date: Thu, 23 Jul 2026 17:51:58 -0400 Subject: [PATCH] THRIFT-6439: Isolate nested Ruby struct defaults Client: rb Co-Authored-By: OpenAI Codex (gpt-6.1-sol) --- lib/rb/ext/struct.c | 91 +++--- lib/rb/lib/thrift/struct.rb | 69 ++++- lib/rb/spec/ThriftSpec.thrift | 20 ++ lib/rb/spec/struct_spec.rb | 277 ++++++++++++++++++ test/rb/benchmarks/README.md | 30 +- test/rb/benchmarks/protocol_benchmark.rb | 30 ++ test/rb/benchmarks/test_protocol_benchmark.rb | 105 +++++++ 7 files changed, 570 insertions(+), 52 deletions(-) create mode 100644 test/rb/benchmarks/test_protocol_benchmark.rb diff --git a/lib/rb/ext/struct.c b/lib/rb/ext/struct.c index dd223b2ba85..47a24b5fa56 100644 --- a/lib/rb/ext/struct.c +++ b/lib/rb/ext/struct.c @@ -34,6 +34,8 @@ ID to_s_method_id; ID name_to_id_method_id; static ID sorted_field_ids_method_id; static ID validate_container_size_method_id; +static ID copy_default_value_method_id; +static VALUE thrift_struct_module; static VALUE default_sym; #define IS_CONTAINER(ttype) ((ttype) == TTYPE_MAP || (ttype) == TTYPE_LIST || (ttype) == TTYPE_SET) @@ -483,20 +485,6 @@ static VALUE rb_thrift_struct_read_recursive(VALUE self, VALUE protocol, int rem static void skip_map_contents(VALUE protocol, VALUE key_type_value, VALUE value_type_value, int size); static void skip_list_or_set_contents(VALUE protocol, VALUE element_type_value, int size); -static inline void set_field_value(VALUE obj, VALUE field_name, VALUE value) { - rb_ivar_set(obj, field_ivar_id(field_name), value); -} - -/* Ruby's rescue and iteration APIs require addressable callback functions. */ -static VALUE duplicate_default_value(VALUE value) { - return rb_obj_dup(value); -} - -static VALUE retain_default_value(VALUE value, VALUE exception) { - (void)exception; - return value; -} - static int reset_struct_field(VALUE field_id, VALUE field_info, VALUE self) { (void)field_id; @@ -509,14 +497,14 @@ static int reset_struct_field(VALUE field_id, VALUE field_info, VALUE self) { rb_ivar_set(self, ivar_id, Qnil); } } else { - VALUE value = rb_rescue2( - duplicate_default_value, - default_value, - retain_default_value, - default_value, - rb_eStandardError, - (VALUE)0 - ); + VALUE value; + if (RB_TYPE_P(default_value, T_STRING)) { + value = rb_str_dup(default_value); + } else if (RB_SPECIAL_CONST_P(default_value) || RB_TYPE_P(default_value, T_FLOAT) || RB_TYPE_P(default_value, T_BIGNUM)) { + value = default_value; + } else { + value = rb_funcall(thrift_struct_module, copy_default_value_method_id, 1, default_value); + } rb_ivar_set(self, ivar_id, value); } @@ -667,13 +655,18 @@ static VALUE read_anything(VALUE protocol, int ttype, VALUE field_info, int rema return result; } -static VALUE rb_thrift_struct_read_recursive(VALUE self, VALUE protocol, int remaining_depth) { - VALUE struct_fields = STRUCT_FIELDS(self); +struct struct_read_context { + VALUE self; + VALUE protocol; + VALUE fields; + VALUE unread_fields; + int remaining_depth; +}; - if (RHASH_SIZE(struct_fields) > 0 && rb_ivar_count(self) > 0) { - rb_check_frozen(self); - rb_hash_foreach(struct_fields, reset_struct_field, self); - } +static VALUE read_struct_fields(VALUE data) { + struct struct_read_context *context = (struct struct_read_context *)data; + VALUE self = context->self; + VALUE protocol = context->protocol; // read struct begin default_read_struct_begin(protocol); @@ -689,14 +682,19 @@ static VALUE rb_thrift_struct_read_recursive(VALUE self, VALUE protocol, int rem } // make sure we got a type we expected - VALUE field_info = rb_hash_aref(struct_fields, rb_ary_entry(field_header, 2)); + VALUE field_id = rb_ary_entry(field_header, 2); + VALUE field_info = rb_hash_aref(context->fields, field_id); if (!NIL_P(field_info)) { int specified_type = FIX2INT(rb_hash_aref(field_info, type_sym)); if (field_type == specified_type) { // read the value VALUE name = rb_hash_aref(field_info, name_sym); - set_field_value(self, name, read_anything(protocol, field_type, field_info, remaining_depth)); + VALUE value = read_anything(protocol, field_type, field_info, context->remaining_depth); + rb_ivar_set(self, field_ivar_id(name), value); + if (!NIL_P(context->unread_fields)) { + rb_hash_delete(context->unread_fields, field_id); + } } else { rb_funcall(protocol, skip_method_id, 1, field_type_value); } @@ -711,7 +709,30 @@ static VALUE rb_thrift_struct_read_recursive(VALUE self, VALUE protocol, int rem // read struct end default_read_struct_end(protocol); - // call validate + return Qnil; +} + +static VALUE reset_unread_struct_fields(VALUE data) { + struct struct_read_context *context = (struct struct_read_context *)data; + rb_hash_foreach(context->unread_fields, reset_struct_field, context->self); + return Qnil; +} + +static VALUE rb_thrift_struct_read_recursive(VALUE self, VALUE protocol, int remaining_depth) { + VALUE fields = STRUCT_FIELDS(self); + VALUE unread_fields = Qnil; + if (RHASH_SIZE(fields) > 0 && rb_ivar_count(self) > 0) { + rb_check_frozen(self); + unread_fields = rb_hash_dup(fields); + } + + struct struct_read_context context = {self, protocol, fields, unread_fields, remaining_depth}; + if (NIL_P(unread_fields)) { + read_struct_fields((VALUE)&context); + } else { + // Reset omitted fields on success or failure, without copying received defaults. + rb_ensure(read_struct_fields, (VALUE)&context, reset_unread_struct_fields, (VALUE)&context); + } rb_funcall(self, validate_method_id, 0); return Qnil; @@ -837,12 +858,13 @@ static VALUE rb_thrift_union_write(int argc, VALUE *argv, VALUE self) { } void Init_struct(void) { - VALUE struct_module = rb_const_get(thrift_module, rb_intern("Struct")); + thrift_struct_module = rb_const_get(thrift_module, rb_intern("Struct")); + rb_global_variable(&thrift_struct_module); recursion_limit = FIX2INT(rb_const_get(thrift_module, rb_intern("DEFAULT_RECURSION_DEPTH"))); - rb_define_method(struct_module, "write", rb_thrift_struct_write, -1); - rb_define_method(struct_module, "read", rb_thrift_struct_read, -1); + rb_define_method(thrift_struct_module, "write", rb_thrift_struct_write, -1); + rb_define_method(thrift_struct_module, "read", rb_thrift_struct_read, -1); thrift_union_class = rb_const_get(thrift_module, rb_intern("Union")); rb_global_variable(&thrift_union_class); @@ -866,6 +888,7 @@ void Init_struct(void) { rb_global_variable(&sorted_field_ids_method_id); validate_container_size_method_id = rb_intern("validate_container_size"); + copy_default_value_method_id = rb_intern("copy_default_value"); default_sym = ID2SYM(rb_intern("default")); rb_global_variable(&default_sym); diff --git a/lib/rb/lib/thrift/struct.rb b/lib/rb/lib/thrift/struct.rb index e47308a99a4..48a06b49231 100644 --- a/lib/rb/lib/thrift/struct.rb +++ b/lib/rb/lib/thrift/struct.rb @@ -49,7 +49,7 @@ def initialize(d = {}, &block) # assign all the default values unless fields_with_defaults.empty? fields_with_defaults.each do |name, default_value| - instance_variable_set("@#{name}", (default_value.dup rescue default_value)) + instance_variable_set("@#{name}", Struct.copy_default_value(default_value)) end end @@ -85,23 +85,25 @@ def inspect(skip_optional_nulls = true) def read(iprot, remaining_depth = DEFAULT_RECURSION_DEPTH) raise ProtocolException.new(ProtocolException::DEPTH_LIMIT, "Maximum recursion depth exceeded") if remaining_depth <= 0 unless instance_variables.empty? - defaults = fields_with_default_values - struct_fields.each_value do |field_info| - instance_variable_set("@#{field_info[:name]}", nil) - end - defaults.each do |name, default_value| - instance_variable_set("@#{name}", (default_value.dup rescue default_value)) - end + unread_fields = struct_fields.dup + raise FrozenError, "can't modify frozen #{self.class}" if frozen? && !unread_fields.empty? end - iprot.read_struct_begin - loop do - _, ftype, fid = iprot.read_field_begin - break if (ftype == Types::STOP) - handle_message(iprot, fid, ftype, remaining_depth) - iprot.read_field_end + begin + iprot.read_struct_begin + loop do + _, ftype, fid = iprot.read_field_begin + break if (ftype == Types::STOP) + unread_fields&.delete(fid) if handle_message(iprot, fid, ftype, remaining_depth) + iprot.read_field_end + end + iprot.read_struct_end + ensure + # Reset omitted fields even after a failed read into a reused object. + unread_fields&.each_value do |field| + instance_variable_set("@#{field[:name]}", Struct.copy_default_value(field[:default])) + end end - iprot.read_struct_end validate end @@ -212,6 +214,40 @@ def <=>(other) end end + # Copy generated Thrift defaults, not arbitrary Ruby object graphs. + # Container subclasses and custom Hash behavior are not preserved; + # unrecognized objects are retained. + def self.copy_default_value(value) + case value + when Numeric + value + when String + value.dup + when Array + value.map { |element| copy_default_value(element) } + when Hash + value.each_with_object({}) do |(key, element), copy| + copy[copy_default_value(key)] = copy_default_value(element) + end + when Set + value.each_with_object(Set.new) { |element, copy| copy.add(copy_default_value(element)) } + when Struct, Union + copy = value.dup + if value.is_a?(Union) + copy.instance_variable_set(:@setfield, copy_default_value(value.get_set_field)) + copy.instance_variable_set(:@value, copy_default_value(value.get_value)) + else + value.struct_fields.each_value do |field_info| + ivar = "@#{field_info[:name]}" + copy.instance_variable_set(ivar, copy_default_value(value.instance_variable_get(ivar))) + end + end + copy + else + value + end + end + protected def self.append_features(mod) @@ -245,8 +281,11 @@ def handle_message(iprot, fid, ftype, remaining_depth) if field and field[:type] == ftype value = read_field(iprot, field, remaining_depth) instance_variable_set("@#{field[:name]}", value) + # A received field can legitimately result in false or nil. + true else iprot.skip(ftype) + false end end end diff --git a/lib/rb/spec/ThriftSpec.thrift b/lib/rb/spec/ThriftSpec.thrift index b78e71fa5f7..a56ebc0831e 100644 --- a/lib/rb/spec/ThriftSpec.thrift +++ b/lib/rb/spec/ThriftSpec.thrift @@ -85,6 +85,26 @@ struct Foo2 { 1: binary my_binary } +struct NestedDefaults { + 1: list> nested_list = [[1]] + 2: map> nested_map = {"key": ["value"]} + 3: set> nested_set = [[1]] + 4: Hello child = {"greeting": "child"} + 5: string text = "fixed" + 6: i32 scalar = 7 + 8: binary data = "binary" + 9: uuid token = "00112233-4455-6677-8899-aabbccddeeff" +} + +struct NestedDefaultsList { + 1: list values +} + +struct DefaultCopyNames { + 1: string copy_default_value = "literal" + 2: Hello copy_struct_default = {"greeting": "child"} +} + struct BoolStruct { 1: bool yesno = 1 } diff --git a/lib/rb/spec/struct_spec.rb b/lib/rb/spec/struct_spec.rb index 789d0d079d2..704ee33ba9e 100644 --- a/lib/rb/spec/struct_spec.rb +++ b/lib/rb/spec/struct_spec.rb @@ -83,6 +83,283 @@ def validate_default_arguments(object) SpecNamespace::Foo::FIELDS[4][:default] = [1, 2, 2, 3] end + it "isolates nested generated defaults between instances" do + first = SpecNamespace::NestedDefaults.new + second = SpecNamespace::NestedDefaults.new + + first.nested_list.first << 2 + first.nested_map["key"] << "changed" + first.nested_set.first << 2 + first.nested_set.reset + first.child.greeting << " changed" + first.text << " changed" + first.data << " changed" + first.token.replace("ffeeddcc-bbaa-9988-7766-554433221100") + + expect(first.nested_set.include?([1, 2])).to be(true) + expect(second.nested_list).to eq([[1]]) + expect(second.nested_map).to eq({"key" => ["value"]}) + expect(second.nested_set).to eq(Set.new([[1]])) + expect(second.child.greeting).to eq("child") + expect(second.text).to eq("fixed") + expect(second.data).to eq("binary") + expect(second.token).to eq("00112233-4455-6677-8899-aabbccddeeff") + expect(first.scalar).to equal(second.scalar) + end + + it "copies frozen containers and strings into mutable values" do + defaults = { + "list" => [[1].freeze].freeze, + "text" => "fixed", + } + + copy = Thrift::Struct.copy_default_value(defaults) + copy["list"].first << 2 + copy["text"] << " changed" + + expect(copy).to eq({"list" => [[1, 2]], "text" => "fixed changed"}) + expect(defaults).to eq({"list" => [[1]], "text" => "fixed"}) + end + + it "rejects a frozen defaulted target before consuming truncated input" do + buffer = Thrift::MemoryBufferTransport.new + protocol = Thrift::BinaryProtocol.new(buffer) + protocol.write_struct_begin("BoolStruct") + protocol.write_field_begin("yesno", Thrift::Types::BOOL, 1) + payload = buffer.read(buffer.available) + input = Thrift::MemoryBufferTransport.new(payload) + target = SpecNamespace::BoolStruct.new.freeze + + expect { target.read(Thrift::BinaryProtocol.new(input)) }.to raise_error(FrozenError) + + expect(input.available).to eq(payload.bytesize) + expect(target).to eq(SpecNamespace::BoolStruct.new) + end + + [Thrift::BinaryProtocol, Thrift::CompactProtocol, Thrift::JsonProtocol].each do |protocol_class| + describe "reading nested defaults with #{protocol_class}" do + def empty_nested_defaults(protocol_class) + transport = Thrift::MemoryBufferTransport.new + protocol = protocol_class.new(transport) + protocol.write_struct_begin("NestedDefaultsList") + protocol.write_field_begin("values", Thrift::Types::LIST, 1) + protocol.write_list_begin(Thrift::Types::STRUCT, 2) + 2.times do + protocol.write_struct_begin("NestedDefaults") + protocol.write_field_stop + protocol.write_struct_end + end + protocol.write_list_end + protocol.write_field_end + protocol.write_field_stop + protocol.write_struct_end + protocol + end + + it "isolates omitted defaults when reading nested values" do + protocol = empty_nested_defaults(protocol_class) + default = SpecNamespace::NestedDefaults::FIELDS[1][:default] + + result = SpecNamespace::NestedDefaultsList.new + result.read(protocol) + + first, second = result.values + first.nested_list.first << 2 + expect(second.nested_list).to eq([[1]]) + expect(default).to eq([[1]]) + end + + [[], [:nested_map, :child]].each do |omitted| + it "does not recopy defaults for received fields when #{omitted.inspect} are omitted" do + source = SpecNamespace::NestedDefaults.new + omitted.each { |name| source.public_send("#{name}=", nil) } + buffer = Thrift::MemoryBufferTransport.new + source.write(protocol_class.new(buffer)) + target = SpecNamespace::NestedDefaults.new + list_default = SpecNamespace::NestedDefaults::FIELDS[1][:default] + map_default = SpecNamespace::NestedDefaults::FIELDS[2][:default] + copies = {list: 0, map: 0} + allow(Thrift::Struct).to receive(:copy_default_value).and_wrap_original do |original, value| + copies[:list] += 1 if value.equal?(list_default) + copies[:map] += 1 if value.equal?(map_default) + original.call(value) + end + + target.read(protocol_class.new(buffer)) + + expect(copies).to eq({list: 0, map: omitted.include?(:nested_map) ? 1 : 0}) + expect(target.nested_list).to eq([[1]]) + expect(target.nested_map).to eq({"key" => ["value"]}) + expect(target.child.greeting).to eq("child") + end + end + + it "does not restore a default over a received container with mismatched elements" do + buffer = Thrift::MemoryBufferTransport.new + protocol = protocol_class.new(buffer) + protocol.write_struct_begin("NestedDefaults") + protocol.write_field_begin("nested_list", Thrift::Types::LIST, 1) + protocol.write_list_begin(Thrift::Types::STRING, 1) + protocol.write_string("ignored") + protocol.write_list_end + protocol.write_field_end + protocol.write_field_stop + protocol.write_struct_end + target = SpecNamespace::NestedDefaults.new + + target.read(protocol_class.new(buffer)) + + expect(target.nested_list).to be_nil + expect(target.nested_map).to eq({"key" => ["value"]}) + end + + it "resets unread fields after failure without discarding completed fields" do + buffer = Thrift::MemoryBufferTransport.new + protocol = protocol_class.new(buffer) + protocol.write_struct_begin("NestedDefaults") + protocol.write_field_begin("text", Thrift::Types::STRING, 5) + protocol.write_string("received") + protocol.write_field_end + protocol.write_field_begin("scalar", Thrift::Types::I32, 6) + target = SpecNamespace::NestedDefaults.new(scalar: 99) + old_list = target.nested_list + old_list.first << 99 + + expect { target.read(protocol_class.new(buffer)) }.to raise_error(EOFError) + + expect(target.text).to eq("received") + expect(target.scalar).to eq(7) + expect(target.nested_list).to eq([[1]]) + expect(target.nested_list).not_to equal(old_list) + end + + it "still resets fields changed by a custom initializer" do + custom_class = Class.new(SpecNamespace::NestedDefaults) do + def initialize + super + @nested_list.first << 99 + @text = "custom" + @custom_initialized = true + end + end + field = SpecNamespace::NestedDefaultsList::FIELDS.fetch(1) + stub_const("SpecNamespace::NestedDefaultsList::FIELDS", { + 1 => field.merge(element: field[:element].merge(class: custom_class)), + }) + + result = SpecNamespace::NestedDefaultsList.new + result.read(empty_nested_defaults(protocol_class)) + + result.values.each do |value| + expect(value.nested_list).to eq([[1]]) + expect(value.text).to eq("fixed") + expect(value.instance_variable_get(:@custom_initialized)).to be(true) + end + end + + it "preserves custom readers in the Ruby field reader" do + arguments = nil + custom_class = Class.new(SpecNamespace::NestedDefaults) do + define_method(:read) do |*args| + arguments = args + super(*args) + end + end + protocol = empty_nested_defaults(protocol_class) + protocol.read_struct_begin + protocol.read_field_begin + protocol.read_list_begin + + value = SpecNamespace::Foo.new.read_field(protocol, {type: Thrift::Types::STRUCT, class: custom_class}) + + expect(arguments).to eq([protocol, Thrift::DEFAULT_RECURSION_DEPTH - 1]) + expect(value.nested_list).to eq([[1]]) + end + + it "still resets pooled objects returned by custom constructors" do + custom_class = Class.new(SpecNamespace::NestedDefaults) + existing = custom_class.new + existing.nested_list.first << 99 + allow(custom_class).to receive(:new).and_return(existing) + protocol = empty_nested_defaults(protocol_class) + protocol.read_struct_begin + protocol.read_field_begin + protocol.read_list_begin + + value = SpecNamespace::Foo.new.read_field(protocol, {type: Thrift::Types::STRUCT, class: custom_class}) + + expect(value).to equal(existing) + expect(value.nested_list).to eq([[1]]) + end + end + end + + it "isolates generated union defaults between instances" do + first = SpecNamespace::StructWithUnionDefault.new + second = SpecNamespace::StructWithUnionDefault.new + + first.union_field.string_field << " changed" + expect(second.union_field.string_field).to eq("default union value") + expect(SpecNamespace::StructWithUnionDefault::FIELDS[1][:default].get_value).to eq("default union value") + end + + it "does not confuse generated field accessors with default-copy helpers" do + value = SpecNamespace::DefaultCopyNames.new + + expect(value.copy_default_value).to eq("literal") + expect(value.copy_struct_default.greeting).to eq("child") + end + + it "does not rerun nested struct constructors when copying defaults" do + expect(SpecNamespace::Hello).not_to receive(:new) + + expect(SpecNamespace::Foo.new.hello.greeting).to eq("hello, world!") + end + + it "keeps explicitly supplied mutable values owned by the caller" do + value = [[7]] + struct = SpecNamespace::NestedDefaults.new(nested_list: value) + + expect(struct.nested_list).to equal(value) + end + + [Thrift::BinaryProtocol, Thrift::CompactProtocol, Thrift::JsonProtocol].each do |protocol_class| + it "isolates omitted defaults on repeated #{protocol_class} reads" do + buffer = Thrift::MemoryBufferTransport.new + protocol = protocol_class.new(buffer) + protocol.write_struct_begin("NestedDefaults") + protocol.write_field_stop + protocol.write_struct_end + bytes = buffer.read(buffer.available) + + first = SpecNamespace::NestedDefaults.new + second = SpecNamespace::NestedDefaults.new + 2.times do + [first, second].each do |value| + value.read(protocol_class.new(Thrift::MemoryBufferTransport.new(bytes))) + end + expect(first.nested_list.first).not_to equal(second.nested_list.first) + expect(first.nested_map["key"]).not_to equal(second.nested_map["key"]) + expect(first.nested_set.first).not_to equal(second.nested_set.first) + expect(first.child.greeting).not_to equal(second.child.greeting) + expect(first.nested_list).to eq([[1]]) + + [:text, :data, :token].each do |name| + default = first.fields_with_default_values.fetch(name.to_s) + first_value = first.public_send(name) + second_value = second.public_send(name) + + expect(first_value).to eq(default) + expect(first_value.encoding).to eq(default.encoding) + expect(first_value).not_to equal(default) + expect(first_value).not_to equal(second_value) + first_value << " changed" + expect(second_value).to eq(default) + end + end + end + end + it "should properly initialize boolean values" do struct = SpecNamespace::BoolStruct.new(yesno: false) expect(struct.yesno).to be_falsey diff --git a/test/rb/benchmarks/README.md b/test/rb/benchmarks/README.md index b1eff1c8eb2..28b991ddf21 100644 --- a/test/rb/benchmarks/README.md +++ b/test/rb/benchmarks/README.md @@ -20,6 +20,10 @@ This runs the full benchmark set: - Header zlib - C binary, if `thrift_native.so` loads +The defaults jobs need generated test structs. Run `make stubs` in `test/rb` +to include them. Without these files, the default run skips those jobs with a +warning. Selecting a defaults job by ID instead reports how to build the files. + ## Options Use flags or env vars to tune a run. @@ -50,16 +54,18 @@ Use `--scenarios` or `THRIFT_BENCHMARK_SCENARIOS` to run only part of the matrix ruby test/rb/benchmarks/protocol_benchmark.rb --scenarios rb-bin-write-large,rb-json-read-large,hdr-zlib-read-small ``` -Each ID has four parts: +Each ID starts with three parts: - family: `rb`, `c`, or `hdr` - protocol: `bin`, `cmp`, `json`, or `zlib` - operation: `write` or `read` -- size: `large` or `small` + +The rest names the workload: `large`, `small`, `defaults-full`, or +`defaults-omitted`. ### Full Table -| ID | Family | Protocol | Operation | Size | +| ID | Family | Protocol | Operation | Workload | | --- | --- | --- | --- | --- | | `rb-bin-write-large` | Ruby | binary | write | large | | `rb-bin-read-large` | Ruby | binary | read | large | @@ -83,6 +89,12 @@ Each ID has four parts: | `hdr-cmp-read-small` | Header | compact | read | small | | `hdr-zlib-write-small` | Header | zlib | write | small | | `hdr-zlib-read-small` | Header | zlib | read | small | +| `rb-bin-read-defaults-full` | Ruby | binary | read | all fields present | +| `rb-bin-read-defaults-omitted` | Ruby | binary | read | two defaulted fields omitted | +| `c-bin-read-defaults-full` | C native | binary | read | all fields present | +| `c-bin-read-defaults-omitted` | C native | binary | read | two defaulted fields omitted | +| `rb-cmp-read-defaults-full` | Ruby | compact | read | all fields present | +| `rb-cmp-read-defaults-omitted` | Ruby | compact | read | two defaulted fields omitted | > [!NOTE] > Native-only IDs fail if `thrift_native.so` is not available. @@ -95,6 +107,18 @@ Each ID has four parts: - Small jobs serialize and deserialize many `OneOfEach` payloads. - Read jobs use payloads built before timing so they measure read cost, not payload construction. - Header jobs flush after each struct so reads benchmark framed messages, not a buffered write that was never emitted. +- Defaults jobs use the compiler-generated `Hello` from `test/SmallTest.thrift`. + Generate it with `make stubs` in `test/rb` before running these scenarios. + The full case sends all five fields, including the map and nested-struct defaults. + The omitted case sets those two defaulted fields to `nil`, so they are absent on + the wire and the reader copies their defaults again after construction. Both + cases deserialize `--small-runs` structs; serialization is outside timing. + +Run the defaults workload check with plain Ruby too: + +```sh +ruby test/rb/benchmarks/test_protocol_benchmark.rb +``` ### Files diff --git a/test/rb/benchmarks/protocol_benchmark.rb b/test/rb/benchmarks/protocol_benchmark.rb index 71d530b2eb1..87d85497888 100644 --- a/test/rb/benchmarks/protocol_benchmark.rb +++ b/test/rb/benchmarks/protocol_benchmark.rb @@ -64,9 +64,13 @@ module ProtocolBenchmark rb-cmp-write-small rb-cmp-read-small rb-json-write-small rb-json-read-small hdr-bin-write-small hdr-bin-read-small hdr-cmp-write-small hdr-cmp-read-small hdr-zlib-write-small hdr-zlib-read-small + rb-bin-read-defaults-full rb-bin-read-defaults-omitted + c-bin-read-defaults-full c-bin-read-defaults-omitted + rb-cmp-read-defaults-full rb-cmp-read-defaults-omitted ].freeze NATIVE_SCENARIO_IDS = %w[ c-bin-write-large c-bin-read-large c-bin-write-small c-bin-read-small + c-bin-read-defaults-full c-bin-read-defaults-omitted ].freeze module_function @@ -331,6 +335,32 @@ def build_scenarios(large_runs:, small_runs:, scenario_ids: nil) scenario("hdr-zlib-read-small", "header zlib read #{small_runs} small structures") { deserialize(header_zlib, Fixtures::Structs::OneOfEach, header_zlib_payload, count: small_runs) }, ] + defaults_ids = ALL_SCENARIO_IDS.grep(/-read-defaults-/) + with_scenario_selected(scenario_ids, *defaults_ids) do + fixture = File.expand_path("../gen-rb/small_test_types.rb", __dir__) + unless File.file?(fixture) + raise ArgumentError, "defaulted struct benchmarks require generated fixtures; run `make stubs` in test/rb first." if scenario_ids + warn "Skipping defaulted struct benchmarks: run `make stubs` in test/rb first." + next + end + require fixture + full = TestNamespace::Hello.new(complexer: {1 => {2 => 3}}) + omitted = TestNamespace::Hello.new(complex: nil, thinz: nil, complexer: {1 => {2 => 3}}) + builders = {"rb-bin" => ruby_binary, "rb-cmp" => ruby_compact} + builders["c-bin"] = accelerated_binary if native_available + builders.each do |family, builder| + {"full" => full, "omitted" => omitted}.each do |shape, value| + id = "#{family}-read-defaults-#{shape}" + with_scenario_selected(scenario_ids, id) do + payload = serialize(builder, value, count: small_runs) + scenario_list << scenario(id, "#{family} read #{small_runs} defaulted structs (#{shape})") do + deserialize(builder, TestNamespace::Hello, payload, count: small_runs) + end + end + end + end + end + select_scenarios(scenario_list, scenario_ids, native_available: native_available) end diff --git a/test/rb/benchmarks/test_protocol_benchmark.rb b/test/rb/benchmarks/test_protocol_benchmark.rb new file mode 100644 index 00000000000..7136c8c9d39 --- /dev/null +++ b/test/rb/benchmarks/test_protocol_benchmark.rb @@ -0,0 +1,105 @@ +# frozen_string_literal: true +# +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +# + +require File.expand_path("../test_helper", __dir__) +require "open3" +require_relative "protocol_benchmark" + +class ProtocolBenchmarkTest < Test::Unit::TestCase + def test_defaulted_struct_read_scenarios + ids = ProtocolBenchmark::ALL_SCENARIO_IDS.grep(/-read-defaults-/) + ids -= ProtocolBenchmark::NATIVE_SCENARIO_IDS unless ProtocolBenchmark.native_available? + scenarios = ProtocolBenchmark.build_scenarios(large_runs: 1, small_runs: 2, scenario_ids: ids) + expected = TestNamespace::Hello.new(complexer: {1 => {2 => 3}}) + + assert_equal ids.sort, scenarios.map { |entry| entry[:id] }.sort + scenarios.each do |entry| + value = entry[:job].call + second = entry[:job].call + assert_equal expected, value, entry[:id] + assert_equal expected, second, entry[:id] + assert_not_same TestNamespace::Hello::FIELDS[2][:default], value.complex, entry[:id] + assert_not_same TestNamespace::Hello::FIELDS[5][:default], value.thinz, entry[:id] + assert_not_same second.complex, value.complex, entry[:id] + assert_not_same second.thinz, value.thinz, entry[:id] + value.complex.clear + value.thinz.val = -1 + assert_equal expected, second, entry[:id] + assert_equal expected.complex, TestNamespace::Hello::FIELDS[2][:default], entry[:id] + assert_equal expected.thinz, TestNamespace::Hello::FIELDS[5][:default], entry[:id] + end + end + + def test_default_run_skips_missing_default_fixtures + stdout, stderr, status = without_default_fixtures(nil) + + assert_predicate status, :success?, stderr + ids = JSON.parse(stdout) + assert_include ids, "rb-bin-read-small" + assert_empty ids.grep(/-read-defaults-/) + assert_equal "Skipping defaulted struct benchmarks: run `make stubs` in test/rb first.\n", stderr + end + + def test_explicit_default_scenario_explains_missing_fixtures + stdout, stderr, status = without_default_fixtures(["rb-bin-read-defaults-full"]) + + assert_predicate status, :success?, stderr + assert_equal ["ArgumentError", "defaulted struct benchmarks require generated fixtures; run `make stubs` in test/rb first."], JSON.parse(stdout) + assert_empty stderr + end + + def test_other_scenarios_do_not_need_default_fixtures + stdout, stderr, status = without_default_fixtures(["rb-bin-read-small"]) + + assert_predicate status, :success?, stderr + assert_equal ["rb-bin-read-small"], JSON.parse(stdout) + assert_empty stderr + end + + private + + def without_default_fixtures(ids) + # Isolate the missing-file simulation; do not move generated files in the checkout. + script = <<~'RUBY' + require "json" + if ENV["THRIFT_BENCHMARK_SKIP_NATIVE"] == "1" + $LOADED_FEATURES << File.expand_path("../../../lib/rb/lib/thrift/thrift_native.rb", ARGV[0]) + end + fixture = File.expand_path("../gen-rb/small_test_types.rb", ARGV[0]) + File.singleton_class.prepend(Module.new do + define_method(:file?) { |path| path == fixture ? false : super(path) } + end) + Kernel.prepend(Module.new do + define_method(:require) do |path| + raise LoadError, "cannot load such file -- #{path}" if path == fixture || "#{path}.rb" == fixture + super(path) + end + end) + require File.join(ARGV[0], "protocol_benchmark") + begin + scenarios = ProtocolBenchmark.build_scenarios(large_runs: 1, small_runs: 1, scenario_ids: JSON.parse(ARGV[1])) + puts JSON.generate(scenarios.map { |entry| entry[:id] }) + rescue ArgumentError => error + puts JSON.generate([error.class.name, error.message]) + end + RUBY + Open3.capture3(RbConfig.ruby, "-e", script, __dir__, JSON.generate(ids)) + end +end