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

Make some Mono sources and aggregators lazier #3081

Merged
merged 25 commits into from Aug 3, 2022
Merged

Make some Mono sources and aggregators lazier #3081

merged 25 commits into from Aug 3, 2022

Conversation

OlegDokuka
Copy link
Contributor

Signed-off-by: Oleh Dokuka odokuka@vmware.com

@OlegDokuka
adds support for lazy evaluation for MonoCallable
18c36c7 
Signed-off-by: Oleh Dokuka <odokuka@vmware.com>
@OlegDokuka
fixes callable tests
8590947 
Signed-off-by: Oleh Dokuka <odokuka@vmware.com>
@OlegDokuka
adds lazy execution to MonoAll
743ec19 
Signed-off-by: Oleh Dokuka <odokuka@vmware.com>
@OlegDokuka
adds lazy execution to MonoZip
a34b17a 
Signed-off-by: Oleh Dokuka <odokuka@vmware.com>
@OlegDokuka
removes async fusion checking tests
6f69252 
Signed-off-by: Oleh Dokuka <odokuka@vmware.com>
@OlegDokuka
adds discard support for monoAll
1e12d77 
Signed-off-by: Oleh Dokuka <odokuka@vmware.com>
@OlegDokuka
simplifies MonoCallable impl
ef2afb2 
Signed-off-by: Oleh Dokuka <odokuka@vmware.com>
@OlegDokuka
makes MonoAny lazy
42fa21f 
Signed-off-by: Oleh Dokuka <odokuka@vmware.com>
@OlegDokuka
fixes callable behaviours
eeb0307 
Signed-off-by: Oleh Dokuka <odokuka@vmware.com>
@OlegDokuka
makes MonoCount lazy
576be09 
Signed-off-by: Oleh Dokuka <odokuka@vmware.com>
@OlegDokuka
fixes failing tests
274505f 
Signed-off-by: Oleh Dokuka <odokuka@vmware.com>
Signed-off-by: SerhiiPanasiuk <odokuka@vmware.com>
@OlegDokuka
makes ParallelThen lazy
1c9c45c 
Signed-off-by: Oleh Dokuka <odokuka@vmware.com>
Signed-off-by: SerhiiPanasiuk <odokuka@vmware.com>
@OlegDokuka
makes MonoFlatMap lazy
4be49da 
Signed-off-by: Oleh Dokuka <odokuka@vmware.com>
Signed-off-by: SerhiiPanasiuk <odokuka@vmware.com>
@OlegDokuka
fixes tests
14be9e8 
Signed-off-by: Oleh Dokuka <odokuka@vmware.com>
Signed-off-by: SerhiiPanasiuk <odokuka@vmware.com>
@OlegDokuka
makes MonoCollectList lazy
8f2a7c4 
Signed-off-by: Oleh Dokuka <odokuka@vmware.com>
Signed-off-by: SerhiiPanasiuk <odokuka@vmware.com>
@OlegDokuka
fixes tests
c3f0e96 
Signed-off-by: Oleh Dokuka <odokuka@vmware.com>
Signed-off-by: SerhiiPanasiuk <odokuka@vmware.com>
@OlegDokuka
makes MonoCollect lazy
9ac6e9d 
Signed-off-by: Oleh Dokuka <odokuka@vmware.com>
Signed-off-by: SerhiiPanasiuk <odokuka@vmware.com>
@OlegDokuka
reworks flux to mono operators to support backpressure
b786d15 
in case of empty sources flux to mono operators emits a value which in that case may violate backpressure components of RS spec. Thus, this PR reworks implementation and adds lighter ways of checking if there is request using non-volatile field and extra volatile int state

Signed-off-by: Oleh Dokuka <odokuka@vmware.com>
Signed-off-by: OlegDokuka <odokuka@vmware.com>
@OlegDokuka
makes MonoStreamCollector lazy
12382e0 
Signed-off-by: Oleh Dokuka <odokuka@vmware.com>
Signed-off-by: OlegDokuka <odokuka@vmware.com>
simonbasle
simonbasle requested changes Jul 13, 2022
View reviewed changes
Copy link
Member

@simonbasle simonbasle left a comment

Choose a reason for hiding this comment

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

I know this is in an intermediate state, but I've commented anyway.
I like the direction it is going with the hasRequest/state 👍

things that needs to be addressed in the final revision:

  • scan support of at least TERMINATED, instead of removing the test lines that check this attribute (there is no reason the attribute couldn't be supported thanks to state)
  • test coverage regression: some tests for operator X use one of the modified operators Y (eg. count) to trigger a fusion path. now operator Y doesn't support fusion anymore => the test is deleted 😨. since operator X does support fusion, the case has to be covered which means we need to find an alternative to Y instead of purely deleting the whole test
  • as discussed, you already intend to mutualize some code around QueueSubscription that always negotiate Fuseable.NONE in a common base class, which I think makes sense. this would allow fixing the bug of isEmpty() == false && size() == 0 in one go :) that said, we might want to reconsider some of these fusion deactivation/removal...

reactor-core/src/main/java/reactor/core/publisher/FluxDefaultIfEmpty.java Outdated Show resolved Hide resolved
reactor-core/src/main/java/reactor/core/publisher/MonoAll.java Outdated Show resolved Hide resolved
reactor-core/src/main/java/reactor/core/publisher/MonoCollect.java Outdated Show resolved Hide resolved
reactor-core/src/main/java/reactor/core/publisher/MonoCollectList.java Outdated Show resolved Hide resolved
reactor-core/src/main/java/reactor/core/publisher/MonoFlatMap.java Outdated Show resolved Hide resolved
reactor-core/src/main/java/reactor/core/publisher/MonoStreamCollector.java Outdated Show resolved Hide resolved
reactor-core/src/main/java/reactor/core/publisher/ParallelThen.java Outdated Show resolved Hide resolved
reactor-core/src/test/java/reactor/core/publisher/FluxDoOnEachTest.java Show resolved Hide resolved
reactor-core/src/test/java/reactor/core/publisher/FluxDefaultIfEmptyTest.java Outdated Show resolved Hide resolved
reactor-core/src/test/java/reactor/core/publisher/MonoCountTest.java Show resolved Hide resolved
@OlegDokuka
migrates remaining operators and generify impls
305ab49 
Signed-off-by: Oleh Dokuka <odokuka@vmware.com>
Signed-off-by: OlegDokuka <odokuka@vmware.com>
@OlegDokuka
fixes discard issue
c0995ca 
Signed-off-by: Oleh Dokuka <odokuka@vmware.com>
Signed-off-by: OlegDokuka <odokuka@vmware.com>
@OlegDokuka OlegDokuka marked this pull request as ready for review July 27, 2022 13:05
@OlegDokuka OlegDokuka requested a review from a team as a code owner July 27, 2022 13:05
simonbasle
simonbasle requested changes Jul 28, 2022
View reviewed changes
Copy link
Member

@simonbasle simonbasle left a comment

Choose a reason for hiding this comment

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

shaping up good, this is a big one !
I have found some issues and also added more minor comments / questions

reactor-core/src/main/java/reactor/core/publisher/Operators.java Outdated Show resolved Hide resolved
reactor-core/src/main/java/reactor/core/publisher/MonoReduce.java Show resolved Hide resolved
reactor-core/src/main/java/reactor/core/publisher/MonoReduce.java Outdated
this.aggregator = aggregator;
//noinspection unchecked
this.aggregate = (T) INITIAL_STATE;
Copy link
Member

Choose a reason for hiding this comment

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

this feels a bit hacky but I understand 😅

my concern is that it becomes too easy to add code like actual.onNext(aggregate) while forgetting the aggregate != INITIAL_STATE check beforehand, as this wouldn't trigger any compiler error.

having an Object field instead kinda forces us to at least add an explicit cast, which in turns should raise our awareness (and shouldn't need warning suppression either).

instead of 1 cast, it would mean at least 2 though:

  • when the BiFunction is applied in onNext (with slightly more work, 1 more local variable to track probably)
  • when actual.onNext(r) is called in onComplete

wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

☑️

reactor-core/src/main/java/reactor/core/publisher/MonoTakeLastOne.java Show resolved Hide resolved
reactor-core/src/main/java/reactor/core/publisher/Operators.java Show resolved Hide resolved
reactor-core/src/main/java/reactor/core/publisher/MonoTakeLastOne.java Show resolved Hide resolved
reactor-core/src/main/java/reactor/core/publisher/MonoZip.java Show resolved Hide resolved
reactor-core/src/test/java/reactor/core/publisher/MonoReduceSeedTest.java Outdated Show resolved Hide resolved
reactor-core/src/test/java/reactor/core/publisher/MonoReduceTest.java Outdated Show resolved Hide resolved
reactor-core/src/test/java/reactor/core/publisher/ParallelCollectTest.java Outdated Show resolved Hide resolved
OlegDokuka and others added 3 commits August 1, 2022 16:29
@OlegDokuka
fixes comments
27de820 
Signed-off-by: Oleh Dokuka <odokuka@vmware.com>
Signed-off-by: OlegDokuka <odokuka@vmware.com>
@simonbasle
added comment on the MonoZip cancelledSubscription trick
6991d1d
@OlegDokuka
adds onDiscard checks and fixes related issues
38a3ece 
Signed-off-by: Oleh Dokuka <odokuka@vmware.com>
Signed-off-by: OlegDokuka <odokuka@vmware.com>
@OlegDokuka
apply spotless
6fb28a5 
Signed-off-by: Oleh Dokuka <odokuka@vmware.com>
Signed-off-by: OlegDokuka <odokuka@vmware.com>
simonbasle
simonbasle approved these changes Aug 3, 2022
View reviewed changes
Copy link
Member

@simonbasle simonbasle left a comment

Choose a reason for hiding this comment

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

nice ! finally ready to merge 😄 🎉

@simonbasle simonbasle changed the title Adds support for lazy evaluation to Mono producers Make some Mono sources and aggregators lazier Aug 3, 2022
@simonbasle simonbasle linked an issue Aug 3, 2022 that may be closed by this pull request
Lazier Monos: act on first request rather than onSubscribe #2913
Closed
1 task
@simonbasle simonbasle added this to the 3.5.0-M5 milestone Aug 3, 2022
@simonbasle simonbasle added type/enhancement A general enhancement wide-change warn/behavior-change Breaking change of publicly advertised behavior labels Aug 3, 2022
@OlegDokuka OlegDokuka merged commit 88587fc into main Aug 3, 2022
This was referenced Aug 3, 2022
Open
Closed
@chemicL chemicL deleted the feature/lazy-mono branch April 11, 2024 12:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simonbasle simonbasle simonbasle approved these changes

Assignees

@OlegDokuka OlegDokuka

Labels
type/enhancement A general enhancement warn/behavior-change Breaking change of publicly advertised behavior wide-change
Projects
None yet
Milestone
3.5.0-M5
Development

Successfully merging this pull request may close these issues.

Lazier Monos: act on first request rather than onSubscribe
2 participants
@OlegDokuka @simonbasle