-
Notifications
You must be signed in to change notification settings - Fork 15.5k
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.
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
Ported Ruby extension to upb_msg #8184
Conversation
Still a lot of bugs to fix, but it is a major step! 214 tests, 378 assertions, 37 failures, 147 errors, 0 pendings, 0 omissions, 0 notifications 14.0187% passed
214 tests, 134243 assertions, 30 failures, 144 errors, 0 pendings, 0 omissions, 0 notifications 18.6916% passed
214 tests, 124651 assertions, 53 failures, 70 errors, 0 pendings, 0 omissions, 0 notifications 42.5234% passed
214 tests, 202091 assertions, 41 failures, 23 errors, 0 pendings, 0 omissions, 0 notifications 70.0935% passed
214 tests, 322331 assertions, 30 failures, 9 errors, 0 pendings, 0 omissions, 0 notifications 81.7757% passed Unfortunately there is also a sporadic bug/segfault hanging around that appears to be GC-related.
214 tests, 349898 assertions, 15 failures, 1 errors, 0 pendings, 0 omissions, 0 notifications 92.5234% passed
214 tests, 369388 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications 100% passed
1. Removed a bunch of internal-only symbols from headers. 2. Required a frozen check to get a non-const pointer to a map or array. 3. De-duplicated the code to get a type argument for Map/RepeatedField.
…assert failure. Intermittent failure: ruby: ../../../../ext/google/protobuf_c/protobuf.c:263: ObjectCache_Add: Assertion `rb_funcall(obj_cache2, (__builtin_constant_p("[]") ? __extension__ ({ static ID rb_intern_id_cache; if (!rb_intern_id_cache) rb_intern_id_cache = rb_intern2((("[]") ), (long)strlen(("[]"))); (ID) rb_intern_id_cache; }) : rb_intern("[]")), 1, key_rb) == val' failed
Is it ready to be shipped? xref: #7922 |
@KapilSachdev the change is looking great in unit tests. I'm hoping to get some feedback about whether it is working for users before I merge. A team at Google is planning to try it out sometime this week, but any other feedback from open-source users would be very helpful too. |
Oooh, I'm very interested in this. When do you anticipate this will land? |
+1 |
This will be in the next release, which should land in the next few weeks. |
I suspect this is the cause of #8311 but only due to the size of the changeset. |
This PR implements a significant refactoring of the Ruby C extension. The internal representation of messages is changing from a custom Ruby-specific raw memory layout to using
upb_msg
, a representation that is defined in the upb library. Upb defines a reflection interface that we can use to access message data from C.This refactoring should lead to much better performance, particularly when parsing large messages. The old code would eagerly create a Ruby object for every single sub-message parsed from the wire, which meant tons of objects would participate in garbage collection. The new extension only creates C objects at parse time, and Ruby wrapper objects are created on demand, only when accessed.
This fixes a number of conformance errors in the old extension.
There are a few slight behavior changes:
Google::Protobuf::TypeError
in some cases but plainTypeError
in others. The new code standardizes on usingGoogle::Protobuf::TypeError
everywhere.#inspect
serializers were emitting empty optional fields, whereas they should only be emitted when they are set.