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

Fast-path protoreflect.Message methods #3

Closed
37 of 38 tasks
aaronc opened this issue Jul 14, 2021 · 4 comments
Closed
37 of 38 tasks

Fast-path protoreflect.Message methods #3

aaronc opened this issue Jul 14, 2021 · 4 comments

Comments

@aaronc
Copy link
Member

aaronc commented Jul 14, 2021

Needed for custom types and Anys, will also speed up other codes.

Types to implement:

ProtoReflect.Messsage:

  • Descriptor
  • Type
  • New
  • Interface
  • Range
  • Has
  • Clear
  • Get:
    • Scalar types
    • Oneof types
    • Message types
    • List types
    • Map types
    • Extension Types
  • Set
  • Mutable
  • NewField
  • WhichOneof
  • GetUnknown
  • SetUnknown
  • IsValid

Protoreflect.List:

  • Len
  • Get
  • Set
  • Append
  • AppendMutable
  • Truncate
  • NewElement
  • IsValid

Protoreflect.Map:

  • Len
  • Range
  • Has
  • Clear
  • Get
  • Set
  • Mutable
  • NewValue
  • IsValid

Ex for https://pkg.go.dev/google.golang.org/[email protected]/reflect/protoreflect#Message Has method:

func (foo *Foo) Has(f FieldDescriptor) bool {
  switch f.Number() {
    case 1:
      return foo.Bar != nil
    default:
      return false
  }
}

If we wanted to reuse the default protoreflect implementation, maybe this will work but it won't speed up other code:

func (foo *Foo) Has(f FieldDescriptor) bool {
  switch f.Number() {
    case 1:
      return foo.Bar != nil
    default:
      return f.ProtoReflect().Has(f)
  }
}

Implementing all of the protoreflect.Message methods to be as fast as possible is the goal. Some benchmarks are here: cosmos/cosmos-sdk#9156 (reply in thread).

@fdymylja
Copy link
Contributor

fdymylja commented Jul 14, 2021

I could take care of implementing this.

Adding here more findings regarding my experimentations:

I ran some benches to output some concrete numbers of using the protoreflect approach vs golang reflection vs two >autogenerated fast reflection paths:

cpu: Intel(R) Core(TM) i7-8700K CPU @ 3.70GHz

protoreflect:
Benchmark_ProtoReflect-12 12509864 86.64 ns/op

golang reflection:
Benchmark_GoReflection-12 14749225 70.45 ns/op

reflection fast path with field descriptor:
Benchmark_FastPath-12 70583667 16.33 ns/op

reflection fast path with field name:
Benchmark_GetFieldByName-12 282286454 4.331 ns/op

I was experimenting by creating an object which wraps the fast reflection API logic, in order not to pollute the object with many methods (especially in case we might extend this interface in the future):

// autogenerated by our impl
type messageTestFastReflection {
    x *TestFastReflection
}

func (x messageTestFastReflection) GetFieldByName(name protoreflect.Name) {
... logic
}

// autogenerated by protoc-gen-go
type TestFastReflection {
}


func (x *TestFastReflection) FastReflection() messageFastReflection {
   return messageTestFastReflection{x}
}

^ This would look cleaner API wise (note: messageFastReflection would be substituted by a fastreflection interface which is TBD), but at the same time adds a little bit of performance overhead (I don't have clear numbers yet, will get them tomorrow).

@aaronc
Copy link
Member Author

aaronc commented Jul 14, 2021

That would be great @fdymylja 👍

@technicallyty can you push your latest code to a branch? Let's find a way to coordinate with PRs.

I think it makes sense to just implement the whole protoreflect.Message with the fast path to start. Then every API can take advantage of the performance boost. We can explore other even faster APIs like field by name later. How does that sound?

@technicallyty
Copy link
Contributor

updated my branch with my latest progress

@aaronc
Copy link
Member Author

aaronc commented Jul 14, 2021

Looks like all the protoreflect.Message methods are generated here: https://github.com/cosmos/cosmos-proto/blob/ty/pulsar-msg_interface/generator/protoMessage.go. So this is where we would update the fast path codegen. Not sure if it makes sense to start with PRs against your branch. Maybe let's coordinate on discord?

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

No branches or pull requests

3 participants