Internal optimisations

[jponge]

A set of performance-oriented optimisations and cleanups.

[jponge]

Todo: add a consolidating summary in the PR description when ready for review

[codecov]

Codecov Report

Merging #377 (3c6f46a) into master (4c9cceb) will increase coverage by 0.16%.
The diff coverage is 92.20%.

@@             Coverage Diff              @@
##             master     #377      +/-   ##
============================================
+ Coverage     89.82%   89.99%   +0.16%     
- Complexity     2768     2776       +8     
============================================
  Files           369      370       +1     
  Lines         10597    10624      +27     
  Branches       1298     1304       +6     
============================================
+ Hits           9519     9561      +42     
+ Misses          570      568       -2     
+ Partials        508      495      -13     

Impacted Files	Coverage Δ	Complexity Δ
...smallrye/mutiny/infrastructure/Infrastructure.java	67.64% <75.00%> (-3.63%)	25.00 <8.00> (+2.00)	⬇️
...rye/mutiny/converters/uni/UniToMultiPublisher.java	88.88% <88.88%> (ø)	2.00 <2.00> (?)
...io/smallrye/mutiny/converters/uni/ToPublisher.java	100.00% <100.00%> (+12.90%)	2.00 <1.00> (-1.00)	⬆️
...o/smallrye/mutiny/helpers/ParameterValidation.java	100.00% <100.00%> (ø)	24.00 <0.00> (ø)
...va/io/smallrye/mutiny/operators/AbstractMulti.java	100.00% <100.00%> (+2.94%)	28.00 <2.00> (ø)
...java/io/smallrye/mutiny/operators/AbstractUni.java	86.20% <100.00%> (ø)	23.00 <1.00> (ø)
...lrye/mutiny/operators/UniSerializedSubscriber.java	92.30% <100.00%> (ø)	15.00 <6.00> (ø)
...mallrye/mutiny/operators/multi/MultiFlatMapOp.java	85.04% <100.00%> (+0.57%)	3.00 <0.00> (ø)
...rye/mutiny/operators/multi/MultiRepeatUntilOp.java	100.00% <100.00%> (ø)	5.00 <0.00> (ø)
...ye/mutiny/operators/multi/MultiRepeatWhilstOp.java	86.20% <100.00%> (+2.87%)	2.00 <0.00> (ø)
... and 19 more

[jponge]

It's ok to fail, it needs #378

[jponge]

Status: currently breaking due to cd92c55 depending on #379

[cescoffier]

I don't believe we should call onError here, if the failure comes from the downstream. You need to cancel, but I don't think we need to send a second event.

[jponge]

If we just do:

if (upstream.getAndSet(CANCELLED) != CANCELLED) {
    if (item != null) {
      downstream.onNext(item);
    }
    downstream.onComplete();
}

then tests fail in UniRepeatTest and UniCastProcessorTest fail.

Handling exceptions as dropped exceptions does not work either, the downstream in these tests does want the error:

if (upstream.getAndSet(CANCELLED) != CANCELLED) {
    if (item != null) {
        try {
            downstream.onNext(item);
        } catch (Throwable failure) {
            Infrastructure.handleDroppedException(failure);
            return;
        }
    }
    downstream.onComplete();
}

[jponge]

(basically calling onError does preserve the existing behaviour)

[cescoffier]

I can only accept that argument!

…

On Fri 8 Jan 2021 at 10:09, Julien Ponge ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In implementation/src/main/java/io/smallrye/mutiny/converters/uni/ToPublisher.java <#377 (comment)> : > @@ -22,74 +27,8 @@ private ToPublisher() { @OverRide public Publisher<T> apply(Uni<T> uni) { Yes, it's clearer when looking at stack traces believe me :-) — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#377 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADCG7LRTYYYXMFEZY22Q33SY3DVTANCNFSM4UKLOCEA> .

[cescoffier]

Ok, we would need to see why these other tests require this second signal as the failure comes from downstream. It is fine because the downstream being failed the event is dropped.

…

On Fri 8 Jan 2021 at 10:20, Julien Ponge ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In implementation/src/main/java/io/smallrye/mutiny/converters/uni/UniToMultiPublisher.java <#377 (comment)> : > + @OverRide + public void onSubscribe(UniSubscription subscription) { + if (!upstream.compareAndSet(null, subscription)) { + downstream.onError(new IllegalStateException( + "Invalid subscription state - already have a subscription for upstream")); + } + } + + @OverRide + public void onItem(T item) { + if (upstream.getAndSet(CANCELLED) != CANCELLED) { + if (item != null) { + try { + downstream.onNext(item); + } catch (Throwable failure) { + downstream.onError(failure); (basically calling onError does preserve the existing behaviour) — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#377 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADCG7JAABESMF6NVCUVJBDSY3E7BANCNFSM4UKLOCEA> .

[jponge]

Looking into the corresponding operators, looks like they don't handle predicates throwing an exception as they should.

[cescoffier]

Here we go :-)

…

On Fri 8 Jan 2021 at 10:43, Julien Ponge ***@***.***> wrote: Looking into the corresponding operators, looks like they don't handle predicates throwing an exception as they should. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#377 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADCG7PN7PVLES3R64PKLY3SY3HUPANCNFSM4UKLOCEA> .