[BEAM-3388] Use general untyped function call mechanism in Go SDK runtime#4443
[BEAM-3388] Use general untyped function call mechanism in Go SDK runtime#4443lukecwik merged 4 commits intoapache:go-sdkfrom
Conversation
herohde
commented
Jan 18, 2018
herohde
commented
- Dynamic functions can thus use it directly and avoid reflection
- Change decoders/encodes to simple wrappers to unify arity instead of full specialization.
- Moved type-specialized code into separate package and import it in beamx
- Made templates more easily reusable for other types.
* Dynamic functions can thus use it directly and avoid reflection * Change decoders/encodes to simple wrappers over a reflectx.Func.
sdks/go/pkg/beam/core/graph/fn.go
Outdated
| // T is the type of the generated function | ||
| T reflect.Type | ||
| // Data holds the data for the generator. | ||
| // Data holds the data for the generator. This data is trivially serializable. |
There was a problem hiding this comment.
Maybe we don't need the comment. What []byte isn't trivially serializable?
Or maybe this is a comment for DynFn, that they are trivially serializable because of this, which is why they are awesome, and we built them? :)
There was a problem hiding this comment.
Clarified the comment.
| //go:generate specialize --input=callers.tmpl --x=data,universals | ||
|
|
||
| // NOTE(herohde) 12/11/2017: the below helpers are ripe for type-specialization, | ||
| // if the reflection overhead here turns out to be significant. It would |
There was a problem hiding this comment.
Is this comment still relevant? Seems we're doing all the type specialization now.
|
Thanks @wcn3. PTAL |
|
|
||
| // RegisterCaller registers an custom caller factory for the given type, such as | ||
| // "func(int)bool". If multiple caller factories are registered for the same type, | ||
| // RegisterFunc registers an custom reflectFunc factory for the given type, such as |
There was a problem hiding this comment.
reflectFunc -> reflectx.Func ?
Or maybe just Func? it seems appropriate to drop the package specifier for local Exported types.
|
|
||
| package optimized | ||
|
|
||
| // NOTE(herohde) 1/18/2018: omit []byte for encodes to avoid re-registration due to symmetry. |
There was a problem hiding this comment.
Is this a TODO, or is it something that's actually done already? It sounds like a TODO.
There was a problem hiding this comment.
It's already done. The command for encoders use "primitives" instead of "data".
| encodersMu.Lock() | ||
| defer encodersMu.Unlock() | ||
| func makeEncoder(fn reflectx.Func) Encoder { | ||
| // Detect one of the valid decoder forms and allow it to be invoked |
| callersMu.Lock() | ||
| maker, exists := callers[reflect.TypeOf(fn).String()] | ||
| callersMu.Unlock() | ||
| // MakeFunc returns a reflectFunc for given function. |
There was a problem hiding this comment.
It's confusing to refer to an unexported type in godoc. Consider simply calling it Func instead.
| // RegisterCaller registers an custom caller factory for the given type, such as | ||
| // "func(int)bool". If multiple caller factories are registered for the same type, | ||
| // RegisterFunc registers an custom reflectFunc factory for the given type, such as | ||
| // "func(int)bool". If multiple func factories are registered for the same type, |
There was a problem hiding this comment.
If the intent is to refer to the type, consider Func instead of func.
|
Thanks! |
|
R: @lukecwik (please merge) |
4 participants
