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.

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

Merged
merged 1 commit into from
Jan 6, 2022
Merged

fix: nested descriptors #56

merged 1 commit into from
Jan 6, 2022

Conversation

fdymylja
Copy link
Contributor

@fdymylja fdymylja commented Jan 6, 2022

Fixes a bug in which in multilayered nested messages, fast message descriptors were panicking at init phase because correct message hierarchy was not respected.

Copy link
Member

@aaronc aaronc left a 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")

@fdymylja
Copy link
Contributor Author

fdymylja commented Jan 6, 2022

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.

@aaronc
Copy link
Member

aaronc commented Jan 6, 2022

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?

@fdymylja
Copy link
Contributor Author

fdymylja commented Jan 6, 2022

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.

@fdymylja fdymylja merged commit 42f1f41 into main Jan 6, 2022
@fdymylja fdymylja deleted the fdymylja/fix-descriptors branch January 6, 2022 18:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants