-
Notifications
You must be signed in to change notification settings - Fork 15
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
fix: nested descriptors #56
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch!
I'm a bit unclear as to how this works. Can you explain why we need to do this:
md_MultiLayeredNesting = File_internal_testprotos_test3_test_nesting_proto.Messages().ByName("MultiLayeredNesting")
fd_MultiLayeredNesting_nested1 = md_MultiLayeredNesting.Fields().ByName("nested1")
Basically if we have a message which is defining another message inside (as we have in cosmos-sdk's signing), we can fetch the nested message's descriptor from within the parent. example: MessageA defines NestedInMessageA message. So in order to get the descriptor of NestedInMessageA we need first to get the descriptor of MessageA and then from the descriptor of MessageA we get NestedInMessageA descriptor. Also regarding why we pre-load field descriptors is because of the APIs that make use of FieldDescriptors (such as range), since we already know the field descriptors of a message we don't load them each time from the protobuf message descriptor interface (it's super slow), but instead we pre-compile them at init. |
So the basic gist is that by fetching them in init, we're doing some pre-compilation that's needed later? |
Basically this: https://github.com/cosmos/cosmos-proto/blob/main/testpb/1.pulsar.go#L328 As you can see here we're using the pre-compiled FieldDescs, if we didn't have them we'd need to call message.ProtoReflect().Descriptor().Fields().ByName("fieldName") For each field... Based on my benches this drastically impacts performance. |
Fixes a bug in which in multilayered nested messages, fast message descriptors were panicking at init phase because correct message hierarchy was not respected.