[infinispan-dev] Sequential interceptors API

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[infinispan-dev] Sequential interceptors API

Radim Vansa
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 <[hidden email]>
JBoss Performance Team

_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [infinispan-dev] Sequential interceptors API

Galder Zamarreño
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 <[hidden email]> 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 <[hidden email]>
> JBoss Performance Team
>
> _______________________________________________
> infinispan-dev mailing list
> [hidden email]
> https://lists.jboss.org/mailman/listinfo/infinispan-dev


_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [infinispan-dev] Sequential interceptors API

Radim Vansa
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/org/infinispan/interceptors/impl/PrefetchInvalidationInterceptor.java

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 <[hidden email]> 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 <[hidden email]>
>> JBoss Performance Team
>>
>> _______________________________________________
>> infinispan-dev mailing list
>> [hidden email]
>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>
> _______________________________________________
> infinispan-dev mailing list
> [hidden email]
> https://lists.jboss.org/mailman/listinfo/infinispan-dev


--
Radim Vansa <[hidden email]>
JBoss Performance Team

_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [infinispan-dev] Sequential interceptors API

Galder Zamarreño
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/AsyncResponseHandler
[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 <[hidden email]> 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/org/infinispan/interceptors/impl/PrefetchInvalidationInterceptor.java
>
> 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 <[hidden email]> 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 <[hidden email]>
>>> JBoss Performance Team
>>>
>>> _______________________________________________
>>> infinispan-dev mailing list
>>> [hidden email]
>>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>>
>> _______________________________________________
>> infinispan-dev mailing list
>> [hidden email]
>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>
>
> --
> Radim Vansa <[hidden email]>
> JBoss Performance Team
>
> _______________________________________________
> infinispan-dev mailing list
> [hidden email]
> https://lists.jboss.org/mailman/listinfo/infinispan-dev


_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [infinispan-dev] Sequential interceptors API

Radim Vansa
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/AsyncResponseHandler
> [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 <[hidden email]> 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/org/infinispan/interceptors/impl/PrefetchInvalidationInterceptor.java
>>
>> 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 <[hidden email]> 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 <[hidden email]>
>>>> JBoss Performance Team
>>>>
>>>> _______________________________________________
>>>> infinispan-dev mailing list
>>>> [hidden email]
>>>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>>> _______________________________________________
>>> infinispan-dev mailing list
>>> [hidden email]
>>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>>
>> --
>> Radim Vansa <[hidden email]>
>> JBoss Performance Team
>>
>> _______________________________________________
>> infinispan-dev mailing list
>> [hidden email]
>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>
> _______________________________________________
> infinispan-dev mailing list
> [hidden email]
> https://lists.jboss.org/mailman/listinfo/infinispan-dev


--
Radim Vansa <[hidden email]>
JBoss Performance Team

_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [infinispan-dev] Sequential interceptors API

Dan Berindei
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:ISPN-5467_CompletableFuture-like_API?expand=1


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. 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 <[hidden email]> 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/AsyncResponseHandler
>> [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 <[hidden email]> 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/org/infinispan/interceptors/impl/PrefetchInvalidationInterceptor.java
>>>
>>> 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 <[hidden email]> 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 <[hidden email]>
>>>>> JBoss Performance Team
>>>>>
>>>>> _______________________________________________
>>>>> infinispan-dev mailing list
>>>>> [hidden email]
>>>>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>>>> _______________________________________________
>>>> infinispan-dev mailing list
>>>> [hidden email]
>>>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>>>
>>> --
>>> Radim Vansa <[hidden email]>
>>> JBoss Performance Team
>>>
>>> _______________________________________________
>>> infinispan-dev mailing list
>>> [hidden email]
>>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>>
>> _______________________________________________
>> infinispan-dev mailing list
>> [hidden email]
>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>
>
> --
> Radim Vansa <[hidden email]>
> JBoss Performance Team
>
> _______________________________________________
> infinispan-dev mailing list
> [hidden email]
> https://lists.jboss.org/mailman/listinfo/infinispan-dev

_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [infinispan-dev] Sequential interceptors API

Galder Zamarreño

--
Galder Zamarreño
Infinispan, Red Hat

> On 3 Aug 2016, at 13:54, Dan Berindei <[hidden email]> 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:ISPN-5467_CompletableFuture-like_API?expand=1
>
>
> 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 <[hidden email]> 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/AsyncResponseHandler
>>> [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 <[hidden email]> 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/org/infinispan/interceptors/impl/PrefetchInvalidationInterceptor.java
>>>>
>>>> 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 <[hidden email]> 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 <[hidden email]>
>>>>>> JBoss Performance Team
>>>>>>
>>>>>> _______________________________________________
>>>>>> infinispan-dev mailing list
>>>>>> [hidden email]
>>>>>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>>>>> _______________________________________________
>>>>> infinispan-dev mailing list
>>>>> [hidden email]
>>>>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>>>>
>>>> --
>>>> Radim Vansa <[hidden email]>
>>>> JBoss Performance Team
>>>>
>>>> _______________________________________________
>>>> infinispan-dev mailing list
>>>> [hidden email]
>>>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>>>
>>> _______________________________________________
>>> infinispan-dev mailing list
>>> [hidden email]
>>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>>
>>
>> --
>> Radim Vansa <[hidden email]>
>> JBoss Performance Team
>>
>> _______________________________________________
>> infinispan-dev mailing list
>> [hidden email]
>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>
> _______________________________________________
> infinispan-dev mailing list
> [hidden email]
> https://lists.jboss.org/mailman/listinfo/infinispan-dev


_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Loading...