Review request

[yzhdanov]

Vladimir Ozerov and Alex Goncharuk, can you please take a look at PR and
provide comments? Other reviewers are welcome, too! =)

https://github.com/apache/ignite/pull/422

I did some changes to decrease allocation pressure and fixed force keys
request not to be sent if rebalancing has already been successfully
completed on operation's topology version.

--Yakov

[dsetrakyan]

Any preliminary performance numbers?

On Mon, Feb 1, 2016 at 8:52 AM, Yakov Zhdanov <[hidden email]> wrote:

> Vladimir Ozerov and Alex Goncharuk, can you please take a look at PR and
> provide comments? Other reviewers are welcome, too! =)
>
> https://github.com/apache/ignite/pull/422
>
> I did some changes to decrease allocation pressure and fixed force keys
> request not to be sent if rebalancing has already been successfully
> completed on operation's topology version.
>
> --Yakov
>

[yzhdanov]

No visible changes to throughput and latency on our common configuration,
but allocation pressure reduced up to 20% in put-get benchmarks.

--Yakov

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

> Any preliminary performance numbers?
>
> On Mon, Feb 1, 2016 at 8:52 AM, Yakov Zhdanov <[hidden email]> wrote:
>
> > Vladimir Ozerov and Alex Goncharuk, can you please take a look at PR and
> > provide comments? Other reviewers are welcome, too! =)
> >
> > https://github.com/apache/ignite/pull/422
> >
> > I did some changes to decrease allocation pressure and fixed force keys
> > request not to be sent if rebalancing has already been successfully
> > completed on operation's topology version.
> >
> > --Yakov
> >
>

[dsetrakyan]

On Mon, Feb 1, 2016 at 10:08 AM, Yakov Zhdanov <[hidden email]> wrote:

> No visible changes to throughput and latency on our common configuration,
> but allocation pressure reduced up to 20% in put-get benchmarks.
>

Nice!

>
> --Yakov
>
> 2016-02-01 20:02 GMT+03:00 Dmitriy Setrakyan <[hidden email]>:
>
> > Any preliminary performance numbers?
> >
> > On Mon, Feb 1, 2016 at 8:52 AM, Yakov Zhdanov <[hidden email]>
> wrote:
> >
> > > Vladimir Ozerov and Alex Goncharuk, can you please take a look at PR
> and
> > > provide comments? Other reviewers are welcome, too! =)
> > >
> > > https://github.com/apache/ignite/pull/422
> > >
> > > I did some changes to decrease allocation pressure and fixed force keys
> > > request not to be sent if rebalancing has already been successfully
> > > completed on operation's topology version.
> > >
> > > --Yakov
> > >
> >
>

[Vladimir Ozerov]

As per cache - I hardly understand affected logic, so my review wouldn't
help much here.

As per the rest changes - looks good for me. I also see garbage from NIO
and "force keys" as huge memory hotspots. The only problem is
GridCompoundFuture:

if (futs == null)
    futs = new ArrayList<>();

futs.add(fut);


Things like this are proven to be anti-pattern in terms of memory
allocations, because on the last line you effectively allocate Object[10],
while usually you will have much less child futures. We could delay
allocation of array if we store the very first child future as direct
reference. And only second added future should lead to ArrayList
allocation. This should positively affect lots operations with "compound
semantics" and single cache key involved (e.g. single puts/gets).

On Mon, Feb 1, 2016 at 9:08 PM, Yakov Zhdanov <[hidden email]> wrote:

> No visible changes to throughput and latency on our common configuration,
> but allocation pressure reduced up to 20% in put-get benchmarks.
>
> --Yakov
>
> 2016-02-01 20:02 GMT+03:00 Dmitriy Setrakyan <[hidden email]>:
>
> > Any preliminary performance numbers?
> >
> > On Mon, Feb 1, 2016 at 8:52 AM, Yakov Zhdanov <[hidden email]>
> wrote:
> >
> > > Vladimir Ozerov and Alex Goncharuk, can you please take a look at PR
> and
> > > provide comments? Other reviewers are welcome, too! =)
> > >
> > > https://github.com/apache/ignite/pull/422
> > >
> > > I did some changes to decrease allocation pressure and fixed force keys
> > > request not to be sent if rebalancing has already been successfully
> > > completed on operation's topology version.
> > >
> > > --Yakov
> > >
> >
>

[dsetrakyan]

On Wed, Feb 3, 2016 at 7:02 AM, Vladimir Ozerov <[hidden email]>
wrote:

> As per cache - I hardly understand affected logic, so my review wouldn't
> help much here.
>
> As per the rest changes - looks good for me. I also see garbage from NIO
> and "force keys" as huge memory hotspots. The only problem is
> GridCompoundFuture:
>
> if (futs == null)
>     futs = new ArrayList<>();
>
> futs.add(fut);
>
>
> Things like this are proven to be anti-pattern in terms of memory
> allocations, because on the last line you effectively allocate Object[10],
> while usually you will have much less child futures. We could delay
> allocation of array if we store the very first child future as direct
> reference. And only second added future should lead to ArrayList
> allocation. This should positively affect lots operations with "compound
> semantics" and single cache key involved (e.g. single puts/gets).
>

Vova,
I couldn’t agree more. Can we try it out and see what kind of GC or
performance improvement we get?

> On Mon, Feb 1, 2016 at 9:08 PM, Yakov Zhdanov <[hidden email]> wrote:
>
> > No visible changes to throughput and latency on our common configuration,
> > but allocation pressure reduced up to 20% in put-get benchmarks.
> >
> > --Yakov
> >
> > 2016-02-01 20:02 GMT+03:00 Dmitriy Setrakyan <[hidden email]>:
> >
> > > Any preliminary performance numbers?
> > >
> > > On Mon, Feb 1, 2016 at 8:52 AM, Yakov Zhdanov <[hidden email]>
> > wrote:
> > >
> > > > Vladimir Ozerov and Alex Goncharuk, can you please take a look at PR
> > and
> > > > provide comments? Other reviewers are welcome, too! =)
> > > >
> > > > https://github.com/apache/ignite/pull/422
> > > >
> > > > I did some changes to decrease allocation pressure and fixed force
> keys
> > > > request not to be sent if rebalancing has already been successfully
> > > > completed on operation's topology version.
> > > >
> > > > --Yakov
> > > >
> > >
> >
>

[yzhdanov]

I think we should not allocate list at all! We can just add listener to
added futures!:) and the semantics will be preserved!

However this will not work if we want to iterate over the added ones.
Number of unfinished futures will be still available through listener calls
count. Let me review.

If this doesn't work, we can maintain an array on our own.

And the last point.. This GC impact should be measured. What happen if you
add a reference to future and reference to collection?

Yakov
On Feb 3, 2016 7:44 PM, "Dmitriy Setrakyan" <[hidden email]> wrote:

> On Wed, Feb 3, 2016 at 7:02 AM, Vladimir Ozerov <[hidden email]>
> wrote:
>
> > As per cache - I hardly understand affected logic, so my review wouldn't
> > help much here.
> >
> > As per the rest changes - looks good for me. I also see garbage from NIO
> > and "force keys" as huge memory hotspots. The only problem is
> > GridCompoundFuture:
> >
> > if (futs == null)
> >     futs = new ArrayList<>();
> >
> > futs.add(fut);
> >
> >
> > Things like this are proven to be anti-pattern in terms of memory
> > allocations, because on the last line you effectively allocate
> Object[10],
> > while usually you will have much less child futures. We could delay
> > allocation of array if we store the very first child future as direct
> > reference. And only second added future should lead to ArrayList
> > allocation. This should positively affect lots operations with "compound
> > semantics" and single cache key involved (e.g. single puts/gets).
> >
>
> Vova,
> I couldn’t agree more. Can we try it out and see what kind of GC or
> performance improvement we get?
>
>
> > On Mon, Feb 1, 2016 at 9:08 PM, Yakov Zhdanov <[hidden email]>
> wrote:
> >
> > > No visible changes to throughput and latency on our common
> configuration,
> > > but allocation pressure reduced up to 20% in put-get benchmarks.
> > >
> > > --Yakov
> > >
> > > 2016-02-01 20:02 GMT+03:00 Dmitriy Setrakyan <[hidden email]>:
> > >
> > > > Any preliminary performance numbers?
> > > >
> > > > On Mon, Feb 1, 2016 at 8:52 AM, Yakov Zhdanov <[hidden email]>
> > > wrote:
> > > >
> > > > > Vladimir Ozerov and Alex Goncharuk, can you please take a look at
> PR
> > > and
> > > > > provide comments? Other reviewers are welcome, too! =)
> > > > >
> > > > > https://github.com/apache/ignite/pull/422
> > > > >
> > > > > I did some changes to decrease allocation pressure and fixed force
> > keys
> > > > > request not to be sent if rebalancing has already been successfully
> > > > > completed on operation's topology version.
> > > > >
> > > > > --Yakov
> > > > >
> > > >
> > >
> >
>