Speculative optimization: Use custom string serializer for Type

[kyessenov]

It seems that the generic text proto CompactStringMarshal is more expensive that hand-made one.
Looking for feedback. I think we probably want to use a Struct as a hashmap key, but that would require building a new family of structs mirroring Types, since using proto Type does not seem to work.

[TristonianJones]

I'm not necessarily opposed to the change, but could you give more context about the expected impact/benefit to tests, execution, and performance?

[kyessenov]

Sure:

• prior:

Benchmark             300         324989142 ns/op        46162742 B/op    1151582 allocs/op

• after:

Benchmark            2000          64968044 ns/op        11220121 B/op     377465 allocs/op

[kyessenov]

The reason seems to be excessive allocations caused by standard protobuf. String() indirectly marshals the protos, and the mashaller is fairly expensive.

[TristonianJones]

Thanks for the info. Let's merge it as long as it doesn't impact the checking logic in any way.

[kyessenov]

I examined FormatCheckedType and it seems to be creating unique strings for distinct types. That assumes no obvious collisions, such as naming a proto "dyn". Let me know if you want to extend this further to disambiguate between proto "dyn" and dynamic "dyn".

[TristonianJones]

I don't think there's any need to differentiate the dyn types. They should be treated identically here.

[kyessenov]

Sorry, I wasn't clear. I can in theory create a proto with:

message dyn { }

in which case "dyn" is ambiguous between object type "dyn" and type "dyn". Frankly, noone should do it, but we can be more cautious in this function.

[TristonianJones]

While that is possible, it would be pretty nutty. If you want to add a precondition check to prevent adding a proto with that name that seems reasonable

…

On Thu, Mar 28, 2019, 2:15 PM Kuat ***@***.***> wrote: Sorry, I wasn't clear. I can in theory create a proto with: message dyn { } in which case "dyn" is ambiguous between object type "dyn" and type "dyn". Frankly, noone should do it, but we can be more cautious in this function. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#192 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAdf5ICYey8mmPA5kckM_YL3WvkCRW5sks5vbTDmgaJpZM4cQowu> .

[kyessenov]

Doing something like that will cause "check" to misbehave and reject the expression, so I guess the outcome is right -- don'to do nutty things.