GridFutureAdapter: deprecate startTime(), duration() and endTime()

[yzhdanov]

Guys,

We have startTime(), duration() and endTime() methods which have several
usages each along internals of the project, but to support these methods we
have 2 long fields in GridFutureAdapter which gives us 16 bytes.

Other fields - res (reference 8 bytes at max), ignoreInterrupts (boolean 1
byte) and resFlag (byte 1 byte) = 10 bytes

I did quick tests and I see that removing these fields (i.e. making each
future 16 bytes thinner) can give us 5-6% in performance results.

I want to deprecate  startTime(), duration() and endTime() and therefore
deprecate corresponding methods in IgniteFuture.

Thoughts?

--Yakov

[Vladimir Ozerov]

+1

BTW, corresponding ticket already exists. You can find it under umbrella
ticket IGNITE-2232
14 янв. 2016 г. 18:40 пользователь "Yakov Zhdanov" <[hidden email]>
написал:

> Guys,
>
> We have startTime(), duration() and endTime() methods which have several
> usages each along internals of the project, but to support these methods we
> have 2 long fields in GridFutureAdapter which gives us 16 bytes.
>
> Other fields - res (reference 8 bytes at max), ignoreInterrupts (boolean 1
> byte) and resFlag (byte 1 byte) = 10 bytes
>
> I did quick tests and I see that removing these fields (i.e. making each
> future 16 bytes thinner) can give us 5-6% in performance results.
>
> I want to deprecate  startTime(), duration() and endTime() and therefore
> deprecate corresponding methods in IgniteFuture.
>
> Thoughts?
>
> --Yakov
>

[dsetrakyan]

Really hard to believe that removing 16 bytes in futures gives 5% of
performance. Yakov, are you certain about this?

If this turns out to be true, let’s see if we can slim down the futures
used internally, without breaking the public API.

D.

On Thu, Jan 14, 2016 at 8:36 AM, Vladimir Ozerov <[hidden email]>
wrote:

> +1
>
> BTW, corresponding ticket already exists. You can find it under umbrella
> ticket IGNITE-2232
> 14 янв. 2016 г. 18:40 пользователь "Yakov Zhdanov" <[hidden email]>
> написал:
>
> > Guys,
> >
> > We have startTime(), duration() and endTime() methods which have several
> > usages each along internals of the project, but to support these methods
> we
> > have 2 long fields in GridFutureAdapter which gives us 16 bytes.
> >
> > Other fields - res (reference 8 bytes at max), ignoreInterrupts (boolean
> 1
> > byte) and resFlag (byte 1 byte) = 10 bytes
> >
> > I did quick tests and I see that removing these fields (i.e. making each
> > future 16 bytes thinner) can give us 5-6% in performance results.
> >
> > I want to deprecate  startTime(), duration() and endTime() and therefore
> > deprecate corresponding methods in IgniteFuture.
> >
> > Thoughts?
> >
> > --Yakov
> >
>

[yzhdanov]

All of optimizations applied to futures so far were extremely effective.
Ignite can create different number of futures per operation depending on
context. Multiple this by ops/sec.. This is probably one of the most
intensively instantiated (and therefore GCed) object. Internal futures is
very sensitive part of the system and should be 100% effective.

As far as public API, anyone
uses org.apache.ignite.lang.IgniteFuture#startTime
or org.apache.ignite.lang.IgniteFuture#duration?

--Yakov

2016-01-15 1:20 GMT+03:00 Dmitriy Setrakyan <[hidden email]>:

> Really hard to believe that removing 16 bytes in futures gives 5% of
> performance. Yakov, are you certain about this?
>
> If this turns out to be true, let’s see if we can slim down the futures
> used internally, without breaking the public API.
>
> D.
>
> On Thu, Jan 14, 2016 at 8:36 AM, Vladimir Ozerov <[hidden email]>
> wrote:
>
> > +1
> >
> > BTW, corresponding ticket already exists. You can find it under umbrella
> > ticket IGNITE-2232
> > 14 янв. 2016 г. 18:40 пользователь "Yakov Zhdanov" <[hidden email]>
> > написал:
> >
> > > Guys,
> > >
> > > We have startTime(), duration() and endTime() methods which have
> several
> > > usages each along internals of the project, but to support these
> methods
> > we
> > > have 2 long fields in GridFutureAdapter which gives us 16 bytes.
> > >
> > > Other fields - res (reference 8 bytes at max), ignoreInterrupts
> (boolean
> > 1
> > > byte) and resFlag (byte 1 byte) = 10 bytes
> > >
> > > I did quick tests and I see that removing these fields (i.e. making
> each
> > > future 16 bytes thinner) can give us 5-6% in performance results.
> > >
> > > I want to deprecate  startTime(), duration() and endTime() and
> therefore
> > > deprecate corresponding methods in IgniteFuture.
> > >
> > > Thoughts?
> > >
> > > --Yakov
> > >
> >
>

[Artem Shutak]

If it really can give as 5% of performance we have to do it.

If anyone uses this methods we can support it by user request (if user
explicitly asked about) and it will work slower in that case. For example,
we can add *IgniteCache.withAsync(boolean useFutureWithTimer)* that will
give to user an async cache with futures that support startTime(),
duration() and endTime methods as well.

-- Artem --

On Fri, Jan 15, 2016 at 3:10 PM, Yakov Zhdanov <[hidden email]> wrote:

> All of optimizations applied to futures so far were extremely effective.
> Ignite can create different number of futures per operation depending on
> context. Multiple this by ops/sec.. This is probably one of the most
> intensively instantiated (and therefore GCed) object. Internal futures is
> very sensitive part of the system and should be 100% effective.
>
> As far as public API, anyone
> uses org.apache.ignite.lang.IgniteFuture#startTime
> or org.apache.ignite.lang.IgniteFuture#duration?
>
> --Yakov
>
> 2016-01-15 1:20 GMT+03:00 Dmitriy Setrakyan <[hidden email]>:
>
> > Really hard to believe that removing 16 bytes in futures gives 5% of
> > performance. Yakov, are you certain about this?
> >
> > If this turns out to be true, let’s see if we can slim down the futures
> > used internally, without breaking the public API.
> >
> > D.
> >
> > On Thu, Jan 14, 2016 at 8:36 AM, Vladimir Ozerov <[hidden email]>
> > wrote:
> >
> > > +1
> > >
> > > BTW, corresponding ticket already exists. You can find it under
> umbrella
> > > ticket IGNITE-2232
> > > 14 янв. 2016 г. 18:40 пользователь "Yakov Zhdanov" <
> [hidden email]>
> > > написал:
> > >
> > > > Guys,
> > > >
> > > > We have startTime(), duration() and endTime() methods which have
> > several
> > > > usages each along internals of the project, but to support these
> > methods
> > > we
> > > > have 2 long fields in GridFutureAdapter which gives us 16 bytes.
> > > >
> > > > Other fields - res (reference 8 bytes at max), ignoreInterrupts
> > (boolean
> > > 1
> > > > byte) and resFlag (byte 1 byte) = 10 bytes
> > > >
> > > > I did quick tests and I see that removing these fields (i.e. making
> > each
> > > > future 16 bytes thinner) can give us 5-6% in performance results.
> > > >
> > > > I want to deprecate  startTime(), duration() and endTime() and
> > therefore
> > > > deprecate corresponding methods in IgniteFuture.
> > > >
> > > > Thoughts?
> > > >
> > > > --Yakov
> > > >
> > >
> >
>

[Vladimir Ozerov]

I also hardly believe that such change can give us such big immediate
performance benefit. Profiling shows that in our standard benchmarks
futures doesn't generate so much garbage to get ~5% from field removal. I
would rather re-check if benchmark is valid.

On the other hand, lots of our utility classes like GridFunc, GridUtils,
futures, etc., are overly complex and inefficient. As they are used almost
everywhere, if we see something what can be easily optimized, we should do
that right away, even if the benefit cannot be observed in benchmarks. This
will give us confidence that our fundamental abstractions are good enough,
so we can focus on business logic and algorithms, rather than on supporting
code.

So I think that regardless of the benchmark numbers, we should invest
efforts in changes like this.

On Fri, Jan 15, 2016 at 6:15 PM, Artem Shutak <[hidden email]> wrote:

> If it really can give as 5% of performance we have to do it.
>
> If anyone uses this methods we can support it by user request (if user
> explicitly asked about) and it will work slower in that case. For example,
> we can add *IgniteCache.withAsync(boolean useFutureWithTimer)* that will
> give to user an async cache with futures that support startTime(),
> duration() and endTime methods as well.
>
> -- Artem --
>
> On Fri, Jan 15, 2016 at 3:10 PM, Yakov Zhdanov <[hidden email]>
> wrote:
>
> > All of optimizations applied to futures so far were extremely effective.
> > Ignite can create different number of futures per operation depending on
> > context. Multiple this by ops/sec.. This is probably one of the most
> > intensively instantiated (and therefore GCed) object. Internal futures is
> > very sensitive part of the system and should be 100% effective.
> >
> > As far as public API, anyone
> > uses org.apache.ignite.lang.IgniteFuture#startTime
> > or org.apache.ignite.lang.IgniteFuture#duration?
> >
> > --Yakov
> >
> > 2016-01-15 1:20 GMT+03:00 Dmitriy Setrakyan <[hidden email]>:
> >
> > > Really hard to believe that removing 16 bytes in futures gives 5% of
> > > performance. Yakov, are you certain about this?
> > >
> > > If this turns out to be true, let’s see if we can slim down the futures
> > > used internally, without breaking the public API.
> > >
> > > D.
> > >
> > > On Thu, Jan 14, 2016 at 8:36 AM, Vladimir Ozerov <[hidden email]
> >
> > > wrote:
> > >
> > > > +1
> > > >
> > > > BTW, corresponding ticket already exists. You can find it under
> > umbrella
> > > > ticket IGNITE-2232
> > > > 14 янв. 2016 г. 18:40 пользователь "Yakov Zhdanov" <
> > [hidden email]>
> > > > написал:
> > > >
> > > > > Guys,
> > > > >
> > > > > We have startTime(), duration() and endTime() methods which have
> > > several
> > > > > usages each along internals of the project, but to support these
> > > methods
> > > > we
> > > > > have 2 long fields in GridFutureAdapter which gives us 16 bytes.
> > > > >
> > > > > Other fields - res (reference 8 bytes at max), ignoreInterrupts
> > > (boolean
> > > > 1
> > > > > byte) and resFlag (byte 1 byte) = 10 bytes
> > > > >
> > > > > I did quick tests and I see that removing these fields (i.e. making
> > > each
> > > > > future 16 bytes thinner) can give us 5-6% in performance results.
> > > > >
> > > > > I want to deprecate  startTime(), duration() and endTime() and
> > > therefore
> > > > > deprecate corresponding methods in IgniteFuture.
> > > > >
> > > > > Thoughts?
> > > > >
> > > > > --Yakov
> > > > >
> > > >
> > >
> >
>

[dsetrakyan]

Yakov,

Any chance you can rerun the benchmarks to confirm your findings?

Would also be nice if someone else could run them as well. Is there a
branch that we can check out?

D.

On Fri, Jan 15, 2016 at 12:17 PM, Vladimir Ozerov <[hidden email]>
wrote:

> I also hardly believe that such change can give us such big immediate
> performance benefit. Profiling shows that in our standard benchmarks
> futures doesn't generate so much garbage to get ~5% from field removal. I
> would rather re-check if benchmark is valid.
>
> On the other hand, lots of our utility classes like GridFunc, GridUtils,
> futures, etc., are overly complex and inefficient. As they are used almost
> everywhere, if we see something what can be easily optimized, we should do
> that right away, even if the benefit cannot be observed in benchmarks. This
> will give us confidence that our fundamental abstractions are good enough,
> so we can focus on business logic and algorithms, rather than on supporting
> code.
>
> So I think that regardless of the benchmark numbers, we should invest
> efforts in changes like this.
>
> On Fri, Jan 15, 2016 at 6:15 PM, Artem Shutak <[hidden email]>
> wrote:
>
> > If it really can give as 5% of performance we have to do it.
> >
> > If anyone uses this methods we can support it by user request (if user
> > explicitly asked about) and it will work slower in that case. For
> example,
> > we can add *IgniteCache.withAsync(boolean useFutureWithTimer)* that will
> > give to user an async cache with futures that support startTime(),
> > duration() and endTime methods as well.
> >
> > -- Artem --
> >
> > On Fri, Jan 15, 2016 at 3:10 PM, Yakov Zhdanov <[hidden email]>
> > wrote:
> >
> > > All of optimizations applied to futures so far were extremely
> effective.
> > > Ignite can create different number of futures per operation depending
> on
> > > context. Multiple this by ops/sec.. This is probably one of the most
> > > intensively instantiated (and therefore GCed) object. Internal futures
> is
> > > very sensitive part of the system and should be 100% effective.
> > >
> > > As far as public API, anyone
> > > uses org.apache.ignite.lang.IgniteFuture#startTime
> > > or org.apache.ignite.lang.IgniteFuture#duration?
> > >
> > > --Yakov
> > >
> > > 2016-01-15 1:20 GMT+03:00 Dmitriy Setrakyan <[hidden email]>:
> > >
> > > > Really hard to believe that removing 16 bytes in futures gives 5% of
> > > > performance. Yakov, are you certain about this?
> > > >
> > > > If this turns out to be true, let’s see if we can slim down the
> futures
> > > > used internally, without breaking the public API.
> > > >
> > > > D.
> > > >
> > > > On Thu, Jan 14, 2016 at 8:36 AM, Vladimir Ozerov <
> [hidden email]
> > >
> > > > wrote:
> > > >
> > > > > +1
> > > > >
> > > > > BTW, corresponding ticket already exists. You can find it under
> > > umbrella
> > > > > ticket IGNITE-2232
> > > > > 14 янв. 2016 г. 18:40 пользователь "Yakov Zhdanov" <
> > > [hidden email]>
> > > > > написал:
> > > > >
> > > > > > Guys,
> > > > > >
> > > > > > We have startTime(), duration() and endTime() methods which have
> > > > several
> > > > > > usages each along internals of the project, but to support these
> > > > methods
> > > > > we
> > > > > > have 2 long fields in GridFutureAdapter which gives us 16 bytes.
> > > > > >
> > > > > > Other fields - res (reference 8 bytes at max), ignoreInterrupts
> > > > (boolean
> > > > > 1
> > > > > > byte) and resFlag (byte 1 byte) = 10 bytes
> > > > > >
> > > > > > I did quick tests and I see that removing these fields (i.e.
> making
> > > > each
> > > > > > future 16 bytes thinner) can give us 5-6% in performance results.
> > > > > >
> > > > > > I want to deprecate  startTime(), duration() and endTime() and
> > > > therefore
> > > > > > deprecate corresponding methods in IgniteFuture.
> > > > > >
> > > > > > Thoughts?
> > > > > >
> > > > > > --Yakov
> > > > > >
> > > > >
> > > >
> > >
> >
>

[Yakov Zhdanov-2]

Here is the PR https://github.com/apache/ignite/pull/409 - not ready to
merge, but cache full api suite passes, so I can run distributed benchmarks
tomorrow.

Thanks!
--
Yakov Zhdanov, Director R&D
*GridGain Systems*
www.gridgain.com

2016-01-16 19:04 GMT+03:00 Dmitriy Setrakyan <[hidden email]>:

> Yakov,
>
> Any chance you can rerun the benchmarks to confirm your findings?
>
> Would also be nice if someone else could run them as well. Is there a
> branch that we can check out?
>
> D.
>
> On Fri, Jan 15, 2016 at 12:17 PM, Vladimir Ozerov <[hidden email]>
> wrote:
>
> > I also hardly believe that such change can give us such big immediate
> > performance benefit. Profiling shows that in our standard benchmarks
> > futures doesn't generate so much garbage to get ~5% from field removal. I
> > would rather re-check if benchmark is valid.
> >
> > On the other hand, lots of our utility classes like GridFunc, GridUtils,
> > futures, etc., are overly complex and inefficient. As they are used
> almost
> > everywhere, if we see something what can be easily optimized, we should
> do
> > that right away, even if the benefit cannot be observed in benchmarks.
> This
> > will give us confidence that our fundamental abstractions are good
> enough,
> > so we can focus on business logic and algorithms, rather than on
> supporting
> > code.
> >
> > So I think that regardless of the benchmark numbers, we should invest
> > efforts in changes like this.
> >
> > On Fri, Jan 15, 2016 at 6:15 PM, Artem Shutak <[hidden email]>
> > wrote:
> >
> > > If it really can give as 5% of performance we have to do it.
> > >
> > > If anyone uses this methods we can support it by user request (if user
> > > explicitly asked about) and it will work slower in that case. For
> > example,
> > > we can add *IgniteCache.withAsync(boolean useFutureWithTimer)* that
> will
> > > give to user an async cache with futures that support startTime(),
> > > duration() and endTime methods as well.
> > >
> > > -- Artem --
> > >
> > > On Fri, Jan 15, 2016 at 3:10 PM, Yakov Zhdanov <[hidden email]>
> > > wrote:
> > >
> > > > All of optimizations applied to futures so far were extremely
> > effective.
> > > > Ignite can create different number of futures per operation depending
> > on
> > > > context. Multiple this by ops/sec.. This is probably one of the most
> > > > intensively instantiated (and therefore GCed) object. Internal
> futures
> > is
> > > > very sensitive part of the system and should be 100% effective.
> > > >
> > > > As far as public API, anyone
> > > > uses org.apache.ignite.lang.IgniteFuture#startTime
> > > > or org.apache.ignite.lang.IgniteFuture#duration?
> > > >
> > > > --Yakov
> > > >
> > > > 2016-01-15 1:20 GMT+03:00 Dmitriy Setrakyan <[hidden email]>:
> > > >
> > > > > Really hard to believe that removing 16 bytes in futures gives 5%
> of
> > > > > performance. Yakov, are you certain about this?
> > > > >
> > > > > If this turns out to be true, let’s see if we can slim down the
> > futures
> > > > > used internally, without breaking the public API.
> > > > >
> > > > > D.
> > > > >
> > > > > On Thu, Jan 14, 2016 at 8:36 AM, Vladimir Ozerov <
> > [hidden email]
> > > >
> > > > > wrote:
> > > > >
> > > > > > +1
> > > > > >
> > > > > > BTW, corresponding ticket already exists. You can find it under
> > > > umbrella
> > > > > > ticket IGNITE-2232
> > > > > > 14 янв. 2016 г. 18:40 пользователь "Yakov Zhdanov" <
> > > > [hidden email]>
> > > > > > написал:
> > > > > >
> > > > > > > Guys,
> > > > > > >
> > > > > > > We have startTime(), duration() and endTime() methods which
> have
> > > > > several
> > > > > > > usages each along internals of the project, but to support
> these
> > > > > methods
> > > > > > we
> > > > > > > have 2 long fields in GridFutureAdapter which gives us 16
> bytes.
> > > > > > >
> > > > > > > Other fields - res (reference 8 bytes at max), ignoreInterrupts
> > > > > (boolean
> > > > > > 1
> > > > > > > byte) and resFlag (byte 1 byte) = 10 bytes
> > > > > > >
> > > > > > > I did quick tests and I see that removing these fields (i.e.
> > making
> > > > > each
> > > > > > > future 16 bytes thinner) can give us 5-6% in performance
> results.
> > > > > > >
> > > > > > > I want to deprecate  startTime(), duration() and endTime() and
> > > > > therefore
> > > > > > > deprecate corresponding methods in IgniteFuture.
> > > > > > >
> > > > > > > Thoughts?
> > > > > > >
> > > > > > > --Yakov
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>