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.

Sign up for GitHub

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

Jump to bottom

fix: avoid boxing in zipWithIndex and fix type signature in SubSource#zipWithIndex #1669

Merged
merged 1 commit into from
Jan 7, 2025
Merged

fix: avoid boxing in zipWithIndex and fix type signature in SubSource#zipWithIndex #1669

merged 1 commit into from
Jan 7, 2025

Conversation

He-Pin
Copy link
Member

@He-Pin He-Pin commented Jan 3, 2025 •
edited
Loading

Motivation:

  1. Performance, but it was reverted in zipWithIndex is broken for org.apache.pekko.stream.scaladsl.GraphDSL.Implicits.PortOps #1525
    instead of chore: Implement zipWithIndex with statefulMap #1657
  2. signature on SubSource#zipWithIndex returns a Pair <T, Object> instead of Pair<T, java.lang.Long>, which is a bug

Modification:
Reimplemented it in a dedicated graph stage.

Result:
Better performance and fix SubSource#zipWithIndex

@He-Pin He-Pin added the t:stream Pekko Streams label Jan 3, 2025
@He-Pin He-Pin added this to the 1.2.0 milestone Jan 3, 2025
He-Pin
He-Pin commented Jan 3, 2025
View reviewed changes
stream/src/main/scala/org/apache/pekko/stream/javadsl/SubSource.scala
@@ -2246,8 +2247,9 @@ class SubSource[Out, Mat](
*
* '''Cancels when''' downstream cancels
*/
def zipWithIndex: javadsl.SubSource[pekko.japi.Pair[Out @uncheckedVariance, Long], Mat] =
new SubSource(delegate.zipWithIndex.map { case (elem, index) => pekko.japi.Pair(elem, index) })
Copy link
Member Author

Choose a reason for hiding this comment

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

too much boxing for Java users, that's unfair.

Copy link
Member Author

Choose a reason for hiding this comment

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

image Fixed now.

@He-Pin He-Pin force-pushed the zipWithIndexOpt branch 2 times, most recently from 46042b8 to de96089 Compare January 4, 2025 03:27
@He-Pin He-Pin requested a review from pjfanning January 4, 2025 03:28
@He-Pin He-Pin added the bug Something isn't working label Jan 4, 2025
@He-Pin He-Pin mentioned this pull request Jan 4, 2025
Closed
@He-Pin He-Pin force-pushed the zipWithIndexOpt branch 2 times, most recently from 948c53b to d197bf7 Compare January 4, 2025 04:15
@He-Pin He-Pin requested review from raboof and mdedetrich January 4, 2025 04:16
mdedetrich
mdedetrich approved these changes Jan 4, 2025
View reviewed changes
Copy link
Contributor

@mdedetrich mdedetrich left a comment

Choose a reason for hiding this comment

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

lgtm, would be great if @raboof could also review it (hopefully can avoid making a regression this time).

@He-Pin
Copy link
Member Author

He-Pin commented Jan 4, 2025

@mdedetrich Yes, with additional test added and actually find a bug in the subSource

@pjfanning
Copy link
Contributor

#1526 added a test so we shold be able to avoid the same break as before - but we could still hit issues in other untested scenarios

@He-Pin He-Pin requested a review from pjfanning January 4, 2025 09:57
@pjfanning pjfanning removed the bug Something isn't working label Jan 4, 2025
@pjfanning
Copy link
Contributor

removed 'bug' label - this is a performance improvement according to the description

@He-Pin
Copy link
Member Author

He-Pin commented Jan 4, 2025

@pjfanning it's a bug too, the signature onSubSource returns a Pair <T, Object> instead of Pair<T, java.lang.Long>

@pjfanning
Copy link
Contributor

@pjfanning it's a bug too, the signature onSubSource returns a Pair <T, Object> instead of Pair<T, java.lang.Long>

Can you fix the description then to mention this?

@He-Pin He-Pin changed the title perf: avoid boxing in zipWithIndex fix: avoid boxing in zipWithIndex and fix type signature in SubSource#zipWithIndex Jan 4, 2025
@He-Pin
Copy link
Member Author

He-Pin commented Jan 7, 2025

@pjfanning @mdedetrich @raboof Would you like to give this another look , the confit have been fixed.

pjfanning
pjfanning approved these changes Jan 7, 2025
View reviewed changes
Copy link
Contributor

@pjfanning pjfanning left a comment

Choose a reason for hiding this comment

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

lgtm

@He-Pin He-Pin merged commit 41b4cab into apache:main Jan 7, 2025
9 checks passed
@He-Pin He-Pin deleted the zipWithIndexOpt branch January 7, 2025 13:22
He-Pin added a commit to He-Pin/incubator-pekko that referenced this pull request Jan 7, 2025
@He-Pin
fix: avoid boxing in zipWithIndex and fix type signature in SubSource…
640a9e9 
…#zipWithIndex (apache#1669)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pjfanning pjfanning pjfanning approved these changes

@mdedetrich mdedetrich mdedetrich approved these changes

@raboof raboof Awaiting requested review from raboof

Assignees
No one assigned
Labels
t:stream Pekko Streams
Projects
None yet
Milestone
1.2.0
Development

Successfully merging this pull request may close these issues.
3 participants
@He-Pin @pjfanning @mdedetrich