implement protoreflect fast paths #8

fdymylja · 2021-07-15T15:08:14Z

Types to implement:

ProtoReflect.Messsage:

Protoreflect.List:

Protoreflect.Map:

fdymylja · 2021-07-15T15:14:49Z

This is how Get looks like: https://github.com/cosmos/cosmos-proto/pull/8/files#diff-b80be7ac49d3c20aabc1048615c419c929d42d171d6337a4f09287f770429697R390

I have some questions on top of my head.

Has get to be immutable? If it has we will need DeepCopy methods for protomessages (easy for primitive types, complex for Message types)
What about message extensions? instead of panic if the field descriptor is unknown we could rely back on protoreflect?
fieldDescriptor.Name() vs fieldDescriptor.FullName() vs something else to check for expected field descriptor equality (which means assert the field descriptor is the correct one, we should check the kind? maybe fullname is sufficient)

fdymylja · 2021-07-15T15:16:23Z

Things left to do (for Get):

Implement MapValue
Implement ListValue
Implement Oneof

fdymylja · 2021-07-19T08:41:09Z

Has get to be immutable? If it has we will need DeepCopy methods for protomessages (easy for primitive types, complex for Message types)

Regarding this, I ran a test:

p := &Params{
		ConstantFee: &v1alpha1.Coin{
			Denom:  "non-mutated",
			Amount: "1",
		},
	}

	value := p.ProtoReflect().Get(p.ProtoReflect().Descriptor().Fields().ByName("constant_fee"))
	value.Message().Set((&v1alpha1.Coin{}).ProtoReflect().Descriptor().Fields().Get(0), protoreflect.ValueOfString("mutated"))

	log.Printf("%s", p)
	log.Printf("%s", value.Message().Interface())

output:
2021/07/19 10:38:47 constant_fee:{denom:"mutated"  amount:"1"}
2021/07/19 10:38:47 denom:"mutated"  amount:"1"

This makes me assume that fields which are retrieved via Get can be mutated.

If the message type field is not set, a panic occurs in Set.

Same behaviour was observed for Map and List.

Other behaviour experimentation:

Tried to use create a dynamic protobuf message using the dynamicpb package, and tried to set a message field using dynamicpb.Message as value instead of the concrete type. Got a panic.

func TestDynamic(t *testing.T) {
	dynCoin := dynamicpb.NewMessage((&v1alpha1.Coin{}).ProtoReflect().Descriptor())

	p := &Params{}
	fd := p.ProtoReflect().Descriptor().Fields().ByName("constant_fee")
	p.ProtoReflect().Set(fd, protoreflect.ValueOfMessage(dynCoin))

	log.Printf("%s", p)
}

panic: invalid type: got *dynamicpb.Message, want *v1alpha1.Coin [recovered]
	panic: invalid type: got *dynamicpb.Message, want *v1alpha1.Coin

technicallyty · 2021-07-19T17:52:57Z

i've added the enumeration generators from protoc-gen-go to my branch if you would like to merge that in to your draft

aaronc

Great work @fdymylja !

The list struct codegen seems pretty involved. Are they needed? What if we just used something like the dynamicList from dynamicpb?

aaronc · 2021-07-19T20:59:52Z

testpb/1.pulsar.go

+// For unpopulated composite types, it returns an empty, read-only view
+// of the value; to obtain a mutable reference, use Mutable.
+func (x *A) Get(descriptor protoreflect.FieldDescriptor) protoreflect.Value {
+	switch descriptor.Name() {


Why not switching on field number? Would that maybe be faster?

What if we just used something like the dynamicList from dynamicpb?

I've tried combining dynamicpb + normal proto messages and it ends up in invalid type panics -> invalid type: got *dynamicpb.Message, want *v1alpha1.Coin [recovered] - dynamicpb also does not give us access to dynamiclist constructors.

we could create a "wrapper", I was experimenting with it but then i got stuck at some point during message.Set where field is a list field, and value is a protoreflect.ListValue in which you'd need to cast the value to the slice type.

Why not switching on field number? Would that maybe be faster?

The Get logic will need to be rewritten partially (to also include extension support), because it's not that safe, meaning that using just field name or number I think might not be sufficient, probably the safest way (assuming we're working with a global protobuf registry) is to use the field fullname. Reason being to avoid for example having a field which is number: 1, kind: message being Get using a field number: 1, kind: int64.

The user of Get would be left with an invalid protoreflect.Value, and the panic would happen only when value is being unwrapped.

fdymylja · 2021-07-20T08:14:46Z

Why not switching on field number? Would that maybe be faster?

I ran some tests on original protobuf implementation, Get does not work if I try to fetch a FieldDescriptor owned by another Message even if the name, field number and field kind are the same.

If we want to respect protoreflection behaviour: we should use FieldDescriptor.FullName()

fdymylja · 2021-07-20T12:13:32Z

I've update the Get implementation with the safer field checking.

I've also added the Oneof logic, please have a look at it so we can make sure the logic is correct: https://github.com/cosmos/cosmos-proto/pull/8/files#diff-ec66d96ce4cf4e11416971540fb8c46e759ed99ee0f1cb5e0738bc49172cae1fR255

technicallyty · 2021-07-20T15:14:15Z

should we have the checkbox list as a separate issue and complete each item in separate PRs?

technicallyty · 2021-07-20T15:35:43Z

went ahead and updated this issue #3 (comment) i think it would be a bit cleaner to track there and do small PR's for each item

technicallyty · 2021-07-19T18:06:05Z

generator/gen.go

+	/*
+		g.P("// Code generated by Pulsar \U0001FA90. DO NOT EDIT.")
+		if bi, ok := debug.ReadBuildInfo(); ok {
+			g.P("// Pulsar version: ", bi.Main.Version)
+		}
+		g.P("// source: ", file.Desc.Path())
+		g.P()
+	*/ // TODO(fdymylja): remove me


we should still have the DO NOT EDIT notice here no?

Yes sorry! I removed it so my editor allows me to see errors or warning! otherwise it won't parse it!

I will remove this when I'm done with the PR

fdymylja · 2021-07-20T16:31:43Z

went ahead and updated this issue #3 (comment) i think it would be a bit cleaner to track there and do small PR's for each item

sounds perfectly good to me!

fdymylja · 2021-07-20T17:23:11Z

Note on ProtoReflect implementation details on repeated types:

If list is nil || len(list) == 0 -> list cannot be mutated with Get.

if there's one element in the list, then the list can be mutated by gettings its protoreflect.ListValue. And the mutation reflects on the message containing the list (so if I append on ListValue then the message containing the list will be updated too).

Mutable allows modifications which are reflected on the type EVEN if the list is nil.

@aaronc, based on this findings I don't think we can do without custom list and map types. Maybe we could use unsafe to create a generic list/map type and cast the unsafe pointer to its concrete type. But I'm not sure of itse feasability.

aaronc · 2021-07-20T17:44:35Z

Note on ProtoReflect implementation details on repeated types:

If list is nil || len(list) == 0 -> list cannot be mutated with Get.

if there's one element in the list, then the list can be mutated by gettings its protoreflect.ListValue. And the mutation reflects on the message containing the list (so if I append on ListValue then the message containing the list will be updated too).

Mutable allows modifications which are reflected on the type EVEN if the list is nil.

@aaronc, based on this findings I don't think we can do without custom list and map types. Maybe we could use unsafe to create a generic list/map type and cast the unsafe pointer to its concrete type. But I'm not sure of itse feasability.

I think it's fine with custom. Was just wondering if we could simplify. Maybe a generic type makes sense, but we should aim for correctness and performance first I think

fdymylja · 2021-07-27T10:35:18Z

I've found a spec discrepancy in protoreflect.Set regarding mutability:

If type is List, setting and mutating the List after DOES NOT reflect list changes in the message.
If type is Map, setting a nd mutating the Map DOES reflect map changes in the message.
if type is Message, setting and mutating the message DOES reflect map changes in the message.

fdymylja · 2021-07-30T13:06:37Z

I will wrap up this PR early next week, I will also open some issues regarding behaviour in the goproto repo. Then we can focus on testing, if someone is available to write tests I can give a shoot at #5 in the meantime (after I finalize this PR).

Would that be fine @aaronc, @technicallyty ?

fdymylja · 2021-08-03T11:14:51Z

All the fastpath implementations are done.

I am trying to experiment a little for a faster Message.Range implementation which does not access FieldDescriptorsList but uses predefined FDs, as per the following profiling:

The generated code might increase a bit but we might get a significant code performance increase.

…dymylja/fast-reflection

fdymylja · 2021-08-04T08:53:38Z

All the fastpath implementations are done.

I am trying to experiment a little for a faster Message.Range implementation which does not access FieldDescriptorsList but uses predefined FDs, as per the following profiling:

The generated code might increase a bit but we might get a significant code performance increase.

Managed to make Range 12x more performant than using standard protoreflect.

technicallyty · 2021-08-25T18:51:13Z

should we just target the main branch here and merge this in now?

…terface

technicallyty · 2021-09-03T18:21:26Z

okay ive merged main into the branch, made fastreflect implement the feature interface, and removed all the copied protoc-gen-go code.

also i've shortened up fastreflection in places where one might manually type them like make fastreflection -> changed this to just make fast

I'll merge this in now

fdymylja requested review from technicallyty and aaronc July 15, 2021 15:10

aaronc reviewed Jul 19, 2021

View reviewed changes

fdymylja added 6 commits July 20, 2021 10:15

change: split implementations

6d86e72

change: finalize impl splitting

de40997

add: implement New

11169d1

add: implement Interface

d06295b

add: finalize Get oneof logic

54d78c4

add: fastreflect demo

c9a01c5

chore: update err msg

439819d

technicallyty reviewed Jul 20, 2021

View reviewed changes

fix: reflect list mutability

0ca66ed

fdymylja added 2 commits July 20, 2021 18:40

chore: minor refactor

24b15ed

chore: cleanup list gen

0c808e7

fdymylja added 5 commits July 21, 2021 11:08

add: map implementation

1c88202

chore: cleanup code

2c59daf

chore: cleanup code 2

87f5c04

chore: cleanup code 3

532f54a

add: list and map implementation

25c2320

fdymylja added 3 commits July 26, 2021 17:14

chore: cleanup code

36869df

add: clear impl

12dd2a7

chore: clear tests and benches

b5eea42

fdymylja added 9 commits July 27, 2021 12:57

add: set implementation + extension panic msg

b595c8a

add: set benches

7e44edc

add: newfield impl

e966d27

add: mutable impl

a2ea828

add: mutable bench

acba70b

fix: remove message mutability in new field

82f541d

add: range impl

9649ba9

add: benches, make more performant

46d6be3

add: is valid impl

cc0c220

fdymylja added 2 commits August 3, 2021 13:00

add: implement type and descriptor fastpaths

d6d3049

add: implement get/set unknown reflection fast paths

685d53f

add: optimize range performance

800c31c

fdymylja marked this pull request as ready for review August 4, 2021 08:50

fdymylja and others added 2 commits August 4, 2021 10:52

Merge branch 'ty/pulsar-msg_interface' into fdymylja/fast-reflection

37f6c7c

Merge remote-tracking branch 'origin/fdymylja/fast-reflection' into f…

80d1165

…dymylja/fast-reflection

fdymylja added 2 commits August 4, 2021 11:14

add: codec equality

2a6f205

fix: make test more sound

25eec5b

aaronc mentioned this pull request Aug 18, 2021

Explore generic map and list wrappers for fast reflection #15

Open

fdymylja changed the base branch from ty/pulsar-msg_interface to main September 3, 2021 09:11

feat: cleanup for merge and make fast reflection implement feature in…

9d4d4e9

…terface

technicallyty merged commit 99bf1d0 into main Sep 3, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement protoreflect fast paths #8

implement protoreflect fast paths #8

fdymylja commented Jul 15, 2021 •

edited

Loading

fdymylja commented Jul 15, 2021

fdymylja commented Jul 15, 2021 •

edited

Loading

fdymylja commented Jul 19, 2021 •

edited

Loading

technicallyty commented Jul 19, 2021

aaronc left a comment •

edited

Loading

aaronc Jul 19, 2021

fdymylja Jul 19, 2021

fdymylja Jul 19, 2021 •

edited

Loading

fdymylja commented Jul 20, 2021

fdymylja commented Jul 20, 2021

technicallyty commented Jul 20, 2021

technicallyty commented Jul 20, 2021

technicallyty Jul 19, 2021

fdymylja Jul 20, 2021

fdymylja Jul 20, 2021

fdymylja commented Jul 20, 2021

fdymylja commented Jul 20, 2021 •

edited

Loading

aaronc commented Jul 20, 2021

fdymylja commented Jul 27, 2021

fdymylja commented Jul 30, 2021

fdymylja commented Aug 3, 2021

fdymylja commented Aug 4, 2021

technicallyty commented Aug 25, 2021

technicallyty commented Sep 3, 2021 •

edited

Loading

implement protoreflect fast paths #8

implement protoreflect fast paths #8

Conversation

fdymylja commented Jul 15, 2021 • edited Loading

fdymylja commented Jul 15, 2021

fdymylja commented Jul 15, 2021 • edited Loading

fdymylja commented Jul 19, 2021 • edited Loading

technicallyty commented Jul 19, 2021

aaronc left a comment • edited Loading

Choose a reason for hiding this comment

aaronc Jul 19, 2021

Choose a reason for hiding this comment

fdymylja Jul 19, 2021

Choose a reason for hiding this comment

fdymylja Jul 19, 2021 • edited Loading

Choose a reason for hiding this comment

fdymylja commented Jul 20, 2021

fdymylja commented Jul 20, 2021

technicallyty commented Jul 20, 2021

technicallyty commented Jul 20, 2021

technicallyty Jul 19, 2021

Choose a reason for hiding this comment

fdymylja Jul 20, 2021

Choose a reason for hiding this comment

fdymylja Jul 20, 2021

Choose a reason for hiding this comment

fdymylja commented Jul 20, 2021

fdymylja commented Jul 20, 2021 • edited Loading

aaronc commented Jul 20, 2021

fdymylja commented Jul 27, 2021

fdymylja commented Jul 30, 2021

fdymylja commented Aug 3, 2021

fdymylja commented Aug 4, 2021

technicallyty commented Aug 25, 2021

technicallyty commented Sep 3, 2021 • edited Loading

fdymylja commented Jul 15, 2021 •

edited

Loading

fdymylja commented Jul 15, 2021 •

edited

Loading

fdymylja commented Jul 19, 2021 •

edited

Loading

aaronc left a comment •

edited

Loading

fdymylja Jul 19, 2021 •

edited

Loading

fdymylja commented Jul 20, 2021 •

edited

Loading

technicallyty commented Sep 3, 2021 •

edited

Loading