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 the size of the pool. Alternatively, we may consider using public pool for that. May I ask if there is an example use case which doesn’t work with public pool? For .NET, agree that we should follow the rules and APIs of the platform, so the behavior might slightly differ. Thanks, Stan > On 24 Mar 2021, at 09:52, Pavel Tupitsyn <ptupit...@apache.org> wrote: > > 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 <ptupit...@apache.org> >> wrote: >> >> Ready for review: >> https://github.com/apache/ignite/pull/8870 >> >> On Sun, Mar 21, 2021 at 8:09 PM Pavel Tupitsyn <ptupit...@apache.org> >> 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 barely observable on real-world >>> workloads. >>> >>> On Thu, Mar 18, 2021 at 12:35 PM Pavel Tupitsyn <ptupit...@apache.org> >>> wrote: >>> >>>> 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 < >>>> raymond_wil...@trimble.com> wrote: >>>> >>>>> 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: Deadlocking is an issue as is starvation. In >>>>> some >>>>> ways starvation is more insidious because by the time things stop >>>>> working >>>>> the cause and effect distance may be large. >>>>> >>>>> I appreciate the documentation does make statements about not performing >>>>> cache operations in a continuation due to deadlock possibilities, but >>>>> that >>>>> statement does not reveal why this is an issue. It is less a case of a >>>>> async cache operation being followed by some other cache operation (an >>>>> immediate issue), and more a general case of the continuation of >>>>> application logic using a striped pool thread in a way that might mean >>>>> that >>>>> thread is never given back - it's now just a piece of the application >>>>> infrastructure until some other application activity schedules away from >>>>> that thread (eg: by ContinueWith or some other async operation in the >>>>> application code that releases the thread). To be fair, beyond >>>>> structures >>>>> like ContinueWith(), it is not obvious how that continuation thread >>>>> should >>>>> be handed back. This will be the same behaviour for dedicated >>>>> continuation >>>>> pools, but with far less risk in the absence of ContinueWith() >>>>> constructs. >>>>> >>>>> In the .Net world this is becoming more of an issue as fewer .Net use >>>>> cases >>>>> outside of UI bother with synchronization contexts by default. >>>>> >>>>> Raymond. >>>>> >>>>> >>>>> On Thu, Mar 18, 2021 at 9:56 AM Valentin Kulichenko < >>>>> valentin.kuliche...@gmail.com> wrote: >>>>> >>>>>> 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 prior to >>>>> listener >>>>>> invocation (this is actually quite possible in the unit test >>>>> environment >>>>>> causing the test to pass). Finally, I'm pretty sure there are many >>>>> cases >>>>>> when a deadlock does not occur right away, but instead it will reveal >>>>>> itself under high load due to thread starvation. The latter type of >>>>> issues >>>>>> is the most dangerous because they are often reproduced only in >>>>> production. >>>>>> Finally, there are performance considerations as well - cache >>>>> operations >>>>>> and listeners share the same fixed-sized pool which can have negative >>>>>> effects. >>>>>> >>>>>> I'm OK with the change. Although, it might be better to introduce a >>>>> new >>>>>> fixed-sized pool instead of ForkJoinPool for listeners, simply >>>>> because this >>>>>> is the approach taken throughout the project. But this is up to a >>>>> debate. >>>>>> >>>>>> -Val >>>>>> >>>>>> On Wed, Mar 17, 2021 at 11:31 AM Denis Garus <garus....@gmail.com> >>>>> wrote: >>>>>> >>>>>>> Pavel, >>>>>>> I tried this: >>>>>>> >>>>>>> @Test >>>>>>> public void test() throws Exception { >>>>>>> IgniteCache<Integer, String> 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 make listener that leads to deadlock, but >>>>>>> the example in the IEP does not reflect this. >>>>>>> >>>>>>> >>>>>>> >>>>>>> ср, 17 мар. 2021 г. в 17:36, Вячеслав Коптилин < >>>>> slava.kopti...@gmail.com >>>>>>> : >>>>>>> >>>>>>>> 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 >>>>>>>> trivial method like putAsync >>>>>>>> That is sad... However, I don't think that this is a strong >>>>> argument >>>>>>> here. >>>>>>>> >>>>>>>> This is just my opinion. Let's see what other community members >>>>> have to >>>>>>>> say. >>>>>>>> >>>>>>>> Thanks, >>>>>>>> S. >>>>>>>> >>>>>>>> >>>>>>>> ср, 17 мар. 2021 г. в 17:01, Pavel Tupitsyn <ptupit...@apache.org >>>>>> : >>>>>>>> >>>>>>>>>> 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 you know about that, it is difficult to decide what >>>>> can run >>>>>>> on >>>>>>>>> the striped pool, >>>>>>>>> and what can't. It is too easy to forget. >>>>>>>>> And most people don't know, even among Ignite developers. >>>>>>>>> >>>>>>>>> Documentation won't help - no one is going to read the Javadoc >>>>> for a >>>>>>>>> trivial method like putAsync. >>>>>>>>> >>>>>>>>> >>>>>>>>> So I propose to have a safe default. >>>>>>>>> Then document the performance tuning opportunity on [1]. >>>>>>>>> >>>>>>>>> Think about how many users abandon a product because it >>>>> mysteriously >>>>>>>>> crashes and hangs. >>>>>>>>> >>>>>>>>> [1] >>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>> https://ignite.apache.org/docs/latest/perf-and-troubleshooting/general-perf-tips >>>>>>>>> >>>>>>>>> >>>>>>>>> On Wed, Mar 17, 2021 at 4:21 PM Вячеслав Коптилин < >>>>>>>>> slava.kopti...@gmail.com> >>>>>>>>> wrote: >>>>>>>>> >>>>>>>>>> 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 deadlocks. >>>>>>>>>> cache.replace(1, 2); >>>>>>>>>> }, ForkJoinPool.commonPool()); >>>>>>>>>> >>>>>>>>>> Of course, it does not mean that this fact should not be >>>>> properly >>>>>>>>>> documented. >>>>>>>>>> Perhaps, I am missing something. >>>>>>>>>> >>>>>>>>>> Thanks, >>>>>>>>>> S. >>>>>>>>>> >>>>>>>>>> ср, 17 мар. 2021 г. в 16:01, Pavel Tupitsyn < >>>>> ptupit...@apache.org >>>>>>> : >>>>>>>>>> >>>>>>>>>>> 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 performance >>>>> in >>>>>> any >>>>>>>>> case. >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> On Wed, Mar 17, 2021 at 3:34 PM Вячеслав Коптилин < >>>>>>>>>>> slava.kopti...@gmail.com> >>>>>>>>>>> wrote: >>>>>>>>>>> >>>>>>>>>>>> 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 exec Executor to run listener. Cannot be >>>>> {@code >>>>>>>> null}. >>>>>>>>>>>> */ >>>>>>>>>>>> public void listenAsync(IgniteInClosure<? super >>>>>>>> IgniteFuture<V>> >>>>>>>>>>> lsnr, >>>>>>>>>>>> Executor exec); >>>>>>>>>>>> >>>>>>>>>>>> Thanks, >>>>>>>>>>>> S. >>>>>>>>>>>> >>>>>>>>>>>> ср, 17 мар. 2021 г. в 15:25, Вячеслав Коптилин < >>>>>>>>>> slava.kopti...@gmail.com >>>>>>>>>>>> : >>>>>>>>>>>> >>>>>>>>>>>>> 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 in thread that >>>>> completes >>>>>> this >>>>>>>>> future >>>>>>>>>>> or >>>>>>>>>>>>> (if future already >>>>>>>>>>>>> * completed) immediately in current thread. >>>>>>>>>>>>> * >>>>>>>>>>>>> * @param lsnr Listener closure to register. Cannot >>>>> be >>>>>>> {@code >>>>>>>>>>> null}. >>>>>>>>>>>>> */ >>>>>>>>>>>>> public void listen(IgniteInClosure<? super >>>>>> IgniteFuture<V>> >>>>>>>>>> lsnr); >>>>>>>>>>>>> >>>>>>>>>>>>> In your pull request, the listener is always called >>>>> from >>>>>> a >>>>>>>>>>> specified >>>>>>>>>>>>> thread pool (which is fork-join by default) >>>>>>>>>>>>> even though the future is already completed at the >>>>> moment >>>>>>> the >>>>>>>>>>> listen >>>>>>>>>>>>> method is called. >>>>>>>>>>>>> In my opinion, this can lead to significant >>>>> overhead - >>>>>>>>> submission >>>>>>>>>>>>> requires acquiring a lock and notifying a pool thread. >>>>>>>>>>>>> >>>>>>>>>>>>> It seems to me, that we should not change the >>>>> current >>>>>>>> behavior. >>>>>>>>>>>>> However, thread pool executor can be added as an >>>>> optional >>>>>>>> parameter >>>>>>>>>> of >>>>>>>>>>>>> listen() method as follows: >>>>>>>>>>>>> >>>>>>>>>>>>> public void listen(IgniteInClosure<? super >>>>>>>> IgniteFuture<V>> >>>>>>>>>>> lsnr, >>>>>>>>>>>>> Executor exec); >>>>>>>>>>>>> >>>>>>>>>>>>> Thanks, >>>>>>>>>>>>> S. >>>>>>>>>>>>> >>>>>>>>>>>>> пн, 15 мар. 2021 г. в 19:24, Pavel Tupitsyn < >>>>>>>> ptupit...@apache.org >>>>>>>>>> : >>>>>>>>>>>>> >>>>>>>>>>>>>> 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 >>>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>> >>>>> >>>>> -- >>>>> <http://www.trimble.com/> >>>>> Raymond Wilson >>>>> Solution Architect, Civil Construction Software Systems (CCSS) >>>>> 11 Birmingham Drive | Christchurch, New Zealand >>>>> raymond_wil...@trimble.com >>>>> >>>>> < >>>>> https://worksos.trimble.com/?utm_source=Trimble&utm_medium=emailsign&utm_campaign=Launch >>>>>> >>>>> >>>>
