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

[BEAM-3612] Add a benchmark for method invocation methods. #6893

Merged
merged 2 commits into from
Nov 1, 2018

Conversation

lostluck
Copy link
Contributor

@lostluck lostluck commented Oct 30, 2018

With the Go SDK's necessary default use of reflection to achieve more idiomatic user code, it's up to the framework to reduce this overhead as much as possible. This benchmark illustrates the various methods available to the Go SDK and informs work I've been doing on [BEAM-3612].

Having properly generated type assertion shims at present represents a reduction of around ~400ns per function invocation. As mentioned in the JIRA, I've got a tool I've been working on to make generating these shims easier.

Further, this reveals there's a way to change how we handle method invocation which is relevant for Structural DoFns. The way we use a reflect.Value.Interface() call to get to back to an indirect interface{} version of the method, we typically go through the "Implicit Receiver" route, so that the receiver we're operating on (the Structural DoFn with the associated state) is implicitly cached and instated by the reflection code when we call it. This adds ~180ns overhead (on my machine) compared to the the Explicit Receiver version.

The tricky part here is that moving to Explicit Receivers requires a minor overhaul through the beam function handling code, to properly mark the receiver parameter for the type checking phase. This is better accomplished after we have tools to re-generate type assertion shims for the SDK, if we're doing it at all.

Finally, by having arity Explicit invocation of the shims reduces overhead by another 30ns. This is probably something that can be generated one off (with the above concession to the Explicit Receiver approach) to get a base set of combinatorial values. However, the extra overhead of a arity specialized invoker might be more than the 30ns cost of allocating & extracting the values from the slice, so this should be more specifically benchmarked in context of invoking for ParDos.


Follow this checklist to help us incorporate your contribution quickly and easily:

  • Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
  • If this contribution is large, please file an Apache Individual Contributor License Agreement.

It will help us expedite review of your Pull Request if you tag someone (e.g. @username) to look at it.

Post-Commit Tests Status (on master branch)

Lang SDK Apex Dataflow Flink Gearpump Samza Spark
Go Build Status --- --- --- --- --- ---
Java Build Status Build Status Build Status Build Status Build Status Build Status Build Status Build Status
Python Build Status --- Build Status
Build Status
Build Status --- --- ---

@lostluck
Copy link
Contributor Author

R: @aaltay

@lostluck
Copy link
Contributor Author

lostluck commented Nov 1, 2018

R: @aaltay Ping!

Copy link
Member

@aaltay aaltay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Asked a few questions, looks good otherwise.

@lostluck
Copy link
Contributor Author

lostluck commented Nov 1, 2018

R: @aaltay Thanks for the review! Please merge.

Copy link
Member

@aaltay aaltay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanations.

@aaltay aaltay merged commit 479a216 into apache:master Nov 1, 2018
@lostluck lostluck deleted the benchmarked branch January 23, 2019 16:32
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

Successfully merging this pull request may close these issues.

None yet

2 participants