Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Optimized layout_mark() for Ruby #6521

Merged
merged 4 commits into from Aug 20, 2019
Merged

Optimized layout_mark() for Ruby #6521

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0f76f8a
Put oneof case offset in separate oneof table.
haberman Aug 15, 2019
0088a75
Merge branch 'master' into layout_mark
haberman Aug 15, 2019
013a0ea
Optimized layout_mark to not iterate over the msgdef.
haberman Aug 15, 2019
c02a6fb
Bugfix for GC mark of oneof fields.
haberman Aug 16, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
30 changes: 16 additions & 14 deletions ruby/ext/google/protobuf_c/encode_decode.c
View file
Open in desktop
Expand Up @@ -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;
Expand Down Expand Up @@ -706,12 +709,13 @@ 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, desc);
} else if (is_map_field(f)) {
Expand Down Expand Up @@ -1256,19 +1260,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
Expand Down Expand Up @@ -1464,18 +1467,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
Expand Down
20 changes: 2 additions & 18 deletions ruby/ext/google/protobuf_c/message.c
View file
Open in desktop
Expand Up @@ -86,27 +86,11 @@ VALUE Message_alloc(VALUE klass) {
}

static const upb_fielddef* which_oneof_field(MessageHeader* self, const upb_oneofdef* o) {
upb_oneof_iter it;
size_t case_ofs;
uint32_t oneof_case;
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;
Expand Down
24 changes: 18 additions & 6 deletions ruby/ext/google/protobuf_c/protobuf.h
View file
Open in desktop
Expand Up @@ -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;
Expand Down Expand Up @@ -367,6 +368,9 @@ 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);
bool is_value_field(const upb_fielddef* f);

extern rb_encoding* kRubyStringUtf8Encoding;
extern rb_encoding* kRubyStringASCIIEncoding;
Expand Down Expand Up @@ -496,23 +500,31 @@ 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 offset;
uint32_t case_offset;
};

// MessageLayout is owned by the enclosing Descriptor, which must outlive us.
struct MessageLayout {
const Descriptor* desc;
const upb_msgdef* msgdef;
MessageField* fields;
size_t size;
MessageOneof* oneofs;
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,
Expand Down