ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Александр Меньшиков <sharple...@gmail.com>
Subject Re: IGNITE-4713 : refactoring of GridFinishedFuture and GridFutureAdapter
Date Wed, 01 Mar 2017 11:55:05 GMT
First of all I want to replace the "byte resFlag" on the "bool haveResult"
only for aesthetic reasons. I think that use a byte as a boolean flag looks
like C in 1983. And constructions like

return haveResult ? (R)res : null;

more readable unlike

return resFlag == RES ? (R)res : null;

because you don't know how many values the "resFlag" can have.
But it's not big deal, so if community disagrees it's okay.

About volatile:

Class "GridFutureAdapter" uses zero value of the "resFlag" in assert for
check that didn't call the "get0" before the "onDone".
So I suggested to use the "getState()" for this check. But of course it's
work only if "get0" and "onDone" are called in a same thread.
But if threads are different (as you said), we have bug any way because the
"resFlag" is not volatile.
In the GridFutureAdapter we haven't any sync between write operations to
the "resFlag" in the "onDone" and the read operation in the "get0". So the
"get0" in the line "assert resFlag != 0" can throw AssertionError, because
a thread with the "get0" may not see the write operation in "onDone" in
other thread.

So I think in this case we need to add volatile in the
GridFutureAdapter.resFlag, and replace the GridFinishedFuture.resFlag on the
GridFinishedFuture.haveResult (or not if you don't like it).


2017-02-28 19:39 GMT+03:00 Yakov Zhdanov <yzhdanov@apache.org>:

> Alexander,
>
> Can you please clarify this?
>
> >I suppose "volatile" is not need here, because right now if
> GridFutureAdapter#onDone()
> called in another thread than GridFutureAdapter#get0, it will produce
> AssertionError.
>
> 1) 99.999% of time GridFutureAdapter#onDone() is called in some other
> thread than the one which is frozen on get()
> 2) What is volatile now which should not be?
>
> Regarding the flag - boolean and byte takes 1 byte in the object. I suppose
> some time ago there were 3 options, but now there are only 2. I don't think
> we need to change it now.
>
>
>
> --Yakov
>
> 2017-02-28 17:32 GMT+03:00 Александр Меньшиков <sharplermc@gmail.com>:
>
> > Sorry, I mean I want to do fast work on this issue (not make code
> faster).
> > Your code is strange. You can see my view in my local temp PR:
> >
> > https://github.com/SharplEr/ignite/pull/4/files
> >
> > This is what I'm meaning.
> >
> > I suppose "volatile" is not need here, because right now if
> > GridFutureAdapter#onDone() called in another thread than
> > GridFutureAdapter#get0, it will produce AssertionError.
> >
> >
> >
> > 2017-02-28 16:57 GMT+03:00 Vladislav Pyatkov <vldpyatkov@gmail.com>:
> >
> > > Alexander,
> > >
> > > Are you sure, which it will be faster?
> > > The condition
> > >
> > > resFlag == RES and resFlag == ERR
> > >
> > > should be more complicated... like something this:
> > >
> > > getState() != 0 &&  !resFlag and getState() != 0 &&  resFlag
> > >
> > > But unlike getState(), restFag is not volatile...
> > >
> > > On Tue, Feb 28, 2017 at 4:26 PM, Александр Меньшиков <
> > sharplermc@gmail.com
> > > > wrote:
> > >
> > >> Alexey, Vladislav, are you agree with me, or not? I want to do it
> faster
> > >> and move on.
> > >>
> > >> 2017-02-17 17:36 GMT+03:00 Александр Меньшиков <sharplermc@gmail.com
> >:
> > >>
> > >>> We can check "onDone" method was called with using getState() method.
> > If
> > >>> getState()!=0 then  resFlag!=0. Just look at code.
> > >>>
> > >>>
> > >>> 2017-02-17 17:12 GMT+03:00 Александр Меньшиков <sharplermc@gmail.com
> >:
> > >>>
> > >>>> Like I said, if "resFlag==0" (of course 0 came from initialization)
> > >>>> means "you still haven't called the method onDone()", better make
it
> > clear.
> > >>>>
> > >>>>
> > >>>>
> > >>>> 2017-02-17 14:48 GMT+03:00 Vladislav Pyatkov <vldpyatkov@gmail.com
> >:
> > >>>>
> > >>>>> Alexander,
> > >>>>>
> > >>>>> I think, the resFlag will be initiated as 0 (new
> > GridFutureAdapter()),
> > >>>>> but
> > >>>>> 1 and 2 states will be acquired on live.
> > >>>>>
> > >>>>>
> > >>>>> On Fri, Feb 17, 2017 at 1:56 PM, Александр Меньшиков
<
> > >>>>> sharplermc@gmail.com>
> > >>>>> wrote:
> > >>>>>
> > >>>>> > Alexey,
> > >>>>> >
> > >>>>> > I see only one place where writes in resFlag:
> > >>>>> >
> > >>>>> >                 if (err != null) {
> > >>>>> >                     resFlag = ERR;
> > >>>>> >                     this.res = err;
> > >>>>> >                 }
> > >>>>> >                 else {
> > >>>>> >                     resFlag = RES;
> > >>>>> >                     this.res = res;
> > >>>>> >                 }
> > >>>>> >
> > >>>>> > And the comparison with only two values: "ERR" and "RES".
Except
> > >>>>> "assert
> > >>>>> > resFlag != 0;". So if this "assert" protect from call
"get0"
> before
> > >>>>> call
> > >>>>> > "onDone" I think will be clearer to set some ready flag
or use
> > >>>>> "enum" type.
> > >>>>> > And throw IllegalStateException if condition is false,
because
> > right
> > >>>>> now
> > >>>>> > developer will not get clear error massage.
> > >>>>> >
> > >>>>> > 17 февр. 2017 г. 11:34 пользователь "Alexey
Goncharuk" <
> > >>>>> > alexey.goncharuk@gmail.com> написал:
> > >>>>> >
> > >>>>> > Alexander,
> > >>>>> >
> > >>>>> > This change is not applicable for GridFutureAdapter because
> resFlag
> > >>>>> can
> > >>>>> > have 3 values there.
> > >>>>> >
> > >>>>> > 2017-02-16 19:58 GMT+03:00 Александр Меньшиков
<
> > sharplermc@gmail.com
> > >>>>> >:
> > >>>>> >
> > >>>>> > > Hello.
> > >>>>> > >
> > >>>>> > > I propose to do refactoring of classes "GridFinishedFuture"
and
> > >>>>> > > "GridFutureAdapter". There is field "resFlag" which
can equals
> > >>>>> "ERR = 1"
> > >>>>> > or
> > >>>>> > > "RES = 2". So I can replace it to one "bool haveResult"
field.
> > >>>>> > >
> > >>>>> > > If there are no objections, I'm ready to proceed.
If you find
> > more
> > >>>>> such
> > >>>>> > > classes, please write about them.
> > >>>>> > >
> > >>>>> >
> > >>>>>
> > >>>>>
> > >>>>>
> > >>>>> --
> > >>>>> Vladislav Pyatkov
> > >>>>>
> > >>>>
> > >>>>
> > >>>
> > >>
> > >
> > >
> > > --
> > > Vladislav Pyatkov
> > >
> >
>

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