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-14128] Eliminating quadratic behavior of computeCompositeInputOutput #17120

Merged
merged 4 commits into from
Mar 18, 2022

Conversation

lukakalinovcic
Copy link
Contributor

@lukakalinovcic lukakalinovcic commented Mar 18, 2022

Eliminating quadratic behavior of computeCompositeInputOutput by using a lookup map to find transforms that use a particular collection.

Benchmarking on a graph with 6000 transforms shows a speedup from 1.5s to 15ms, for a memory increase of 8%.

R: @lostluck

…g a lookup map to find transforms that use a particular collection.

Benchmarking on a graph with 6000 transforms shows a speedup from 1.5s to 0.015ms, for a memory increase of 8%.
@github-actions github-actions bot added the go label Mar 18, 2022
@github-actions
Copy link
Contributor

Assigning reviewers. If you would like to opt out of this review, comment assign to next reviewer:

R: @riteshghorse for label go.

Available commands:

  • stop reviewer notifications - opt out of the automated review tooling
  • remind me after tests pass - tag the comment author after tests pass
  • waiting on author - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)

@aaltay aaltay requested a review from lostluck March 18, 2022 16:45
@lostluck lostluck changed the title Eliminating quadratic behavior of computeCompositeInputOutput [BEAM-14128] Eliminating quadratic behavior of computeCompositeInputOutput Mar 18, 2022
@codecov
Copy link

codecov bot commented Mar 18, 2022

Codecov Report

Merging #17120 (a31fcc7) into master (31ed331) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master   #17120   +/-   ##
=======================================
  Coverage   73.96%   73.96%           
=======================================
  Files         671      671           
  Lines       88221    88227    +6     
=======================================
+ Hits        65250    65256    +6     
  Misses      21859    21859           
  Partials     1112     1112           
Flag Coverage Δ
go 49.45% <100.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
sdks/go/pkg/beam/core/runtime/pipelinex/replace.go 66.66% <100.00%> (+0.71%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 31ed331...a31fcc7. Read the comment docs.

@lostluck
Copy link
Contributor

Thank you very much for the contribution!

I filed the JIRA (https://issues.apache.org/jira/browse/BEAM-14128) for this issue, and touched up the PR title and description.

I have a few comment lines that I'll be adding to the PR myself before I merge this in.

Copy link
Contributor

@lostluck lostluck left a comment

Choose a reason for hiding this comment

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

I have one question about the benchmark so we can add a clarifying comment, but thank you for a clean improvement for large complex graphs!

sdks/go/pkg/beam/core/runtime/pipelinex/replace.go Outdated Show resolved Hide resolved
sdks/go/pkg/beam/core/runtime/pipelinex/replace.go Outdated Show resolved Hide resolved
Copy link
Contributor Author

@lukakalinovcic lukakalinovcic 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 review! Comments addressed.

Copy link
Contributor

@lostluck lostluck left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you! Will merge shortly.

@lostluck lostluck merged commit 65df6af into apache:master Mar 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants