Skip to content

Conversation

@TristonianJones
Copy link
Collaborator

@TristonianJones TristonianJones commented Nov 11, 2020

Support for the Golang protobuf v2 API

Breaking changes

Value v. ConvertToNative

Calling Value() on a proto-based object will likely return a *dynamicpb.Message instance rather than the strongly typed proto message. Using ConvertToNative(reflect.TypeOf(&<DesiredType>{}) will produce the desired result.

DescriptorProto v. Descriptor

Top-level interfaces have shifted away from using the DescriptorProto and FileDescriptorProto messages directly in favor of the protobuf v2 API protoreflect.Descriptor and protoreflect.FileDescriptor.

Duration and Timestamp

The duration.go and timestamp.go object types are now derived from go-native time.Duration and time.Time respectively as the dependence on the protobuf counterparts was not actually necessary.

Performance

  • Proto operations are now roughly 50% - 100% slower in most cases, but some operations are significantly faster.
  • In general this PR improves the evaluation speed by about 10% across the board due to changes in NativeToValue conversion.
  • The map.go and list.go implementations were completely refactored to reduce repetitive ConvertToNative call paths. Many bugs were fixed, and there is now a slightly higher allocation overhead for these object types, but the overall code is much simpler to maintain.
  • Specializations were added for map[ref.Val]ref.Val and []ref.Val as these are the values of CEL literals.

Bug Fixes

  • Additional coverage has been added and a number of minor inconsistencies in the ConvertToNative call paths of common/types objects have been fixed as a result of testing for conformance with the new proto API.
  • CEL map.go values can be converted to Go-native structs; however, support for configuring a Go native struct from a proto.Message field of type map is not yet implemented.

Benchmarks

These were the changes in evaluation speed which had a notable performance difference (>10%):

Interpreter/call_varargs-8                       417ns ± 6%     320ns ± 8%   -23.34%
Interpreter/cond-8                               209ns ± 4%     172ns ± 6%   -17.41%
Interpreter/in_map-8                             792ns ± 3%      80ns ± 5%   -89.87%
Interpreter/index-8                              240ns ± 7%     195ns ± 9%   -18.86%
Interpreter/index_relative-8                    2.19µs ± 5%    0.69µs ± 7%   -68.71%
Interpreter/literal_pb3_msg-8                   2.26µs ± 1%    2.89µs ± 3%   +27.96%
Interpreter/literal_pb_enum-8                   2.13µs ± 2%    2.66µs ± 3%   +25.24%
Interpreter/timestamp_eq_timestamp-8             707ns ± 2%      22ns ± 2%   -96.89%
Interpreter/timestamp_lt_timestamp-8            69.7ns ± 1%    58.6ns ± 4%   -15.97%
Interpreter/timestamp_le_timestamp-8            70.5ns ± 0%    60.1ns ± 6%   -14.74%
Interpreter/timestamp_gt_timestamp-8            71.0ns ± 0%    59.8ns ± 5%   -15.77%
Interpreter/timestamp_ge_timestamp-8            75.5ns ±10%    59.9ns ± 3%   -20.62%
Interpreter/macro_all_non_strict_var-8          1.91µs ± 3%    1.61µs ± 1%   -15.98%
Interpreter/macro_has_map_key-8                 1.67µs ± 1%    0.30µs ± 1%   -82.07%
Interpreter/macro_has_pb2_field-8               12.8µs ±71%     7.9µs ± 6%   -37.96%
Interpreter/nested_proto_field-8                 533ns ± 1%     435ns ± 4%   -18.27%
Interpreter/nested_proto_field_with_index-8      900ns ± 1%    1816ns ± 1%  +101.89%
Interpreter/or_true_1st-8                       62.8ns ± 2%    44.7ns ± 4%   -28.91%
Interpreter/or_true_2nd-8                        226ns ± 3%     175ns ± 4%   -22.53%
Interpreter/or_false-8                           221ns ± 1%     196ns ±20%   -11.18%
Interpreter/pkg_qualified_id-8                  58.1ns ± 3%    41.1ns ± 6%   -29.20%
Interpreter/pkg_qualified_id_unchecked-8         332ns ± 1%     101ns ± 4%   -69.63%
Interpreter/pkg_qualified_index_unchecked-8     1.18µs ± 2%    0.28µs ± 6%   -76.60%
Interpreter/select_key-8                        1.49µs ± 1%    1.16µs ± 2%   -22.43%
Interpreter/select_bool_key-8                   4.41µs ± 3%    5.26µs ± 4%   +19.37%
Interpreter/select_index-8                      1.89µs ± 1%    1.37µs ± 2%   -27.33%
Interpreter/select_field-8                      1.04µs ± 3%    1.46µs ± 1%   +41.41%
Interpreter/select_pb2_primitive_fields-8       4.14µs ± 4%    1.84µs ± 2%   -55.66%
Interpreter/select_pb3_wrapper_fields-8         1.62µs ± 2%    3.38µs ± 2%  +108.76%
Interpreter/select_pb3_compare-8                 194ns ± 1%     167ns ± 6%   -13.89%
Interpreter/select_custom_pb3_compare-8          116ns ± 1%      75ns ± 6%   -35.17%
Interpreter/select_subsumed_field-8             50.6ns ± 3%    32.5ns ± 5%   -35.77%

Closes #385 #398 #400

@glerchundi
Copy link

This is awesome @TristonianJones, keep up the good work!

@TristonianJones TristonianJones force-pushed the protobuf-v2 branch 3 times, most recently from 0ca2f87 to 73c2c0f Compare November 14, 2020 04:55
@TristonianJones TristonianJones marked this pull request as ready for review November 19, 2020 07:00
@achew22
Copy link
Contributor

achew22 commented Nov 21, 2020

To echo what @glerchundi said, great work @TristonianJones

@TristonianJones
Copy link
Collaborator Author

Thanks @achew22 and @glerchundi. I'm happy to finally get this done.

@TristonianJones
Copy link
Collaborator Author

@JimLarson I think it's ready for a first pass. I couldn't enable proto3 optional fields yet because I can't figure out how to get bazel to recognize a very specific experimental flag that needs to be set to use them publicly. Once this is better understood, I'll follow up with more tests.

@lopezator
Copy link
Contributor

Awesome!

@TristonianJones
Copy link
Collaborator Author

PTAL

Copy link
Contributor

@JimLarson JimLarson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost done. I added replies on naming the right returned type in bool.go and handling Any messages wrapping other wrappers in type.go.

@TristonianJones
Copy link
Collaborator Author

PTAL

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.

protobuf: update deprecated github.com/golang/protobuf/proto to google.golang.org/protobuf/proto

5 participants