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 <voze...@gridgain.com> 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 <ashu...@gridgain.com> > 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 <yzhda...@apache.org> > > 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 <dsetrak...@apache.org>: > > > > > > > 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 < > voze...@gridgain.com > > > > > > > wrote: > > > > > > > > > +1 > > > > > > > > > > BTW, corresponding ticket already exists. You can find it under > > > umbrella > > > > > ticket IGNITE-2232 > > > > > 14 янв. 2016 г. 18:40 пользователь "Yakov Zhdanov" < > > > yzhda...@apache.org> > > > > > написал: > > > > > > > > > > > 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 > > > > > > > > > > > > > > > > > > > > >
