lostluck commented on code in PR #25437:
URL: https://github.com/apache/beam/pull/25437#discussion_r1108967075
##########
sdks/go/pkg/beam/core/runtime/exec/sdf_invokers.go:
##########
@@ -39,61 +43,45 @@ import (
// cirInvoker is an invoker for CreateInitialRestriction.
type cirInvoker struct {
- fn *funcx.Fn
- args []any // Cache to avoid allocating new slices per-element.
- call func(elms *FullValue) (rest any)
+ fn *funcx.Fn
+ args []any // Cache to avoid allocating new slices per-element.
+ ctxIdx int
+ call func() (rest any, err error)
}
func newCreateInitialRestrictionInvoker(fn *funcx.Fn) (*cirInvoker, error) {
n := &cirInvoker{
fn: fn,
args: make([]any, len(fn.Param)),
}
+
+ var ok bool
+ if n.ctxIdx, ok = fn.Context(); !ok {
+ n.ctxIdx = -1
+ }
+
if err := n.initCallFn(); err != nil {
return nil, errors.WithContext(err, "sdf
CreateInitialRestriction invoker")
}
return n, nil
}
-func (n *cirInvoker) initCallFn() error {
- // Expects a signature of the form:
- // (key?, value) restriction
- // TODO(BEAM-9643): Link to full documentation.
- switch fnT := n.fn.Fn.(type) {
- case reflectx.Func1x1:
Review Comment:
OK, now that I've looked at the whole CL I understand your question. I think.
You're right that the generic `register` package doesn't currently wrap
these methods in `CallerMxN` type things. Some of the scaffolding here is
forward looking in the event we augment the `register` package to do this. I
think the old code generator also did some of this, but it wasn't ported to the
`register` package yet.
At present, the only methods that get wrapped for sure are the DoFn Bundle
Lifecycle methods (StartBundle, ProcessElement, FinishBundle). The reason for
this is those methods are called significantly more often, so paying the
repeated overhead of reflection for every call would be very noticible on
profiles.
Conversely, the SDF methods are called rarely. Some only meaningfully happen
on Dynamic Splitting, at the runner's request. So the overhead is unlikely to
appear in any execution profile, which is the motivation for all of these
wrappings and type assertions in the first place.
----
Basically as much as the current set up is neat and versatile, TBH I'd love
to have an alternate DoFn API that is much simpler for users by being more
opinionated, and simpler to invoke execution side.
At one extreme version we lose the ability to use arbitrary functions as
DoFns, but it would be much clearer to everyone what's going on, and how we
execute things at run time. Mentally, I've been calling this the "performance"
API.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
