Re: IEP-70: Async Continuation Executor

I'll keep the common pool then, thank you. Public pool is the "work-horse of the Compute Grid" [1], so it does not seem to fit here at all. [1] https://ignite.apache.org/docs/latest/perf-and-troubleshooting/thread-pools-tuning#public-pool On Thu, Apr 15, 2021 at 2:18 PM Stanislav Lukyanov

Re: IEP-70: Async Continuation Executor

Giving this another thought - I'm kinda torn on this myself now, as I like you argument that a chain of multiple (GG and not GG) continuations should execute in the same pool. This would probably be easier to understand for a beginning or intermediate Ignite user, which is the majority. Anyway,

Re: IEP-70: Async Continuation Executor

Hi Pavel, I'd prefer public pool. Thanks, Stan > On 12 Apr 2021, at 20:17, Pavel Tupitsyn wrote: > > Stan, > > Sorry for the late reply (had a vacation). > >> In my ideal world, the users don't configure thread pools, they just have > safe default behavior (async execution) >> and a way to

Re: IEP-70: Async Continuation Executor

Stan, Sorry for the late reply (had a vacation). > In my ideal world, the users don't configure thread pools, they just have safe default behavior (async execution) > and a way to make it fast for one particular function (with annotation or anything else). I've added

Re: IEP-70: Async Continuation Executor

But what if I need to have exactly one callback synchronous, and all other can be asynchronous? I would separate two cases: an existing user who wants their old behavior back, and a new user that wants to fine tune their app. The existing user needs a global "make it all synchronous" switch.

Re: IEP-70: Async Continuation Executor

Stan, Unfortunately, annotations have a few drawbacks: * Can't configure it globally ("I already use sync callbacks, give me back the old behavior in one line") * Can't configure in Spring * Useless in C++ & .NET * You can already specify executor in IgniteFuture#listenAsync, so there seems to be

Re: IEP-70: Async Continuation Executor

Thought about this some more. I agree that we need to be able to switch to synchronous listeners when it's critical for performance. However, I don't like to introduce an Executor property for that. In fact, the only value we really expect the user to set in that property is Runnable::run -

Re: IEP-70: Async Continuation Executor

Stan, I'm ok with using public pool by default, but we need a way to restore the old behavior, do you agree? I think we should keep the new IgniteConfiguration property. On Fri, Mar 26, 2021 at 2:12 PM Alexei Scherbakov < alexey.scherbak...@gmail.com> wrote: > Pavel, > > Dedicated pool looks

Re: IEP-70: Async Continuation Executor

Pavel, Dedicated pool looks safer and more manageable to me. Make sure the threads in the pool are lazily started and stopped if not used for some time. Because I have no more real arguments against the change, I suggest to proceed with this approach. чт, 25 мар. 2021 г. в 22:16, Pavel Tupitsyn

Re: IEP-70: Async Continuation Executor

Alexei, > we already have ways to control a listener's behavior No, we don't have a way to fix current broken and dangerous behavior globally. You should not expect the user to fix every async call manually. > commonPool can alter existing deployments in unpredictable ways, > if commonPool is

Re: IEP-70: Async Continuation Executor

> I don't agree that the code isn't related to Ignite - it is something that the user does via Ignite API This is a misconception. When you write general-purpose async code, it looks like this: myClass.fooAsync() .chain(igniteCache.putAsync) .chain(myClass.barAsync) .chain(...) And so on, you

Re: IEP-70: Async Continuation Executor

Pavel, The change still looks a bit risky to me, because the default executor is set to commonPool and can alter existing deployments in unpredictable ways, if commonPool is heavily used for other purposes. Runnable::run usage is not obvious as well and should be properly documented as a way to

Re: IEP-70: Async Continuation Executor

I don't agree that the code isn't related to Ignite - it is something that the user does via Ignite API, so they would be right to expect Ignite to handle the invocations. One more thing I'd like to mention - monitoring. I don't think commonPool exposes anything to JMX by default, and a custom

Re: IEP-70: Async Continuation Executor

Alexei, > Sometimes it's more desirable to execute the listener in the same thread > It's up to the user to decide. Yes, we give users a choice to configure the executor as Runnable::run and use the same thread if needed. However, it should not be the default behavior as explained above (bad

Re: IEP-70: Async Continuation Executor

Pavel, While I understand the issue and overall agree with you, I'm against the execution of listeners in separate thread pool by default. Sometimes it's more desirable to execute the listener in the same thread, for example if it's some lightweight closure. It's up to the user to decide. I

Re: IEP-70: Async Continuation Executor

Stan, We have thread pools dedicated for specific purposes, like cache (striped), compute (pub), query, etc As I understand it, the reason here is to limit the number of threads dedicated to a given subsystem. For example, Compute may be overloaded with work, but Cache and Discovery will keep

Re: IEP-70: Async Continuation Executor

Pavel, This is a great work, fully agree with the overall idea and approach. However, I have some reservations about the API. We sure do have a lot of async stuff in the system, and I would suggest to stick to the usual design - create a separate thread pool, add a single property to control

Re: IEP-70: Async Continuation Executor

Igniters, since there are no more comments and/or review feedback, I'm going to merge the changes by the end of the week. On Mon, Mar 22, 2021 at 10:37 PM Pavel Tupitsyn wrote: > Ready for review: > https://github.com/apache/ignite/pull/8870 > > On Sun, Mar 21, 2021 at 8:09 PM Pavel Tupitsyn >

Re: IEP-70: Async Continuation Executor

Ready for review: https://github.com/apache/ignite/pull/8870 On Sun, Mar 21, 2021 at 8:09 PM Pavel Tupitsyn wrote: > Simple benchmark added - see JmhCacheAsyncListenBenchmark in the PR. > There is a 6-8% drop (1 client, 2 servers, 1 machine, int key/val). > I expect this difference to become

Re: IEP-70: Async Continuation Executor

Simple benchmark added - see JmhCacheAsyncListenBenchmark in the PR. There is a 6-8% drop (1 client, 2 servers, 1 machine, int key/val). I expect this difference to become barely observable on real-world workloads. On Thu, Mar 18, 2021 at 12:35 PM Pavel Tupitsyn wrote: > Denis, > > For a

Re: IEP-70: Async Continuation Executor

Denis, For a reproducer, please see CacheAsyncContinuationExecutorTest.java in the linked PoC [1] [1] https://github.com/apache/ignite/pull/8870/files#diff-c788c12013622093df07d8f628a6e8c5fb0c15007f0787f2d459dbb3e377fc5aR54 On Thu, Mar 18, 2021 at 1:58 AM Raymond Wilson wrote: > We

Re: IEP-70: Async Continuation Executor

We implemented the ContinueWith() suggestion from Pavel, and it works well so far in testing, though we do not have data to support if there is or is not a performance penalty in our use case.. To lend another vote to the 'Don't do continuations on the striped thread pool' line of thinking:

Re: IEP-70: Async Continuation Executor

Hi Denis, I think Pavel's main point is that behavior is unpredictable. For example, AFAIK, putAsync can be executed in the main thread instead of the striped pool thread if the operation is local. The listener can also be executed in the main thread - this happens if the future is completed

Re: IEP-70: Async Continuation Executor

Pavel, I tried this: @Test public void test() throws Exception { IgniteCache cache = startGrid().getOrCreateCache("test_cache"); cache.putAsync(1, "one").listen(f -> cache.replace(1, "two")); assertEquals("two", cache.get(1)); } and this test is green. I believe that an user can

Re: IEP-70: Async Continuation Executor

Hi Pavel, > Not a good excuse really. We have a usability problem, you have to admit it. Fair enough. I agree that this is a usability issue, but I have doubts that the proposed approach to overcome it is the best one. > Documentation won't help - no one is going to read the Javadoc for a

Re: IEP-70: Async Continuation Executor

> the user should use the right API Not a good excuse really. We have a usability problem, you have to admit it. "The brakes did not work on your car - too bad, you should have known that on Sundays only your left foot is allowed on the pedal" This particular use case is too intricate. Even when

Re: IEP-70: Async Continuation Executor

Hi Pavel, Well, I think that the user should use the right API instead of introducing uncontested overhead for everyone. For instance, the code that is provided by IEP can changed as follows: IgniteFuture fut = cache.putAsync(1, 1); fut.listenAync(f -> { // Executes on Striped pool and

Re: IEP-70: Async Continuation Executor

Slava, Your suggestion is to keep things as is and discard the IEP, right? > this can lead to significant overhead Yes, there is some overhead, but the cost of accidentally starving the striped pool is worse, not to mention the deadlocks. I believe that we should favor correctness over

Re: IEP-70: Async Continuation Executor

Well, the specified method already exists :) /** * Registers listener closure to be asynchronously notified whenever future completes. * Closure will be processed in specified executor. * * @param lsnr Listener closure to register. Cannot be {@code null}. * @param

Re: IEP-70: Async Continuation Executor

Pavel, I got it. Thank you. ср, 17 мар. 2021 г. в 13:11, Pavel Tupitsyn : > Denis, > > Yes, I've updated the Motivation section, it really was a bit vague - thank > you. > > > why you suggest using ForkJoinPool.commonPool as the default pool to run > listeners > > - It is recommended to be used

Re: IEP-70: Async Continuation Executor

Hello Pavel, I took a look at your IEP and pool request. I have the following concerns. First of all, this change breaks the contract of IgniteFuture#listen(lsnr) /** * Registers listener closure to be asynchronously notified whenever future completes. * Closure will be processed

Re: IEP-70: Async Continuation Executor

Denis, Yes, I've updated the Motivation section, it really was a bit vague - thank you. > why you suggest using ForkJoinPool.commonPool as the default pool to run listeners - It is recommended to be used by default [1] - There are no alternatives? [1]

Re: IEP-70: Async Continuation Executor

Pavel, thank you for your answer. Sorry, the previous version of IEP motivation highlighted what could be wrong with user listeners, so I thought that is the main problem. I'm wondering why you suggest using ForkJoinPool.commonPool as the default pool to run listeners? ср, 17 мар. 2021 г. в

Re: IEP-70: Async Continuation Executor

My initial confusion was due to I did not know the TaskCompletionSource.SetResult() internally handles SynchronizationContext captured before the async operation. I thought we would have to do it in Ignite. Now I know that and have no problems with the IEP. ср, 17 мар. 2021 г. в 11:18, Pavel

Re: IEP-70: Async Continuation Executor

Denis, > we don't try to solve the problem mentioned in the IEP The problem: user code executes on striped pool (which causes issues) The solution: don't execute user code on striped pool I believe this solves the problem and removes any and all restrictions for async listeners/continuations.

Re: IEP-70: Async Continuation Executor

Hello! As I understood, we don't try to solve the problem mentioned in the IEP: there is unexpected behavior, users should carefully read the docs to know about this, and so on. A thread that executes an incorrect listener hung in any case, and the suggested solution is to change the pool which

Re: IEP-70: Async Continuation Executor

Pavel, So what you are saying is submitting a continuation to .NET Thread Pool already respects current SynchronizationContext and ConfigureAwait. In this case the IEP looks complete (for some reason I thought that we should take care about the SynchronizationContext inside the Ignite.NET

Re: IEP-70: Async Continuation Executor

Raymond, > If it is present in memory, then a Sync operation will > be more performant. Is it possible to do a two step operation like this > 'Cache.GetIfInMemory() ?? await Cache.GetAsync()' Ignite already does that - if the key is present locally (primary, backup, near cache, platform cache),

Re: IEP-70: Async Continuation Executor

Alexey, I'm not sure your understanding is entirely correct. The proposal won't break or change anything for UI apps and other apps with SynchronizationContext. The code in Button1_Click works now and will work after the proposal: continuations are routed to SynchronizationContext automatically

Re: IEP-70: Async Continuation Executor

Raymond, The article you referenced is great! It seems to me the article rather proved what I suggested: an async operation implementation: - Uses the async operation thread (Ignite's thread) if ConfigureAwait is false (by default

Re: IEP-70: Async Continuation Executor

There is a (long) discussion here ( https://devblogs.microsoft.com/dotnet/configureawait-faq/) regarding use of ConfigureAwait(). Putting edge cases aside, there is a general use case for .ConfigureAwait(true) in application UI contexts to ensure the continuation joins a UI thread (for example),

Re: IEP-70: Async Continuation Executor

Pavel, My understanding might be wrong but I think the best practice (or even strongly recommended way) to implement async methods in .NET is to execute continuation on the caller's thread if ConfigureAwait(false) was not specified. Pseudo-code might look like: async Task PutAsync(K k, V v) {

Re: IEP-70: Async Continuation Executor

Pavel, I like the proposal, +1 from me Best Regards, Igor On Tue, Mar 16, 2021 at 6:49 PM Pavel Tupitsyn wrote: > Alexey, > > .NET thick API delegates to Java directly. > > When you do ICache.PutAsync(): > * Future is created on Java side, .listen() is called > * TaskCompletionSource is

Re: IEP-70: Async Continuation Executor

Alexey, .NET thick API delegates to Java directly. When you do ICache.PutAsync(): * Future is created on Java side, .listen() is called * TaskCompletionSource is created on .NET side, its Task is returned to the user * Operation completes, Future listener is called on the Java side * Listener

Re: IEP-70: Async Continuation Executor

Hi Pavel, Extending Java async API with additional Executor parameters looks OK to me. It is not clear from the IEP how you are going to do that for .NET async API. My understanding is in .NET we do not add any Executors. Instead, the Ignite Async API should use (SynchronizationContext.Current

IEP-70: Async Continuation Executor

Igniters, Please review the IEP [1] and let me know your thoughts. [1] https://cwiki.apache.org/confluence/display/IGNITE/IEP-70%3A+Async+Continuation+Executor