Ported Ruby extension to upb_msg #8184
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 |
gerben-s
approved these changes
Jan 11, 2021
|
@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. |
This was referenced Jan 27, 2021
This was referenced Feb 16, 2021
Closed
|
I suspect this is the cause of #8311 but only due to the size of the changeset. |
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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::TypeErrorin some cases but plainTypeErrorin others. The new code standardizes on usingGoogle::Protobuf::TypeErroreverywhere.#inspectserializers were emitting empty optional fields, whereas they should only be emitted when they are set.