ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Raymond Wilson <raymond_wil...@trimble.com>
Subject Re: IEP-70: Async Continuation Executor
Date Wed, 17 Mar 2021 22:58:12 GMT
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.kulichenko@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.d.g@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.koptilin@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 <ptupitsyn@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.koptilin@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 <ptupitsyn@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.koptilin@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.koptilin@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
<
> > > ptupitsyn@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_wilson@trimble.com

<https://worksos.trimble.com/?utm_source=Trimble&utm_medium=emailsign&utm_campaign=Launch>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message