You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by GitBox <gi...@apache.org> on 2022/11/07 23:08:44 UTC
[GitHub] [beam] lukecwik opened a new pull request, #24025: Stop wrapping light weight functions with Contextful as they add a lot of overhead for functions that are meant to do almost no work.
lukecwik opened a new pull request, #24025:
URL: https://github.com/apache/beam/pull/24025
Fixes #24024
Stop wrapping light weight functions with Contextful as they add a lot of overhead for functions that are meant to do almost no work.
Whole profile is 33.64s. We can eliminate (36.94-19.19)+(92.42-37.9) millis from which leads to a 0.2% performance improvement for the pipeline but leads to substantially larger gains for trivial lambdas. It may lead to a larger improvement as we are reducing the wasteful creation and destruction of the wrapped context.
See attached pprof data.
[profile (47).pb.gz](https://github.com/apache/beam/files/9956221/profile.47.pb.gz)
------------------------
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
- [ ] [**Choose reviewer(s)**](https://beam.apache.org/contribute/#make-your-change) and mention them in a comment (`R: @username`).
- [ ] Mention the appropriate issue in your description (for example: `addresses #123`), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, comment `fixes #<ISSUE NUMBER>` instead.
- [ ] Update `CHANGES.md` with noteworthy changes.
- [ ] If this contribution is large, please file an Apache [Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
See the [Contributor Guide](https://beam.apache.org/contribute) for more tips on [how to make review process smoother](https://beam.apache.org/contribute/get-started-contributing/#make-the-reviewers-job-easier).
To check the build health, please visit [https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md](https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md)
GitHub Actions Tests Status (on master branch)
------------------------------------------------------------------------------------------------
[](https://github.com/apache/beam/actions?query=workflow%3A%22Build+python+source+distribution+and+wheels%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Python+Tests%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Java+Tests%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Go+tests%22+branch%3Amaster+event%3Aschedule)
See [CI.md](https://github.com/apache/beam/blob/master/CI.md) for more information about GitHub Actions CI.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] lukecwik commented on pull request #24025: [#24024] Stop wrapping light weight functions with Contextful as they add a lot of overhead for functions that are meant to do almost no work.
Posted by GitBox <gi...@apache.org>.
lukecwik commented on PR #24025:
URL: https://github.com/apache/beam/pull/24025#issuecomment-1340057686
R: @damccorm
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] lukecwik merged pull request #24025: [#24024] Stop wrapping light weight functions with Contextful as they add a lot of overhead for functions that are meant to do almost no work.
Posted by GitBox <gi...@apache.org>.
lukecwik merged PR #24025:
URL: https://github.com/apache/beam/pull/24025
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] lukecwik commented on a diff in pull request #24025: [#24024] Stop wrapping light weight functions with Contextful as they add a lot of overhead for functions that are meant to do almost no work.
Posted by GitBox <gi...@apache.org>.
lukecwik commented on code in PR #24025:
URL: https://github.com/apache/beam/pull/24025#discussion_r1035251386
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/FlatMapElements.java:
##########
@@ -45,16 +45,13 @@
extends PTransform<PCollection<? extends InputT>, PCollection<OutputT>> {
private final transient @Nullable TypeDescriptor<InputT> inputType;
private final transient @Nullable TypeDescriptor<OutputT> outputType;
- private final transient @Nullable Object originalFnForDisplayData;
- private final @Nullable Contextful<Fn<InputT, Iterable<OutputT>>> fn;
+ private final @Nullable Object fn;
private FlatMapElements(
- @Nullable Contextful<Fn<InputT, Iterable<OutputT>>> fn,
- @Nullable Object originalFnForDisplayData,
+ @Nullable Object fn,
Review Comment:
You're correct, the type system was too painful to specify `OR` between `ProcessFunction` and `Contextful`.
Note that the constructor is private so this detail isn't exposed to users.
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/FlatMapElements.java:
##########
@@ -137,57 +132,90 @@ public <NewInputT> FlatMapElements<NewInputT, OutputT> via(
@Experimental(Kind.CONTEXTFUL)
public <NewInputT> FlatMapElements<NewInputT, OutputT> via(
Contextful<Fn<NewInputT, Iterable<OutputT>>> fn) {
- return new FlatMapElements<>(
- fn, fn.getClosure(), TypeDescriptors.inputOf(fn.getClosure()), outputType);
+ return new FlatMapElements<>(fn, TypeDescriptors.inputOf(fn.getClosure()), outputType);
}
@Override
public PCollection<OutputT> expand(PCollection<? extends InputT> input) {
checkArgument(fn != null, ".via() is required");
- return input.apply(
- "FlatMap",
- ParDo.of(
- new DoFn<InputT, OutputT>() {
- @ProcessElement
- public void processElement(ProcessContext c) throws Exception {
- Iterable<OutputT> res =
- fn.getClosure().apply(c.element(), Fn.Context.wrapProcessContext(c));
- for (OutputT output : res) {
- c.output(output);
+ if (fn instanceof Contextful) {
Review Comment:
Yes, half the cost of these trivial functions was going to creating and destroying a Contextful that wraps a ProcessBundleDescriptor that is passed in.
Also note that the branch only happens during pipeline construction so it won't show up on the execution side.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] robertwb commented on a diff in pull request #24025: [#24024] Stop wrapping light weight functions with Contextful as they add a lot of overhead for functions that are meant to do almost no work.
Posted by GitBox <gi...@apache.org>.
robertwb commented on code in PR #24025:
URL: https://github.com/apache/beam/pull/24025#discussion_r1035233601
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/FlatMapElements.java:
##########
@@ -45,16 +45,13 @@
extends PTransform<PCollection<? extends InputT>, PCollection<OutputT>> {
private final transient @Nullable TypeDescriptor<InputT> inputType;
private final transient @Nullable TypeDescriptor<OutputT> outputType;
- private final transient @Nullable Object originalFnForDisplayData;
- private final @Nullable Contextful<Fn<InputT, Iterable<OutputT>>> fn;
+ private final @Nullable Object fn;
private FlatMapElements(
- @Nullable Contextful<Fn<InputT, Iterable<OutputT>>> fn,
- @Nullable Object originalFnForDisplayData,
+ @Nullable Object fn,
Review Comment:
Shouldn't this still be an InferableFunction? Or is the goal to accept either InferableFunction or Contextful? (If the latter, is this too painful to encode in the type system?)
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/FlatMapElements.java:
##########
@@ -137,57 +132,90 @@ public <NewInputT> FlatMapElements<NewInputT, OutputT> via(
@Experimental(Kind.CONTEXTFUL)
public <NewInputT> FlatMapElements<NewInputT, OutputT> via(
Contextful<Fn<NewInputT, Iterable<OutputT>>> fn) {
- return new FlatMapElements<>(
- fn, fn.getClosure(), TypeDescriptors.inputOf(fn.getClosure()), outputType);
+ return new FlatMapElements<>(fn, TypeDescriptors.inputOf(fn.getClosure()), outputType);
}
@Override
public PCollection<OutputT> expand(PCollection<? extends InputT> input) {
checkArgument(fn != null, ".via() is required");
- return input.apply(
- "FlatMap",
- ParDo.of(
- new DoFn<InputT, OutputT>() {
- @ProcessElement
- public void processElement(ProcessContext c) throws Exception {
- Iterable<OutputT> res =
- fn.getClosure().apply(c.element(), Fn.Context.wrapProcessContext(c));
- for (OutputT output : res) {
- c.output(output);
+ if (fn instanceof Contextful) {
Review Comment:
Do I understand correctly that doing this branch every time is cheaper than the indirection of an "empty" Contextful?
Would it make sense to create a subclass based on the Fn type rather than branch here (and below).
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] lukecwik commented on pull request #24025: [#24024] Stop wrapping light weight functions with Contextful as they add a lot of overhead for functions that are meant to do almost no work.
Posted by GitBox <gi...@apache.org>.
lukecwik commented on PR #24025:
URL: https://github.com/apache/beam/pull/24025#issuecomment-1322537717
Run Java PreCommit
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] lukecwik commented on pull request #24025: [#24024] Stop wrapping light weight functions with Contextful as they add a lot of overhead for functions that are meant to do almost no work.
Posted by GitBox <gi...@apache.org>.
lukecwik commented on PR #24025:
URL: https://github.com/apache/beam/pull/24025#issuecomment-1322631115
Run Java PreCommit
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] github-actions[bot] commented on pull request #24025: [#24024] Stop wrapping light weight functions with Contextful as they add a lot of overhead for functions that are meant to do almost no work.
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #24025:
URL: https://github.com/apache/beam/pull/24025#issuecomment-1306347198
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] lukecwik commented on pull request #24025: [#24024] Stop wrapping light weight functions with Contextful as they add a lot of overhead for functions that are meant to do almost no work.
Posted by GitBox <gi...@apache.org>.
lukecwik commented on PR #24025:
URL: https://github.com/apache/beam/pull/24025#issuecomment-1306346116
R: @bhisevishal
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] lukecwik commented on pull request #24025: [#24024] Stop wrapping light weight functions with Contextful as they add a lot of overhead for functions that are meant to do almost no work.
Posted by GitBox <gi...@apache.org>.
lukecwik commented on PR #24025:
URL: https://github.com/apache/beam/pull/24025#issuecomment-1319111230
Run Java PreCommit
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] lukecwik commented on pull request #24025: [#24024] Stop wrapping light weight functions with Contextful as they add a lot of overhead for functions that are meant to do almost no work.
Posted by GitBox <gi...@apache.org>.
lukecwik commented on PR #24025:
URL: https://github.com/apache/beam/pull/24025#issuecomment-1312358758
Run Java PreCommit
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] lukecwik commented on pull request #24025: [#24024] Stop wrapping light weight functions with Contextful as they add a lot of overhead for functions that are meant to do almost no work.
Posted by GitBox <gi...@apache.org>.
lukecwik commented on PR #24025:
URL: https://github.com/apache/beam/pull/24025#issuecomment-1324284524
Run Java PreCommit
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] lukecwik commented on pull request #24025: [#24024] Stop wrapping light weight functions with Contextful as they add a lot of overhead for functions that are meant to do almost no work.
Posted by GitBox <gi...@apache.org>.
lukecwik commented on PR #24025:
URL: https://github.com/apache/beam/pull/24025#issuecomment-1325524746
Run Java PreCommit
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] lukecwik commented on pull request #24025: [#24024] Stop wrapping light weight functions with Contextful as they add a lot of overhead for functions that are meant to do almost no work.
Posted by GitBox <gi...@apache.org>.
lukecwik commented on PR #24025:
URL: https://github.com/apache/beam/pull/24025#issuecomment-1320342631
Run Java PreCommit
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] lukecwik commented on pull request #24025: [#24024] Stop wrapping light weight functions with Contextful as they add a lot of overhead for functions that are meant to do almost no work.
Posted by GitBox <gi...@apache.org>.
lukecwik commented on PR #24025:
URL: https://github.com/apache/beam/pull/24025#issuecomment-1324547446
Run Java PreCommit
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] lukecwik commented on pull request #24025: [#24024] Stop wrapping light weight functions with Contextful as they add a lot of overhead for functions that are meant to do almost no work.
Posted by GitBox <gi...@apache.org>.
lukecwik commented on PR #24025:
URL: https://github.com/apache/beam/pull/24025#issuecomment-1307598730
Run Java PreCommit
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] lukecwik commented on pull request #24025: [#24024] Stop wrapping light weight functions with Contextful as they add a lot of overhead for functions that are meant to do almost no work.
Posted by GitBox <gi...@apache.org>.
lukecwik commented on PR #24025:
URL: https://github.com/apache/beam/pull/24025#issuecomment-1311016105
Run Java PreCommit
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] lukecwik commented on pull request #24025: [#24024] Stop wrapping light weight functions with Contextful as they add a lot of overhead for functions that are meant to do almost no work.
Posted by GitBox <gi...@apache.org>.
lukecwik commented on PR #24025:
URL: https://github.com/apache/beam/pull/24025#issuecomment-1310357592
Run Java PreCommit
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] lukecwik commented on pull request #24025: [#24024] Stop wrapping light weight functions with Contextful as they add a lot of overhead for functions that are meant to do almost no work.
Posted by GitBox <gi...@apache.org>.
lukecwik commented on PR #24025:
URL: https://github.com/apache/beam/pull/24025#issuecomment-1319255512
Run Java PreCommit
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] lukecwik commented on pull request #24025: [#24024] Stop wrapping light weight functions with Contextful as they add a lot of overhead for functions that are meant to do almost no work.
Posted by GitBox <gi...@apache.org>.
lukecwik commented on PR #24025:
URL: https://github.com/apache/beam/pull/24025#issuecomment-1325243838
Run Java PreCommit
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] lukecwik commented on pull request #24025: [#24024] Stop wrapping light weight functions with Contextful as they add a lot of overhead for functions that are meant to do almost no work.
Posted by GitBox <gi...@apache.org>.
lukecwik commented on PR #24025:
URL: https://github.com/apache/beam/pull/24025#issuecomment-1311938895
Run Java PreCommit
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] lukecwik commented on pull request #24025: [#24024] Stop wrapping light weight functions with Contextful as they add a lot of overhead for functions that are meant to do almost no work.
Posted by GitBox <gi...@apache.org>.
lukecwik commented on PR #24025:
URL: https://github.com/apache/beam/pull/24025#issuecomment-1320679343
Run Java PreCommit
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] lukecwik commented on pull request #24025: [#24024] Stop wrapping light weight functions with Contextful as they add a lot of overhead for functions that are meant to do almost no work.
Posted by GitBox <gi...@apache.org>.
lukecwik commented on PR #24025:
URL: https://github.com/apache/beam/pull/24025#issuecomment-1325426065
Run Java PreCommit
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] lukecwik commented on a diff in pull request #24025: [#24024] Stop wrapping light weight functions with Contextful as they add a lot of overhead for functions that are meant to do almost no work.
Posted by GitBox <gi...@apache.org>.
lukecwik commented on code in PR #24025:
URL: https://github.com/apache/beam/pull/24025#discussion_r1035252492
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/FlatMapElements.java:
##########
@@ -137,57 +132,90 @@ public <NewInputT> FlatMapElements<NewInputT, OutputT> via(
@Experimental(Kind.CONTEXTFUL)
public <NewInputT> FlatMapElements<NewInputT, OutputT> via(
Contextful<Fn<NewInputT, Iterable<OutputT>>> fn) {
- return new FlatMapElements<>(
- fn, fn.getClosure(), TypeDescriptors.inputOf(fn.getClosure()), outputType);
+ return new FlatMapElements<>(fn, TypeDescriptors.inputOf(fn.getClosure()), outputType);
}
@Override
public PCollection<OutputT> expand(PCollection<? extends InputT> input) {
checkArgument(fn != null, ".via() is required");
- return input.apply(
- "FlatMap",
- ParDo.of(
- new DoFn<InputT, OutputT>() {
- @ProcessElement
- public void processElement(ProcessContext c) throws Exception {
- Iterable<OutputT> res =
- fn.getClosure().apply(c.element(), Fn.Context.wrapProcessContext(c));
- for (OutputT output : res) {
- c.output(output);
+ if (fn instanceof Contextful) {
Review Comment:
Also, I didn't want to wrap with another PTransform that delineated between these two options
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org