From 0f76f8a83bd12c2395bc27c4b3931727a8eef2ba Mon Sep 17 00:00:00 2001 From: Joshua Haberman Date: Thu, 15 Aug 2019 03:37:06 -0700 Subject: [PATCH 1/3] Put oneof case offset in separate oneof table. --- ruby/ext/google/protobuf_c/encode_decode.c | 27 +++---- ruby/ext/google/protobuf_c/message.c | 17 +--- ruby/ext/google/protobuf_c/protobuf.h | 16 ++-- ruby/ext/google/protobuf_c/storage.c | 94 ++++++++++++---------- 4 files changed, 79 insertions(+), 75 deletions(-) diff --git a/ruby/ext/google/protobuf_c/encode_decode.c b/ruby/ext/google/protobuf_c/encode_decode.c index 40952c6b1b..9e178c6168 100644 --- a/ruby/ext/google/protobuf_c/encode_decode.c +++ b/ruby/ext/google/protobuf_c/encode_decode.c @@ -738,12 +738,13 @@ static void add_handlers_for_message(const void *closure, upb_handlers *h) { !upb_msg_field_done(&i); upb_msg_field_next(&i)) { const upb_fielddef *f = upb_msg_iter_field(&i); + const upb_oneofdef *oneof = upb_fielddef_containingoneof(f); size_t offset = desc->layout->fields[upb_fielddef_index(f)].offset + sizeof(MessageHeader); - if (upb_fielddef_containingoneof(f)) { + if (oneof) { size_t oneof_case_offset = - desc->layout->fields[upb_fielddef_index(f)].case_offset + + desc->layout->oneofs[upb_oneofdef_index(oneof)].case_offset + sizeof(MessageHeader); add_handlers_for_oneof_field(h, f, offset, oneof_case_offset); } else if (is_map_field(f)) { @@ -1311,19 +1312,18 @@ static void putmsg(VALUE msg_rb, const Descriptor* desc, !upb_msg_field_done(&i); upb_msg_field_next(&i)) { upb_fielddef *f = upb_msg_iter_field(&i); + const upb_oneofdef *oneof = upb_fielddef_containingoneof(f); bool is_matching_oneof = false; uint32_t offset = desc->layout->fields[upb_fielddef_index(f)].offset + sizeof(MessageHeader); - if (upb_fielddef_containingoneof(f)) { - uint32_t oneof_case_offset = - desc->layout->fields[upb_fielddef_index(f)].case_offset + - sizeof(MessageHeader); + if (oneof) { + uint32_t oneof_case = + slot_read_oneof_case(desc->layout, Message_data(msg), oneof); // For a oneof, check that this field is actually present -- skip all the // below if not. - if (DEREF(msg, oneof_case_offset, uint32_t) != - upb_fielddef_number(f)) { + if (oneof_case != upb_fielddef_number(f)) { continue; } // Otherwise, fall through to the appropriate singular-field handler @@ -1543,18 +1543,17 @@ static void discard_unknown(VALUE msg_rb, const Descriptor* desc) { !upb_msg_field_done(&it); upb_msg_field_next(&it)) { upb_fielddef *f = upb_msg_iter_field(&it); + const upb_oneofdef *oneof = upb_fielddef_containingoneof(f); uint32_t offset = desc->layout->fields[upb_fielddef_index(f)].offset + sizeof(MessageHeader); - if (upb_fielddef_containingoneof(f)) { - uint32_t oneof_case_offset = - desc->layout->fields[upb_fielddef_index(f)].case_offset + - sizeof(MessageHeader); + if (oneof) { + uint32_t oneof_case = + slot_read_oneof_case(desc->layout, Message_data(msg), oneof); // For a oneof, check that this field is actually present -- skip all the // below if not. - if (DEREF(msg, oneof_case_offset, uint32_t) != - upb_fielddef_number(f)) { + if (oneof_case != upb_fielddef_number(f)) { continue; } // Otherwise, fall through to the appropriate singular-field handler diff --git a/ruby/ext/google/protobuf_c/message.c b/ruby/ext/google/protobuf_c/message.c index fd82d863b9..1f9a5697dd 100644 --- a/ruby/ext/google/protobuf_c/message.c +++ b/ruby/ext/google/protobuf_c/message.c @@ -86,21 +86,8 @@ static const upb_fielddef* which_oneof_field(MessageHeader* self, const upb_oneo const upb_fielddef* first_field; const upb_fielddef* f; - // If no fields in the oneof, always nil. - if (upb_oneofdef_numfields(o) == 0) { - return NULL; - } - // Grab the first field in the oneof so we can get its layout info to find the - // oneof_case field. - upb_oneof_begin(&it, o); - assert(!upb_oneof_done(&it)); - first_field = upb_oneof_iter_field(&it); - assert(upb_fielddef_containingoneof(first_field) != NULL); - - case_ofs = - self->descriptor->layout-> - fields[upb_fielddef_index(first_field)].case_offset; - oneof_case = *((uint32_t*)((char*)Message_data(self) + case_ofs)); + oneof_case = + slot_read_oneof_case(self->descriptor->layout, Message_data(self), o); if (oneof_case == ONEOF_CASE_NONE) { return NULL; diff --git a/ruby/ext/google/protobuf_c/protobuf.h b/ruby/ext/google/protobuf_c/protobuf.h index db9c1eac0e..7fe2fd957a 100644 --- a/ruby/ext/google/protobuf_c/protobuf.h +++ b/ruby/ext/google/protobuf_c/protobuf.h @@ -59,6 +59,7 @@ typedef struct OneofDescriptor OneofDescriptor; typedef struct EnumDescriptor EnumDescriptor; typedef struct MessageLayout MessageLayout; typedef struct MessageField MessageField; +typedef struct MessageOneof MessageOneof; typedef struct MessageHeader MessageHeader; typedef struct MessageBuilderContext MessageBuilderContext; typedef struct OneofBuilderContext OneofBuilderContext; @@ -363,6 +364,8 @@ bool native_slot_eq(upb_fieldtype_t type, void* mem1, void* mem2); VALUE native_slot_encode_and_freeze_string(upb_fieldtype_t type, VALUE value); void native_slot_check_int_range_precision(const char* name, upb_fieldtype_t type, VALUE value); +uint32_t slot_read_oneof_case(MessageLayout* layout, const void* storage, + const upb_oneofdef* oneof); extern rb_encoding* kRubyStringUtf8Encoding; extern rb_encoding* kRubyStringASCIIEncoding; @@ -492,18 +495,21 @@ VALUE Map_iter_value(Map_iter* iter); // Message layout / storage. // ----------------------------------------------------------------------------- -#define MESSAGE_FIELD_NO_CASE ((size_t)-1) -#define MESSAGE_FIELD_NO_HASBIT ((size_t)-1) +#define MESSAGE_FIELD_NO_HASBIT ((uint32_t)-1) struct MessageField { - size_t offset; - size_t case_offset; // for oneofs, a uint32. Else, MESSAGE_FIELD_NO_CASE. - size_t hasbit; + uint32_t offset; + uint32_t hasbit; +}; + +struct MessageOneof { + uint32_t case_offset; }; struct MessageLayout { const upb_msgdef* msgdef; MessageField* fields; + MessageOneof* oneofs; size_t size; }; diff --git a/ruby/ext/google/protobuf_c/storage.c b/ruby/ext/google/protobuf_c/storage.c index e9c70a2827..9abdd61dae 100644 --- a/ruby/ext/google/protobuf_c/storage.c +++ b/ruby/ext/google/protobuf_c/storage.c @@ -465,11 +465,17 @@ static size_t align_up_to(size_t offset, size_t granularity) { MessageLayout* create_layout(const upb_msgdef* msgdef) { MessageLayout* layout = ALLOC(MessageLayout); int nfields = upb_msgdef_numfields(msgdef); + int noneofs = upb_msgdef_numoneofs(msgdef); upb_msg_field_iter it; upb_msg_oneof_iter oit; size_t off = 0; layout->fields = ALLOC_N(MessageField, nfields); + layout->oneofs = NULL; + + if (noneofs > 0) { + layout->oneofs = ALLOC_N(MessageOneof, noneofs); + } size_t hasbit = 0; for (upb_msg_field_begin(&it, msgdef); @@ -480,7 +486,7 @@ MessageLayout* create_layout(const upb_msgdef* msgdef) { layout->fields[upb_fielddef_index(field)].hasbit = hasbit++; } else { layout->fields[upb_fielddef_index(field)].hasbit = - MESSAGE_FIELD_NO_HASBIT; + MESSAGE_FIELD_NO_HASBIT; } } @@ -509,8 +515,6 @@ MessageLayout* create_layout(const upb_msgdef* msgdef) { // Align current offset up to |size| granularity. off = align_up_to(off, field_size); layout->fields[upb_fielddef_index(field)].offset = off; - layout->fields[upb_fielddef_index(field)].case_offset = - MESSAGE_FIELD_NO_CASE; off += field_size; } @@ -558,13 +562,7 @@ MessageLayout* create_layout(const upb_msgdef* msgdef) { size_t field_size = sizeof(uint32_t); // Align the offset. off = (off + field_size - 1) & ~(field_size - 1); - // Assign all fields in the oneof this same offset. - for (upb_oneof_begin(&fit, oneof); - !upb_oneof_done(&fit); - upb_oneof_next(&fit)) { - const upb_fielddef* field = upb_oneof_iter_field(&fit); - layout->fields[upb_fielddef_index(field)].case_offset = off; - } + layout->oneofs[upb_oneofdef_index(oneof)].case_offset = off; off += field_size; } @@ -578,6 +576,7 @@ MessageLayout* create_layout(const upb_msgdef* msgdef) { void free_layout(MessageLayout* layout) { xfree(layout->fields); + xfree(layout->oneofs); upb_msgdef_unref(layout->msgdef, &layout->msgdef); xfree(layout); } @@ -605,9 +604,15 @@ static void* slot_memory(MessageLayout* layout, static uint32_t* slot_oneof_case(MessageLayout* layout, const void* storage, - const upb_fielddef* field) { - return (uint32_t *)(((uint8_t *)storage) + - layout->fields[upb_fielddef_index(field)].case_offset); + const upb_oneofdef* oneof) { + return (uint32_t*)(((uint8_t*)storage) + + layout->oneofs[upb_oneofdef_index(oneof)].case_offset); +} + +uint32_t slot_read_oneof_case(MessageLayout* layout, const void* storage, + const upb_oneofdef* oneof) { + uint32_t* ptr = slot_oneof_case(layout, storage, oneof); + return *ptr; } static void slot_set_hasbit(MessageLayout* layout, @@ -650,13 +655,14 @@ void layout_clear(MessageLayout* layout, const void* storage, const upb_fielddef* field) { void* memory = slot_memory(layout, storage, field); - uint32_t* oneof_case = slot_oneof_case(layout, storage, field); + const upb_oneofdef* oneof = upb_fielddef_containingoneof(field); if (field_contains_hasbit(layout, field)) { slot_clear_hasbit(layout, storage, field); } - if (upb_fielddef_containingoneof(field)) { + if (oneof) { + uint32_t* oneof_case = slot_oneof_case(layout, storage, oneof); memset(memory, 0, NATIVE_SLOT_MAX_SIZE); *oneof_case = ONEOF_CASE_NONE; } else if (is_map_field(field)) { @@ -742,8 +748,7 @@ VALUE layout_get(MessageLayout* layout, const void* storage, const upb_fielddef* field) { void* memory = slot_memory(layout, storage, field); - uint32_t* oneof_case = slot_oneof_case(layout, storage, field); - + const upb_oneofdef* oneof = upb_fielddef_containingoneof(field); bool field_set; if (field_contains_hasbit(layout, field)) { field_set = slot_is_hasbit_set(layout, storage, field); @@ -751,8 +756,9 @@ VALUE layout_get(MessageLayout* layout, field_set = true; } - if (upb_fielddef_containingoneof(field)) { - if (*oneof_case != upb_fielddef_number(field)) { + if (oneof) { + uint32_t oneof_case = slot_read_oneof_case(layout, storage, oneof); + if (oneof_case != upb_fielddef_number(field)) { return layout_get_default(field); } return native_slot_get(upb_fielddef_type(field), @@ -834,9 +840,10 @@ void layout_set(MessageLayout* layout, const upb_fielddef* field, VALUE val) { void* memory = slot_memory(layout, storage, field); - uint32_t* oneof_case = slot_oneof_case(layout, storage, field); + const upb_oneofdef* oneof = upb_fielddef_containingoneof(field); - if (upb_fielddef_containingoneof(field)) { + if (oneof) { + uint32_t* oneof_case = slot_oneof_case(layout, storage, oneof); if (val == Qnil) { // Assigning nil to a oneof field clears the oneof completely. *oneof_case = ONEOF_CASE_NONE; @@ -895,11 +902,12 @@ void layout_mark(MessageLayout* layout, void* storage) { !upb_msg_field_done(&it); upb_msg_field_next(&it)) { const upb_fielddef* field = upb_msg_iter_field(&it); + const upb_oneofdef* oneof = upb_fielddef_containingoneof(field); void* memory = slot_memory(layout, storage, field); - uint32_t* oneof_case = slot_oneof_case(layout, storage, field); - if (upb_fielddef_containingoneof(field)) { - if (*oneof_case == upb_fielddef_number(field)) { + if (oneof) { + uint32_t oneof_case = slot_read_oneof_case(layout, storage, oneof); + if (oneof_case == upb_fielddef_number(field)) { native_slot_mark(upb_fielddef_type(field), memory); } } else if (upb_fielddef_label(field) == UPB_LABEL_REPEATED) { @@ -916,14 +924,16 @@ void layout_dup(MessageLayout* layout, void* to, void* from) { !upb_msg_field_done(&it); upb_msg_field_next(&it)) { const upb_fielddef* field = upb_msg_iter_field(&it); + const upb_oneofdef* oneof = upb_fielddef_containingoneof(field); void* to_memory = slot_memory(layout, to, field); - uint32_t* to_oneof_case = slot_oneof_case(layout, to, field); void* from_memory = slot_memory(layout, from, field); - uint32_t* from_oneof_case = slot_oneof_case(layout, from, field); - if (upb_fielddef_containingoneof(field)) { - if (*from_oneof_case == upb_fielddef_number(field)) { + if (oneof) { + uint32_t* to_oneof_case = slot_oneof_case(layout, to, oneof); + uint32_t* from_oneof_case = slot_oneof_case(layout, from, oneof); + if (slot_read_oneof_case(layout, from, oneof) == + upb_fielddef_number(field)) { *to_oneof_case = *from_oneof_case; native_slot_dup(upb_fielddef_type(field), to_memory, from_memory); } @@ -948,14 +958,16 @@ void layout_deep_copy(MessageLayout* layout, void* to, void* from) { !upb_msg_field_done(&it); upb_msg_field_next(&it)) { const upb_fielddef* field = upb_msg_iter_field(&it); + const upb_oneofdef* oneof = upb_fielddef_containingoneof(field); void* to_memory = slot_memory(layout, to, field); - uint32_t* to_oneof_case = slot_oneof_case(layout, to, field); void* from_memory = slot_memory(layout, from, field); - uint32_t* from_oneof_case = slot_oneof_case(layout, from, field); - if (upb_fielddef_containingoneof(field)) { - if (*from_oneof_case == upb_fielddef_number(field)) { + if (oneof) { + uint32_t* to_oneof_case = slot_oneof_case(layout, to, oneof); + uint32_t* from_oneof_case = slot_oneof_case(layout, from, oneof); + if (slot_read_oneof_case(layout, from, oneof) == + upb_fielddef_number(field)) { *to_oneof_case = *from_oneof_case; native_slot_deep_copy(upb_fielddef_type(field), to_memory, from_memory); } @@ -982,17 +994,18 @@ VALUE layout_eq(MessageLayout* layout, void* msg1, void* msg2) { !upb_msg_field_done(&it); upb_msg_field_next(&it)) { const upb_fielddef* field = upb_msg_iter_field(&it); + const upb_oneofdef* oneof = upb_fielddef_containingoneof(field); void* msg1_memory = slot_memory(layout, msg1, field); - uint32_t* msg1_oneof_case = slot_oneof_case(layout, msg1, field); void* msg2_memory = slot_memory(layout, msg2, field); - uint32_t* msg2_oneof_case = slot_oneof_case(layout, msg2, field); - if (upb_fielddef_containingoneof(field)) { + if (oneof) { + uint32_t* msg1_oneof_case = slot_oneof_case(layout, msg1, oneof); + uint32_t* msg2_oneof_case = slot_oneof_case(layout, msg2, oneof); if (*msg1_oneof_case != *msg2_oneof_case || - (*msg1_oneof_case == upb_fielddef_number(field) && - !native_slot_eq(upb_fielddef_type(field), - msg1_memory, + (slot_read_oneof_case(layout, msg1, oneof) == + upb_fielddef_number(field) && + !native_slot_eq(upb_fielddef_type(field), msg1_memory, msg2_memory))) { return Qfalse; } @@ -1008,9 +1021,8 @@ VALUE layout_eq(MessageLayout* layout, void* msg1, void* msg2) { } } else { if (slot_is_hasbit_set(layout, msg1, field) != - slot_is_hasbit_set(layout, msg2, field) || - !native_slot_eq(upb_fielddef_type(field), - msg1_memory, msg2_memory)) { + slot_is_hasbit_set(layout, msg2, field) || + !native_slot_eq(upb_fielddef_type(field), msg1_memory, msg2_memory)) { return Qfalse; } } From 013a0ea8822a1de95b4076a90dc71f2be8bf31db Mon Sep 17 00:00:00 2001 From: Joshua Haberman Date: Thu, 15 Aug 2019 06:39:38 -0700 Subject: [PATCH 2/3] Optimized layout_mark to not iterate over the msgdef. --- ruby/ext/google/protobuf_c/protobuf.h | 7 ++- ruby/ext/google/protobuf_c/storage.c | 77 +++++++++++++++++---------- 2 files changed, 54 insertions(+), 30 deletions(-) diff --git a/ruby/ext/google/protobuf_c/protobuf.h b/ruby/ext/google/protobuf_c/protobuf.h index 93ec399712..0925185e54 100644 --- a/ruby/ext/google/protobuf_c/protobuf.h +++ b/ruby/ext/google/protobuf_c/protobuf.h @@ -507,6 +507,7 @@ struct MessageField { }; struct MessageOneof { + uint32_t offset; uint32_t case_offset; }; @@ -516,9 +517,13 @@ struct MessageLayout { const upb_msgdef* msgdef; MessageField* fields; MessageOneof* oneofs; - size_t size; + uint32_t size; + uint32_t value_offset; + int value_count; }; +#define ONEOF_CASE_MASK 0x80000000 + MessageLayout* create_layout(const Descriptor* desc); void free_layout(MessageLayout* layout); bool field_contains_hasbit(MessageLayout* layout, diff --git a/ruby/ext/google/protobuf_c/storage.c b/ruby/ext/google/protobuf_c/storage.c index 428fb86a1c..25628ed0fe 100644 --- a/ruby/ext/google/protobuf_c/storage.c +++ b/ruby/ext/google/protobuf_c/storage.c @@ -473,6 +473,11 @@ static size_t align_up_to(size_t offset, size_t granularity) { return (offset + granularity - 1) & ~(granularity - 1); } +static bool is_value_field(const upb_fielddef* f) { + return upb_fielddef_isseq(f) || upb_fielddef_issubmsg(f) || + upb_fielddef_isstring(f); +} + MessageLayout* create_layout(const Descriptor* desc) { const upb_msgdef *msgdef = desc->msgdef; MessageLayout* layout = ALLOC(MessageLayout); @@ -507,24 +512,38 @@ MessageLayout* create_layout(const Descriptor* desc) { off += (hasbit + 8 - 1) / 8; } + off = align_up_to(off, sizeof(VALUE)); + layout->value_offset = off; + layout->value_count = 0; + + // Place all (non-oneof) VALUE fields first. + for (upb_msg_field_begin(&it, msgdef); + !upb_msg_field_done(&it); + upb_msg_field_next(&it)) { + const upb_fielddef* field = upb_msg_iter_field(&it); + if (upb_fielddef_containingoneof(field) || !is_value_field(field)) { + continue; + } + + layout->fields[upb_fielddef_index(field)].offset = off; + off += sizeof(VALUE); + layout->value_count++; + } + + // Now place all other (non-oneof) fields. for (upb_msg_field_begin(&it, msgdef); !upb_msg_field_done(&it); upb_msg_field_next(&it)) { const upb_fielddef* field = upb_msg_iter_field(&it); size_t field_size; - if (upb_fielddef_containingoneof(field)) { - // Oneofs are handled separately below. + if (upb_fielddef_containingoneof(field) || is_value_field(field)) { continue; } // Allocate |field_size| bytes for this field in the layout. - field_size = 0; - if (upb_fielddef_label(field) == UPB_LABEL_REPEATED) { - field_size = sizeof(VALUE); - } else { - field_size = native_slot_size(upb_fielddef_type(field)); - } + field_size = native_slot_size(upb_fielddef_type(field)); + // Align current offset up to |size| granularity. off = align_up_to(off, field_size); layout->fields[upb_fielddef_index(field)].offset = off; @@ -561,6 +580,7 @@ MessageLayout* create_layout(const Descriptor* desc) { upb_oneof_next(&fit)) { const upb_fielddef* field = upb_oneof_iter_field(&fit); layout->fields[upb_fielddef_index(field)].offset = off; + layout->oneofs[upb_oneofdef_index(oneof)].offset = off; } off += field_size; } @@ -620,7 +640,7 @@ static uint32_t* slot_oneof_case(MessageLayout* layout, uint32_t slot_read_oneof_case(MessageLayout* layout, const void* storage, const upb_oneofdef* oneof) { uint32_t* ptr = slot_oneof_case(layout, storage, oneof); - return *ptr; + return *ptr & ~ONEOF_CASE_MASK; } static void slot_set_hasbit(MessageLayout* layout, @@ -850,11 +870,14 @@ void layout_set(MessageLayout* layout, // sync with the value slot whenever the Ruby VM has been called. Thus, we // use native_slot_set_value_and_case(), which ensures that both the value // and case number are altered atomically (w.r.t. the Ruby VM). + uint32_t case_value = upb_fielddef_number(field); + if (upb_fielddef_issubmsg(field) || upb_fielddef_isstring(field)) { + case_value |= ONEOF_CASE_MASK; + } + native_slot_set_value_and_case( - upb_fielddef_name(field), - upb_fielddef_type(field), field_type_class(layout, field), - memory, val, - oneof_case, upb_fielddef_number(field)); + upb_fielddef_name(field), upb_fielddef_type(field), + field_type_class(layout, field), memory, val, oneof_case, case_value); } } else if (is_map_field(field)) { check_map_field_type(layout, val, field); @@ -885,23 +908,19 @@ void layout_init(MessageLayout* layout, } void layout_mark(MessageLayout* layout, void* storage) { - upb_msg_field_iter it; - for (upb_msg_field_begin(&it, layout->msgdef); - !upb_msg_field_done(&it); - upb_msg_field_next(&it)) { - const upb_fielddef* field = upb_msg_iter_field(&it); - const upb_oneofdef* oneof = upb_fielddef_containingoneof(field); - void* memory = slot_memory(layout, storage, field); + VALUE* values = (VALUE*)CHARPTR_AT(storage, layout->value_offset); + int noneofs = upb_msgdef_numoneofs(layout->msgdef); + int i; - if (oneof) { - uint32_t oneof_case = slot_read_oneof_case(layout, storage, oneof); - if (oneof_case == upb_fielddef_number(field)) { - native_slot_mark(upb_fielddef_type(field), memory); - } - } else if (upb_fielddef_label(field) == UPB_LABEL_REPEATED) { - rb_gc_mark(DEREF(memory, VALUE)); - } else { - native_slot_mark(upb_fielddef_type(field), memory); + for (i = 0; i < layout->value_count; i++) { + rb_gc_mark(values[i]); + } + + for (i = 0; i < noneofs; i++) { + MessageOneof* oneof = &layout->oneofs[i]; + uint32_t* case_ptr = (uint32_t*)CHARPTR_AT(storage, oneof->case_offset); + if (*case_ptr & ONEOF_CASE_MASK) { + rb_gc_mark(DEREF_OFFSET(storage, oneof->offset, VALUE)); } } } From c02a6fbf2c2af8e82d917af6afc7ac4e4f73d9e2 Mon Sep 17 00:00:00 2001 From: Joshua Haberman Date: Fri, 16 Aug 2019 09:37:36 -0700 Subject: [PATCH 3/3] Bugfix for GC mark of oneof fields. --- ruby/ext/google/protobuf_c/encode_decode.c | 3 +++ ruby/ext/google/protobuf_c/protobuf.h | 1 + ruby/ext/google/protobuf_c/storage.c | 2 +- 3 files changed, 5 insertions(+), 1 deletion(-) diff --git a/ruby/ext/google/protobuf_c/encode_decode.c b/ruby/ext/google/protobuf_c/encode_decode.c index f4d3de2543..ad3d554706 100644 --- a/ruby/ext/google/protobuf_c/encode_decode.c +++ b/ruby/ext/google/protobuf_c/encode_decode.c @@ -155,6 +155,9 @@ static const void *newoneofhandlerdata(upb_handlers *h, // create a separate ID space. In addition, using the field tag number here // lets us easily look up the field in the oneof accessor. hd->oneof_case_num = upb_fielddef_number(f); + if (is_value_field(f)) { + hd->oneof_case_num |= ONEOF_CASE_MASK; + } hd->subklass = field_type_class(desc->layout, f); upb_handlers_addcleanup(h, hd, xfree); return hd; diff --git a/ruby/ext/google/protobuf_c/protobuf.h b/ruby/ext/google/protobuf_c/protobuf.h index 0925185e54..b8aed5a47f 100644 --- a/ruby/ext/google/protobuf_c/protobuf.h +++ b/ruby/ext/google/protobuf_c/protobuf.h @@ -370,6 +370,7 @@ VALUE native_slot_encode_and_freeze_string(upb_fieldtype_t type, VALUE value); void native_slot_check_int_range_precision(const char* name, upb_fieldtype_t type, VALUE value); uint32_t slot_read_oneof_case(MessageLayout* layout, const void* storage, const upb_oneofdef* oneof); +bool is_value_field(const upb_fielddef* f); extern rb_encoding* kRubyStringUtf8Encoding; extern rb_encoding* kRubyStringASCIIEncoding; diff --git a/ruby/ext/google/protobuf_c/storage.c b/ruby/ext/google/protobuf_c/storage.c index 25628ed0fe..e72fa10df4 100644 --- a/ruby/ext/google/protobuf_c/storage.c +++ b/ruby/ext/google/protobuf_c/storage.c @@ -473,7 +473,7 @@ static size_t align_up_to(size_t offset, size_t granularity) { return (offset + granularity - 1) & ~(granularity - 1); } -static bool is_value_field(const upb_fielddef* f) { +bool is_value_field(const upb_fielddef* f) { return upb_fielddef_isseq(f) || upb_fielddef_issubmsg(f) || upb_fielddef_isstring(f); }