Deprecate GridFunc

[Vladimir Ozerov]

Igniters,

This is about our infamous *GridFunc *class. I suggest to deprecate it and
strongly discourage any usage of any method from it.

*Motivation:*
Currently this class contains ~170 methods. Lots of this methods have poor
performance characteristics or broken semantics.
- Lots of collection manipulation methods are broken. You can easily loose
O(1) or O(log N) and have O(N). You can easily loose Set semantics - merge
two sets and this is not a set anymore. Etc, etc.. This is a nightmare. +
you allocate garbage.
- Some methods operate on varags while we always need no more than one
element -> unnecessary allocations.

But as these methods are convenient to use, developers tend to use them
without understanding of implications. As our product is
performance-sensitive, we certainly should avoid this.

For example, here is a code snippet from cache IO manager:

if (!F.exist(F.nodeIds(nodes), F0.not(F.contains(leftIds)))) {

    ...

}


And how do you think, what is "nodes" variable? Here it is:

F.view(F.viewReadOnly(ids, U.id2Node(ctx), p), F.notNull())


WTF? We *MUST* stop that.

*Proposal:*
1) Deprecate this class.
2) Move several useful and safe methods (like F.eq()) to separate
specialized utility classes.

Thoughts?

Vladimir.

[Semyon Boikov]

+1

Also need to check these classes with lots of static utility methods:
IgniteUtils and X.

On Fri, Apr 1, 2016 at 11:03 AM, Vladimir Ozerov <[hidden email]>
wrote:

> Igniters,
>
> This is about our infamous *GridFunc *class. I suggest to deprecate it and
> strongly discourage any usage of any method from it.
>
> *Motivation:*
> Currently this class contains ~170 methods. Lots of this methods have poor
> performance characteristics or broken semantics.
> - Lots of collection manipulation methods are broken. You can easily loose
> O(1) or O(log N) and have O(N). You can easily loose Set semantics - merge
> two sets and this is not a set anymore. Etc, etc.. This is a nightmare. +
> you allocate garbage.
> - Some methods operate on varags while we always need no more than one
> element -> unnecessary allocations.
>
> But as these methods are convenient to use, developers tend to use them
> without understanding of implications. As our product is
> performance-sensitive, we certainly should avoid this.
>
> For example, here is a code snippet from cache IO manager:
>
> if (!F.exist(F.nodeIds(nodes), F0.not(F.contains(leftIds)))) {
>
>     ...
>
> }
>
>
> And how do you think, what is "nodes" variable? Here it is:
>
> F.view(F.viewReadOnly(ids, U.id2Node(ctx), p), F.notNull())
>
>
> WTF? We *MUST* stop that.
>
> *Proposal:*
> 1) Deprecate this class.
> 2) Move several useful and safe methods (like F.eq()) to separate
> specialized utility classes.
>
> Thoughts?
>
> Vladimir.
>

[endianignite]

Wow, that really is an infamous class.  Yikes.  +1 from me for deprecate & refactor.  

[yzhdanov]

+1,

However, personally I try not to accept new code on reviews relying on F. I
don't think we have any usages of mentioned methods on critical paths that
may affect performance. So, for now I would ask reviewers to very carefully
accept new code using F.

--Yakov

2016-04-01 10:41 GMT+03:00 endianignite <[hidden email]>:

> Wow, that really is an infamous class.  Yikes.  +1 from me for deprecate &
> refactor.
>
>
>
> --
> View this message in context:
> http://apache-ignite-developers.2346864.n4.nabble.com/Deprecate-GridFunc-tp8214p8217.html
> Sent from the Apache Ignite Developers mailing list archive at Nabble.com.
>

[Vladimir Ozerov]

Yakov,

This is very hard to reason whether it is affect performance or no. There
are lots of "views" on critical paths (cache, IO, discovery, affinity), and
their semantics is virtually unpredictable. Not to say about additional GC
pressure.

Anyway, looks like we have consensus that this class should be avoided.

Vladimir.

On Fri, Apr 1, 2016 at 12:36 PM, Yakov Zhdanov <[hidden email]> wrote:

> +1,
>
> However, personally I try not to accept new code on reviews relying on F. I
> don't think we have any usages of mentioned methods on critical paths that
> may affect performance. So, for now I would ask reviewers to very carefully
> accept new code using F.
>
> --Yakov
>
> 2016-04-01 10:41 GMT+03:00 endianignite <[hidden email]>:
>
> > Wow, that really is an infamous class.  Yikes.  +1 from me for deprecate
> &
> > refactor.
> >
> >
> >
> > --
> > View this message in context:
> >
> http://apache-ignite-developers.2346864.n4.nabble.com/Deprecate-GridFunc-tp8214p8217.html
> > Sent from the Apache Ignite Developers mailing list archive at
> Nabble.com.
> >
>

[Sergi]

+1

I think it is a good idea to mark GridFunc and F with @Depricated
annotation and encourage developers to refactor the code which uses them
when they come across it. This way we'll gradually get rid of this "true
functional programming" stuff.

Sergi

2016-04-01 12:46 GMT+03:00 Vladimir Ozerov <[hidden email]>:

> Yakov,
>
> This is very hard to reason whether it is affect performance or no. There
> are lots of "views" on critical paths (cache, IO, discovery, affinity), and
> their semantics is virtually unpredictable. Not to say about additional GC
> pressure.
>
> Anyway, looks like we have consensus that this class should be avoided.
>
> Vladimir.
>
> On Fri, Apr 1, 2016 at 12:36 PM, Yakov Zhdanov <[hidden email]>
> wrote:
>
> > +1,
> >
> > However, personally I try not to accept new code on reviews relying on
> F. I
> > don't think we have any usages of mentioned methods on critical paths
> that
> > may affect performance. So, for now I would ask reviewers to very
> carefully
> > accept new code using F.
> >
> > --Yakov
> >
> > 2016-04-01 10:41 GMT+03:00 endianignite <[hidden email]>:
> >
> > > Wow, that really is an infamous class.  Yikes.  +1 from me for
> deprecate
> > &
> > > refactor.
> > >
> > >
> > >
> > > --
> > > View this message in context:
> > >
> >
> http://apache-ignite-developers.2346864.n4.nabble.com/Deprecate-GridFunc-tp8214p8217.html
> > > Sent from the Apache Ignite Developers mailing list archive at
> > Nabble.com.
> > >
> >
>