On 3 Aug 2016, at 13:54, Dan Berindei <dan.berindei(a)gmail.com>
wrote:
Yeah, I just fixed (almost all of) the test failures this morning and
I pushed it here:
https://github.com/infinispan/infinispan/compare/master...danberindei:ISP...
Galder, I'm not sure I understand the part about mixing promises and
callbacks... almost all CompletableFuture methods take a callback
parameter, so I'm not sure how one could avoid mixing them.
The question how these callbacks are passed:
When using Promises or CompletableFutures, the callbacks are passed to the promise/CF via
its fluent API, e.g:
CompletableFuture cf = methodA();
cf.thenCompose().thenCombine().handle().whenComplete()... // Might not be realistic, just
showing fluency in action
In your case, the forkHandler is passed in as parameter to methodA() instead of it being a
continuation that you add to the CompletableFuture via its fluent API.
Passing handlers as parameters to a method you created (and not as parameters to a
continuation of CompletableFuture) is the old asynchronous call handling method that
Node.js made so famous. The current API mixes both, by passing some callbacks to
CompletableFuture methods and then some being passed around as method parameters, which
makes it ackward.
So, instead of:
CompletableFuture<Void> forkInvocation(VisitableCommand newCommand,
AsyncInterceptor.ForkReturnHandler forkReturnHandler)
I'd expect:
CompletableFuture<Z> forkInvocation(VisitableCommand newCommand);
And use it like this:
CompletableFuture<Z> cf = forkInvocation(cmd);
cf.whenComplete(forkReturnHandler); //
So, what is forkReturnHandler? forkReturnHandler would be a BiConsumer<Z,
Throwable>.
Now, what is Z? Z is everything that the handler needs to deal with, except Throwable,
which gets passed as 2nd element to the BiConsumer. So Z would a wrapper around
InvocationContext, VisitableCommand and Object (return value).
That being
said, the API as it is in master now tends to encourage nesting of
callbacks, and that's what I'm trying to change.
My new approach has a promise-like interface called
IntermediateInvocationStage (for now), which works like a promise on
the interceptors not yet executed. It has various methods you can then
use to add callbacks to be executed after those interceptors (and any
callbacks they installed).
To get an IntermediateInvocationStage, you need to call a method like
invokeNext(ctx, command) on BaseAsyncInterceptor/InterceptorStage. I'm
not yet sure whether exposing InterceptorStage to the interceptor is
really necessary.
Both the interceptor and the callbacks then return an InvocationStage,
which is implemented by the same classes that implement
IntermediateInvocationStage, but doesn't allow adding more callbacks.
This is mainly so that I can have returnWith(returnValue) instead of
returnWith(ctx, command, returnValue).
For the actual asynchronous work, I have 2 goAsync methods in
InterceptorStage, because I think having an explicit callback can be
slightly more efficient - I'm not 100% sure about this either. I used
to have a waitFor(CompletionStage<?>) method that returned an
InterceptorStage, but I decided it was too confusing to have
InterceptorStage behave like a promise as well.
IntermediateInvocationStage also allows "asynchronous" callbacks with
composeAsync, but it seems a bit weird to have a single callback
handle both on an InvocationStage and on a CompletionStage. So I'm
thinking of removing that, and forcing the interceptors to use
compose+goAsync instead. That probably means callback nesting again,
so I need to check how it would affect the L1 interceptors.
About performance: I only tested non-tx local reads so far, but in
that case I'm only getting one real allocation in CallInterceptor,
which gets reused by all the other interceptors. For this scenario all
the callbacks are cached in the interceptor fields, of course, so it's
not HotSpot doing scalar replacement for the callbacks - although in
theory that should work as well. I need to do a lot more testing on
this...
Cheers
Dan
On Wed, Aug 3, 2016 at 1:49 PM, Radim Vansa <rvansa(a)redhat.com> wrote:
> I think that Dan had another branch with more FSM-like API, but I can't
> find it :-/
>
> R.
>
> On 08/03/2016 11:57 AM, Galder Zamarreño wrote:
>> Hey Radim,
>>
>> Sorry for the delay having a look at this (Summit/DevNation then vacation).
>>
>> I agree with you that the way you're passing down the handler down the stack
and accross lambdas is a bit counter-productive, because it means the lambda has to
capture the handler, which in turn means that each lambda execution will result in a
lambda instantiation.
>>
>> Looking at AsyncInvocationContext, methods like this feel a bit of an
anti-pattern:
>>
>> CompletableFuture<Void> forkInvocation(VisitableCommand newCommand,
AsyncInterceptor.ForkReturnHandler forkReturnHandler)
>>
>> The problem here is that it mixes the two main ways of dealing with async
computation:
>>
>> 1. Callback handlers [1].
>> 2. Promises (~ CompletableFuture) [2].
>>
>> Normally, you either use one or the other, but here both are in use because the
method returns a CompletableFuture (e.g. like a JS/Scala promise), and then is taking a
callback handler as well.
>>
>> If you go down the path of promises, the normal thing would be:
>>
>> CompletableFuture<Void> forkInvocation(VisitableCommand newCommand)
>>
>> And then you chain (e.g. thenCompose or similar) the return function when the
fork has completed. I assume that going down this route causes unnecessary allocations?
>>
>> If we're worried about allocations, I wonder whether a pure handler based
approach might work better? It might not be as user-friendly but if we want to avoid
CompletableFuture issues internally, then maybe that's the way? e.g.
>>
>> void forkInvocation(VisitableCommand newCommand,
AsyncInterceptor.ForkReturnHandler forkReturnHandler);
>>
>> While at Summit/DevNation I had a chat with Clement from Vert.x team who had some
reservations about CompletableFutures themselves. I don't remember the details right
now (that's what vacation does to you!) but I've pinged him to find out more.
>>
>> In general though, there are multiple issues associated with the handler
approach, including:
>>
>> * Lack of central error handling logic.
>> * Callback readibility, when multiple handlers are specified in one go [3].
>>
>> Cheers,
>>
>> [1]
http://www.servicedesignpatterns.com/WebServiceInfrastructures/AsyncRespo...
>> [2]
http://exploringjs.com/es6/ch_promises.html
>> [3]
http://callbackhell.com
>> --
>> Galder Zamarreño
>> Infinispan, Red Hat
>>
>>> On 22 Jun 2016, at 17:52, Radim Vansa <rvansa(a)redhat.com> wrote:
>>>
>>> Yes [1]. The longest chaining of operations I had in [2], basically
>>> during ST I have to load a value locally*, perform a unicast/broadcast
>>> to read different value and then execute the original one.
>>>
>>> * I shouldn't load it just from DC, as it could be in cache store, too;
>>> though without persistence (which I don't deal with properly in
>>> scattered cache yet) it would be more efficient to do the DC lookup
>>> directly.
>>>
>>> Radim
>>>
>>> [1]
https://github.com/rvansa/infinispan/tree/ISPN-6645
>>> [2]
>>>
https://github.com/rvansa/infinispan/blob/ISPN-6645/core/src/main/java/or...
>>>
>>> On 06/17/2016 10:52 AM, Galder Zamarreño wrote:
>>>> Radim, do you have a branch where you have been trying these things out?
I'd like to play with what you're trying to do.
>>>>
>>>> Cheers,
>>>> --
>>>> Galder Zamarreño
>>>> Infinispan, Red Hat
>>>>
>>>>> On 8 Jun 2016, at 14:23, Radim Vansa <rvansa(a)redhat.com>
wrote:
>>>>>
>>>>> Hi,
>>>>>
>>>>> I would like to encourage you to play with the (relatively) new API
for
>>>>> sequential interceptors, and voice your comments - especially you
corish
>>>>> devs, and Galder who has much experience with async invocations and
that
>>>>> kind of stuff from JS-world.
>>>>>
>>>>> I am now trying to use the asynchronous methods only (the
>>>>> forkInvocationSync() is only temporary helper!); Dan has made it
this
>>>>> way as he wanted to avoid unnecessary allocations, and I welcome
this
>>>>> GC-awareness, but regrettably I find it rather hard to use, due to
its
>>>>> handler-style nature. For the simplest style interceptors (do this,
>>>>> invoke next interceptor, and process result) it's fine, but when
you
>>>>> want to do something like:
>>>>>
>>>>> visitFoo(cmd) {
>>>>> Object x = null;
>>>>> if (/* ... */) {
>>>>> x = invoke(new OtherCommand());
>>>>> }
>>>>> invoke(new DifferentCommand(x));
>>>>> Object retval = invoke(cmd);
>>>>> return wrap(retval);
>>>>> }
>>>>>
>>>>> I find myself passing handlers deep down. There is allocation cost
for
>>>>> closures, so API that does not allocate CompletableFutures does not
pay off.
>>>>>
>>>>> I don't say that I could improve it (I have directed my comments
to Dan
>>>>> on IRC when I had something in particular), I just say that this is
very
>>>>> important API for further Infinispan development and everyone should
pay
>>>>> attention before it gets final.
>>>>>
>>>>> So please, play with it, and show your opinion.
>>>>>
>>>>> Radim
>>>>>
>>>>> --
>>>>> Radim Vansa <rvansa(a)redhat.com>
>>>>> JBoss Performance Team
>>>>>
>>>>> _______________________________________________
>>>>> infinispan-dev mailing list
>>>>> infinispan-dev(a)lists.jboss.org
>>>>>
https://lists.jboss.org/mailman/listinfo/infinispan-dev
>>>> _______________________________________________
>>>> infinispan-dev mailing list
>>>> infinispan-dev(a)lists.jboss.org
>>>>
https://lists.jboss.org/mailman/listinfo/infinispan-dev
>>>
>>> --
>>> Radim Vansa <rvansa(a)redhat.com>
>>> JBoss Performance Team
>>>
>>> _______________________________________________
>>> infinispan-dev mailing list
>>> infinispan-dev(a)lists.jboss.org
>>>
https://lists.jboss.org/mailman/listinfo/infinispan-dev
>>
>> _______________________________________________
>> infinispan-dev mailing list
>> infinispan-dev(a)lists.jboss.org
>>
https://lists.jboss.org/mailman/listinfo/infinispan-dev
>
>
> --
> Radim Vansa <rvansa(a)redhat.com>
> JBoss Performance Team
>
> _______________________________________________
> infinispan-dev mailing list
> infinispan-dev(a)lists.jboss.org
>
https://lists.jboss.org/mailman/listinfo/infinispan-dev
_______________________________________________
infinispan-dev mailing list
infinispan-dev(a)lists.jboss.org
https://lists.jboss.org/mailman/listinfo/infinispan-dev