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

=str Make decider caculation lazy #627

Merged
merged 1 commit into from
Sep 27, 2023
Merged

=str Make decider caculation lazy #627

merged 1 commit into from
Sep 27, 2023

Conversation

He-Pin
Copy link
Member

@He-Pin He-Pin commented Sep 3, 2023

Motivation:
We don't expect much exception throws, and it's lazy val in MapAsync, so better to keep those two the same.

Result:
Consistent with MapAsync.

@He-Pin He-Pin requested a review from pjfanning September 3, 2023 12:44
@mdedetrich
Copy link
Contributor

So I don't know what to really think of this, I know that in other cases we use lazy for the decider however its kind of arbitrary whether its strict or lazy and I am actually leaning towards it being strict because it is slightly faster.

@jxnu-liguobin
Copy link
Member

We don't expect much exception throws, and it's lazy val in MapAsync, so better to keep those two the same.

Is this just guess? Because it's Try

@He-Pin
Copy link
Member Author

He-Pin commented Sep 26, 2023

@jxnu-liguobin Noop, I think it was not lazy just because out of sight in the first place.

@@ -1402,8 +1402,7 @@ private[stream] object Collect {
new GraphStageLogic(shape) with InHandler with OutHandler {
override def toString = s"MapAsyncUnordered.Logic(inFlight=$inFlight, buffer=$buffer)"

private val decider =
inheritedAttributes.mandatoryAttribute[SupervisionStrategy].decider
private lazy val decider = inheritedAttributes.mandatoryAttribute[SupervisionStrategy].decider
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we make this a def? In most places in this source file, this is a def.

The def means no synchronize block. lazy vals come with the overhead of having synchronize in the generated byte code.

Copy link
Member Author

@He-Pin He-Pin Sep 26, 2023

Choose a reason for hiding this comment

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

  1. def will do recaculation, and that should be avoid.
  2. lazy val will only do synchronization the first time, and the supervision strategy method is expected to be quick for calculation, so even the undering Thread is a VirtualThread, it will not cause any problem either.

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 changed the title =str Make decider in MapAsyncUnordered lazy. =str Make decider caculation lazy Sep 26, 2023
@He-Pin He-Pin requested a review from pjfanning September 26, 2023 12:58
@He-Pin He-Pin closed this Sep 26, 2023
@He-Pin He-Pin reopened this Sep 26, 2023
@pjfanning
Copy link
Contributor

pjfanning commented Sep 26, 2023

You don't need to close a PR to rerun the tests. Go to the broken build and there are buttons to rerun failed builds. You can rerun the one build that failed instead of having every single build run again.

@He-Pin
Copy link
Member Author

He-Pin commented Sep 26, 2023

the streamRefspec is flasky

@He-Pin He-Pin merged commit cdfcc56 into apache:main Sep 27, 2023
33 of 34 checks passed
@He-Pin He-Pin deleted the lazyDecider branch September 27, 2023 06: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.

4 participants