IgniteUtils#currentTimeMillis

classic Classic list List threaded Threaded
22 messages Options
12
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

IgniteUtils#currentTimeMillis

Николай Ижиков
Hello, Igniters.

I found unusual implementation of IgniteUtils#currentTimeMillis function.
Method is not simple proxy to System.currentTimeMillis
Instead it read static variable which updated by dedicated thread:

https://github.com/apache/ignite/blob/master/modules/core/src/main/java/org/apache/ignite/internal/util/IgniteUtils.java#L3251

while (true) {
    curTimeMillis = System.currentTimeMillis();

    try {
        Thread.sleep(10);
    }
    catch (InterruptedException ignored) {
        break;
    }
}

As far as i know `Thread.sleep` is unprecise function especially under
heavy load and on small timeout values like 10 ms.
Seems that we get very unprecise values of currentTimeMillis.

I think that can lead to difficult bugs like
https://issues.apache.org/jira/browse/IGNITE-5963

What is purpose of current implementation?

--
Nikolay Izhikov
[hidden email]
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: IgniteUtils#currentTimeMillis

Николай Ижиков
Addition to my previous message:

1. IgniteUtils#currentTimeMillis used in current master to check
transaction timeout:

https://github.com/apache/ignite/blob/master/modules/
core/src/main/java/org/apache/ignite/internal/processors/cache/transactions/
IgniteTxAdapter.java#L664

2. According to jdk docs System.nanoTime() is only method to measure
elapsed time
https://docs.oracle.com/javase/7/docs/api/java/lang/System.html#nanoTime().

"This method can only be used to measure elapsed time and is not related to
any other notion of system or wall-clock time"

What are reasons to use System.currentTimeMillis() in the way it used for
now?


2017-08-07 18:42 GMT+03:00 Николай Ижиков <[hidden email]>:

> Hello, Igniters.
>
> I found unusual implementation of IgniteUtils#currentTimeMillis function.
> Method is not simple proxy to System.currentTimeMillis
> Instead it read static variable which updated by dedicated thread:
>
> https://github.com/apache/ignite/blob/master/modules/
> core/src/main/java/org/apache/ignite/internal/util/IgniteUtils.java#L3251
>
> while (true) {
>     curTimeMillis = System.currentTimeMillis();
>
>     try {
>         Thread.sleep(10);
>     }
>     catch (InterruptedException ignored) {
>         break;
>     }
> }
>
> As far as i know `Thread.sleep` is unprecise function especially under
> heavy load and on small timeout values like 10 ms.
> Seems that we get very unprecise values of currentTimeMillis.
>
> I think that can lead to difficult bugs like https://issues.apache.
> org/jira/browse/IGNITE-5963
>
> What is purpose of current implementation?
>
> --
> Nikolay Izhikov
> [hidden email]
>



--
Nikolay Izhikov
[hidden email]
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: IgniteUtils#currentTimeMillis

yzhdanov
Nickolay, IgniteUtils#currentTimeMillis() is some kind of an old heritage.
I guess nobody remembers when this method has been introduced. I agree that
we can use System.currentTimeMillis(). I would suggest you file a ticket
and replace this method calls with System.currentTimeMillis(). Sounds good?

As far as reliable elapsed time measurement I agree with you that
nanoTime() is better here, but it is definitely not a reason for mentioned
failure, since that test is launched in single JVM on a machine that most
probably does not do any ntp syncs during the test to make Ignite's timeout
machinery fail.

Please file a ticket to switch Ignite's timeouts to nanoTime() at some
point.

--Yakov
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: IgniteUtils#currentTimeMillis

Vladimir Ozerov
Java could do some heavy stuff when doing currentTimeMillis, depending on
the platform or vendor. I remember I saw some articles about performance
issues caused by currentTimeMillis (something about high contention on
certain OS configuration). So I do not see a reason why we should remove
it. It is not very good in terms of resolution, but AFAIK we do need it
anyway.

On Wed, Aug 9, 2017 at 2:41 PM, Yakov Zhdanov <[hidden email]> wrote:

> Nickolay, IgniteUtils#currentTimeMillis() is some kind of an old heritage.
> I guess nobody remembers when this method has been introduced. I agree that
> we can use System.currentTimeMillis(). I would suggest you file a ticket
> and replace this method calls with System.currentTimeMillis(). Sounds good?
>
> As far as reliable elapsed time measurement I agree with you that
> nanoTime() is better here, but it is definitely not a reason for mentioned
> failure, since that test is launched in single JVM on a machine that most
> probably does not do any ntp syncs during the test to make Ignite's timeout
> machinery fail.
>
> Please file a ticket to switch Ignite's timeouts to nanoTime() at some
> point.
>
> --Yakov
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: IgniteUtils#currentTimeMillis

Dmitry Pavlov
In reply to this post by yzhdanov
It seems System.currentTimeMillis () is now in intrinsic list. This means
on modern JVMs performance penalty will not be so significiant.

Nickolay, could you please raise standalone ticket for U.currentTimeMillis
() ?

Could you also please check if system.nanoTime / system.currentTimeMs can
fix https://issues.apache.org/jira/browse/IGNITE-5963 When you create a PR,
I can start several run for Ignite Cache 6 suite to check if issue is still
reprodacible.

ср, 9 авг. 2017 г. в 14:41, Yakov Zhdanov <[hidden email]>:

> Nickolay, IgniteUtils#currentTimeMillis() is some kind of an old heritage.
> I guess nobody remembers when this method has been introduced. I agree that
> we can use System.currentTimeMillis(). I would suggest you file a ticket
> and replace this method calls with System.currentTimeMillis(). Sounds good?
>
> As far as reliable elapsed time measurement I agree with you that
> nanoTime() is better here, but it is definitely not a reason for mentioned
> failure, since that test is launched in single JVM on a machine that most
> probably does not do any ntp syncs during the test to make Ignite's timeout
> machinery fail.
>
> Please file a ticket to switch Ignite's timeouts to nanoTime() at some
> point.
>
> --Yakov
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: IgniteUtils#currentTimeMillis

Dmitry Pavlov
Vladimir, could we check it using benchmarks? Internet contains a lot of
articles about this issue. But do we know if it is still actual for new VMs?

ср, 9 авг. 2017 г. в 14:50, Dmitry Pavlov <[hidden email]>:

> It seems System.currentTimeMillis () is now in intrinsic list. This means
> on modern JVMs performance penalty will not be so significiant.
>
> Nickolay, could you please raise standalone ticket for U.currentTimeMillis
> () ?
>
> Could you also please check if system.nanoTime / system.currentTimeMs can
> fix https://issues.apache.org/jira/browse/IGNITE-5963 When you create a
> PR, I can start several run for Ignite Cache 6 suite to check if issue is
> still reprodacible.
>
> ср, 9 авг. 2017 г. в 14:41, Yakov Zhdanov <[hidden email]>:
>
>> Nickolay, IgniteUtils#currentTimeMillis() is some kind of an old heritage.
>> I guess nobody remembers when this method has been introduced. I agree
>> that
>> we can use System.currentTimeMillis(). I would suggest you file a ticket
>> and replace this method calls with System.currentTimeMillis(). Sounds
>> good?
>>
>> As far as reliable elapsed time measurement I agree with you that
>> nanoTime() is better here, but it is definitely not a reason for mentioned
>> failure, since that test is launched in single JVM on a machine that most
>> probably does not do any ntp syncs during the test to make Ignite's
>> timeout
>> machinery fail.
>>
>> Please file a ticket to switch Ignite's timeouts to nanoTime() at some
>> point.
>>
>> --Yakov
>>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: IgniteUtils#currentTimeMillis

Николай Ижиков
In reply to this post by Dmitry Pavlov
Hello, Dima.

> Nickolay, could you please raise standalone ticket for U.currentTimeMillis
() ?

Yes, I can.

> https://issues.apache.org/jira/browse/IGNITE-5963

I will try to make quick fix for issue without change Ignite core code.
So fix will not depend to U.currentTimeMillis() implementation.


2017-08-09 14:50 GMT+03:00 Dmitry Pavlov <[hidden email]>:

> It seems System.currentTimeMillis () is now in intrinsic list. This means
> on modern JVMs performance penalty will not be so significiant.
>
> Nickolay, could you please raise standalone ticket for U.currentTimeMillis
> () ?
>
> Could you also please check if system.nanoTime / system.currentTimeMs can
> fix https://issues.apache.org/jira/browse/IGNITE-5963 When you create a
> PR,
> I can start several run for Ignite Cache 6 suite to check if issue is still
> reprodacible.
>
> ср, 9 авг. 2017 г. в 14:41, Yakov Zhdanov <[hidden email]>:
>
> > Nickolay, IgniteUtils#currentTimeMillis() is some kind of an old
> heritage.
> > I guess nobody remembers when this method has been introduced. I agree
> that
> > we can use System.currentTimeMillis(). I would suggest you file a ticket
> > and replace this method calls with System.currentTimeMillis(). Sounds
> good?
> >
> > As far as reliable elapsed time measurement I agree with you that
> > nanoTime() is better here, but it is definitely not a reason for
> mentioned
> > failure, since that test is launched in single JVM on a machine that most
> > probably does not do any ntp syncs during the test to make Ignite's
> timeout
> > machinery fail.
> >
> > Please file a ticket to switch Ignite's timeouts to nanoTime() at some
> > point.
> >
> > --Yakov
> >
>



--
Nikolay Izhikov
[hidden email]
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: IgniteUtils#currentTimeMillis

Vladimir Ozerov
In reply to this post by Dmitry Pavlov
You cannot check it with benchmarks, because behavior of this method will
vary between different JVMs, OSes and hardware. It can be different even
with the same OS depending on it's settings. Again - let's just avoid
unnecessary work. There is nothing wrong with U.currentTimeMillis() at the
moment.

On Wed, Aug 9, 2017 at 2:52 PM, Dmitry Pavlov <[hidden email]> wrote:

> Vladimir, could we check it using benchmarks? Internet contains a lot of
> articles about this issue. But do we know if it is still actual for new
> VMs?
>
> ср, 9 авг. 2017 г. в 14:50, Dmitry Pavlov <[hidden email]>:
>
> > It seems System.currentTimeMillis () is now in intrinsic list. This means
> > on modern JVMs performance penalty will not be so significiant.
> >
> > Nickolay, could you please raise standalone ticket for
> U.currentTimeMillis
> > () ?
> >
> > Could you also please check if system.nanoTime / system.currentTimeMs can
> > fix https://issues.apache.org/jira/browse/IGNITE-5963 When you create a
> > PR, I can start several run for Ignite Cache 6 suite to check if issue is
> > still reprodacible.
> >
> > ср, 9 авг. 2017 г. в 14:41, Yakov Zhdanov <[hidden email]>:
> >
> >> Nickolay, IgniteUtils#currentTimeMillis() is some kind of an old
> heritage.
> >> I guess nobody remembers when this method has been introduced. I agree
> >> that
> >> we can use System.currentTimeMillis(). I would suggest you file a ticket
> >> and replace this method calls with System.currentTimeMillis(). Sounds
> >> good?
> >>
> >> As far as reliable elapsed time measurement I agree with you that
> >> nanoTime() is better here, but it is definitely not a reason for
> mentioned
> >> failure, since that test is launched in single JVM on a machine that
> most
> >> probably does not do any ntp syncs during the test to make Ignite's
> >> timeout
> >> machinery fail.
> >>
> >> Please file a ticket to switch Ignite's timeouts to nanoTime() at some
> >> point.
> >>
> >> --Yakov
> >>
> >
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: IgniteUtils#currentTimeMillis

Николай Ижиков
Vladimir,

> There is nothing wrong with U.currentTimeMillis() at the
moment.

I think we can't rely on the return value for time measurement.
Is it true? Is it OK for you?

It very counterintuitive for me as newcomer.


2017-08-09 14:55 GMT+03:00 Vladimir Ozerov <[hidden email]>:

> You cannot check it with benchmarks, because behavior of this method will
> vary between different JVMs, OSes and hardware. It can be different even
> with the same OS depending on it's settings. Again - let's just avoid
> unnecessary work. There is nothing wrong with U.currentTimeMillis() at the
> moment.
>
> On Wed, Aug 9, 2017 at 2:52 PM, Dmitry Pavlov <[hidden email]>
> wrote:
>
> > Vladimir, could we check it using benchmarks? Internet contains a lot of
> > articles about this issue. But do we know if it is still actual for new
> > VMs?
> >
> > ср, 9 авг. 2017 г. в 14:50, Dmitry Pavlov <[hidden email]>:
> >
> > > It seems System.currentTimeMillis () is now in intrinsic list. This
> means
> > > on modern JVMs performance penalty will not be so significiant.
> > >
> > > Nickolay, could you please raise standalone ticket for
> > U.currentTimeMillis
> > > () ?
> > >
> > > Could you also please check if system.nanoTime / system.currentTimeMs
> can
> > > fix https://issues.apache.org/jira/browse/IGNITE-5963 When you create
> a
> > > PR, I can start several run for Ignite Cache 6 suite to check if issue
> is
> > > still reprodacible.
> > >
> > > ср, 9 авг. 2017 г. в 14:41, Yakov Zhdanov <[hidden email]>:
> > >
> > >> Nickolay, IgniteUtils#currentTimeMillis() is some kind of an old
> > heritage.
> > >> I guess nobody remembers when this method has been introduced. I agree
> > >> that
> > >> we can use System.currentTimeMillis(). I would suggest you file a
> ticket
> > >> and replace this method calls with System.currentTimeMillis(). Sounds
> > >> good?
> > >>
> > >> As far as reliable elapsed time measurement I agree with you that
> > >> nanoTime() is better here, but it is definitely not a reason for
> > mentioned
> > >> failure, since that test is launched in single JVM on a machine that
> > most
> > >> probably does not do any ntp syncs during the test to make Ignite's
> > >> timeout
> > >> machinery fail.
> > >>
> > >> Please file a ticket to switch Ignite's timeouts to nanoTime() at some
> > >> point.
> > >>
> > >> --Yakov
> > >>
> > >
> >
>



--
Nikolay Izhikov
[hidden email]
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: IgniteUtils#currentTimeMillis

dsetrakyan
On Wed, Aug 9, 2017 at 4:59 AM, Николай Ижиков <[hidden email]>
wrote:

> Vladimir,
>
> > There is nothing wrong with U.currentTimeMillis() at the
> moment.
>
> I think we can't rely on the return value for time measurement.
> Is it true? Is it OK for you?
>
> It very counterintuitive for me as newcomer.
>

I agree with Vladimir. There is nothing broken, yet we are trying to fix
something. However, we are definitely running a risk of breaking something,
if we "fix" it. I would leave this method alone.

Nikolai, if you believe that the method is not reliable, write a test that
will demonstrate it.


>
> 2017-08-09 14:55 GMT+03:00 Vladimir Ozerov <[hidden email]>:
>
> > You cannot check it with benchmarks, because behavior of this method will
> > vary between different JVMs, OSes and hardware. It can be different even
> > with the same OS depending on it's settings. Again - let's just avoid
> > unnecessary work. There is nothing wrong with U.currentTimeMillis() at
> the
> > moment.
> >
> > On Wed, Aug 9, 2017 at 2:52 PM, Dmitry Pavlov <[hidden email]>
> > wrote:
> >
> > > Vladimir, could we check it using benchmarks? Internet contains a lot
> of
> > > articles about this issue. But do we know if it is still actual for new
> > > VMs?
> > >
> > > ср, 9 авг. 2017 г. в 14:50, Dmitry Pavlov <[hidden email]>:
> > >
> > > > It seems System.currentTimeMillis () is now in intrinsic list. This
> > means
> > > > on modern JVMs performance penalty will not be so significiant.
> > > >
> > > > Nickolay, could you please raise standalone ticket for
> > > U.currentTimeMillis
> > > > () ?
> > > >
> > > > Could you also please check if system.nanoTime / system.currentTimeMs
> > can
> > > > fix https://issues.apache.org/jira/browse/IGNITE-5963 When you
> create
> > a
> > > > PR, I can start several run for Ignite Cache 6 suite to check if
> issue
> > is
> > > > still reprodacible.
> > > >
> > > > ср, 9 авг. 2017 г. в 14:41, Yakov Zhdanov <[hidden email]>:
> > > >
> > > >> Nickolay, IgniteUtils#currentTimeMillis() is some kind of an old
> > > heritage.
> > > >> I guess nobody remembers when this method has been introduced. I
> agree
> > > >> that
> > > >> we can use System.currentTimeMillis(). I would suggest you file a
> > ticket
> > > >> and replace this method calls with System.currentTimeMillis().
> Sounds
> > > >> good?
> > > >>
> > > >> As far as reliable elapsed time measurement I agree with you that
> > > >> nanoTime() is better here, but it is definitely not a reason for
> > > mentioned
> > > >> failure, since that test is launched in single JVM on a machine that
> > > most
> > > >> probably does not do any ntp syncs during the test to make Ignite's
> > > >> timeout
> > > >> machinery fail.
> > > >>
> > > >> Please file a ticket to switch Ignite's timeouts to nanoTime() at
> some
> > > >> point.
> > > >>
> > > >> --Yakov
> > > >>
> > > >
> > >
> >
>
>
>
> --
> Nikolay Izhikov
> [hidden email]
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: IgniteUtils#currentTimeMillis

Dmitry Pavlov
In reply to this post by Николай Ижиков
+1 to Nikolay, this is very often question from newcomers.
It is not clear that current* method may return outdated value from some
moment in past.

Nikolay, how long outdated value can be returned by method?

ср, 9 авг. 2017 г. в 15:00, Николай Ижиков <[hidden email]>:

> Vladimir,
>
> > There is nothing wrong with U.currentTimeMillis() at the
> moment.
>
> I think we can't rely on the return value for time measurement.
> Is it true? Is it OK for you?
>
> It very counterintuitive for me as newcomer.
>
>
> 2017-08-09 14:55 GMT+03:00 Vladimir Ozerov <[hidden email]>:
>
> > You cannot check it with benchmarks, because behavior of this method will
> > vary between different JVMs, OSes and hardware. It can be different even
> > with the same OS depending on it's settings. Again - let's just avoid
> > unnecessary work. There is nothing wrong with U.currentTimeMillis() at
> the
> > moment.
> >
> > On Wed, Aug 9, 2017 at 2:52 PM, Dmitry Pavlov <[hidden email]>
> > wrote:
> >
> > > Vladimir, could we check it using benchmarks? Internet contains a lot
> of
> > > articles about this issue. But do we know if it is still actual for new
> > > VMs?
> > >
> > > ср, 9 авг. 2017 г. в 14:50, Dmitry Pavlov <[hidden email]>:
> > >
> > > > It seems System.currentTimeMillis () is now in intrinsic list. This
> > means
> > > > on modern JVMs performance penalty will not be so significiant.
> > > >
> > > > Nickolay, could you please raise standalone ticket for
> > > U.currentTimeMillis
> > > > () ?
> > > >
> > > > Could you also please check if system.nanoTime / system.currentTimeMs
> > can
> > > > fix https://issues.apache.org/jira/browse/IGNITE-5963 When you
> create
> > a
> > > > PR, I can start several run for Ignite Cache 6 suite to check if
> issue
> > is
> > > > still reprodacible.
> > > >
> > > > ср, 9 авг. 2017 г. в 14:41, Yakov Zhdanov <[hidden email]>:
> > > >
> > > >> Nickolay, IgniteUtils#currentTimeMillis() is some kind of an old
> > > heritage.
> > > >> I guess nobody remembers when this method has been introduced. I
> agree
> > > >> that
> > > >> we can use System.currentTimeMillis(). I would suggest you file a
> > ticket
> > > >> and replace this method calls with System.currentTimeMillis().
> Sounds
> > > >> good?
> > > >>
> > > >> As far as reliable elapsed time measurement I agree with you that
> > > >> nanoTime() is better here, but it is definitely not a reason for
> > > mentioned
> > > >> failure, since that test is launched in single JVM on a machine that
> > > most
> > > >> probably does not do any ntp syncs during the test to make Ignite's
> > > >> timeout
> > > >> machinery fail.
> > > >>
> > > >> Please file a ticket to switch Ignite's timeouts to nanoTime() at
> some
> > > >> point.
> > > >>
> > > >> --Yakov
> > > >>
> > > >
> > >
> >
>
>
>
> --
> Nikolay Izhikov
> [hidden email]
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: IgniteUtils#currentTimeMillis

Vladimir Ozerov
Guys,

Are you really suggesting change the product for newcomer needs? This is
not an argument for the change. Can someone explain me what is currently
broken from product's perspective? Yes, we can get stale values for some
time, we know that. Does it break something?

On Wed, Aug 9, 2017 at 3:11 PM, Dmitry Pavlov <[hidden email]> wrote:

> +1 to Nikolay, this is very often question from newcomers.
> It is not clear that current* method may return outdated value from some
> moment in past.
>
> Nikolay, how long outdated value can be returned by method?
>
> ср, 9 авг. 2017 г. в 15:00, Николай Ижиков <[hidden email]>:
>
> > Vladimir,
> >
> > > There is nothing wrong with U.currentTimeMillis() at the
> > moment.
> >
> > I think we can't rely on the return value for time measurement.
> > Is it true? Is it OK for you?
> >
> > It very counterintuitive for me as newcomer.
> >
> >
> > 2017-08-09 14:55 GMT+03:00 Vladimir Ozerov <[hidden email]>:
> >
> > > You cannot check it with benchmarks, because behavior of this method
> will
> > > vary between different JVMs, OSes and hardware. It can be different
> even
> > > with the same OS depending on it's settings. Again - let's just avoid
> > > unnecessary work. There is nothing wrong with U.currentTimeMillis() at
> > the
> > > moment.
> > >
> > > On Wed, Aug 9, 2017 at 2:52 PM, Dmitry Pavlov <[hidden email]>
> > > wrote:
> > >
> > > > Vladimir, could we check it using benchmarks? Internet contains a lot
> > of
> > > > articles about this issue. But do we know if it is still actual for
> new
> > > > VMs?
> > > >
> > > > ср, 9 авг. 2017 г. в 14:50, Dmitry Pavlov <[hidden email]>:
> > > >
> > > > > It seems System.currentTimeMillis () is now in intrinsic list. This
> > > means
> > > > > on modern JVMs performance penalty will not be so significiant.
> > > > >
> > > > > Nickolay, could you please raise standalone ticket for
> > > > U.currentTimeMillis
> > > > > () ?
> > > > >
> > > > > Could you also please check if system.nanoTime /
> system.currentTimeMs
> > > can
> > > > > fix https://issues.apache.org/jira/browse/IGNITE-5963 When you
> > create
> > > a
> > > > > PR, I can start several run for Ignite Cache 6 suite to check if
> > issue
> > > is
> > > > > still reprodacible.
> > > > >
> > > > > ср, 9 авг. 2017 г. в 14:41, Yakov Zhdanov <[hidden email]>:
> > > > >
> > > > >> Nickolay, IgniteUtils#currentTimeMillis() is some kind of an old
> > > > heritage.
> > > > >> I guess nobody remembers when this method has been introduced. I
> > agree
> > > > >> that
> > > > >> we can use System.currentTimeMillis(). I would suggest you file a
> > > ticket
> > > > >> and replace this method calls with System.currentTimeMillis().
> > Sounds
> > > > >> good?
> > > > >>
> > > > >> As far as reliable elapsed time measurement I agree with you that
> > > > >> nanoTime() is better here, but it is definitely not a reason for
> > > > mentioned
> > > > >> failure, since that test is launched in single JVM on a machine
> that
> > > > most
> > > > >> probably does not do any ntp syncs during the test to make
> Ignite's
> > > > >> timeout
> > > > >> machinery fail.
> > > > >>
> > > > >> Please file a ticket to switch Ignite's timeouts to nanoTime() at
> > some
> > > > >> point.
> > > > >>
> > > > >> --Yakov
> > > > >>
> > > > >
> > > >
> > >
> >
> >
> >
> > --
> > Nikolay Izhikov
> > [hidden email]
> >
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: IgniteUtils#currentTimeMillis

Николай Ижиков
Vladimir,

As far as I can understand behaviour of U.currentTimeMillis() breaks
transaction timeout test:

https://issues.apache.org/jira/browse/IGNITE-5963

IgniteOptimisticTxSuspendResumeTest#testTxTimeoutOnSuspend :

```
final Transaction tx = ignite.transactions().txStart(OPTIMISTIC, isolation,
TX_TIMEOUT, 0);

cache.put(1, 1);

Thread.sleep(TX_TIMEOUT * 2);

GridTestUtils.assertThrowsWithCause(new Callable<Object>() {
    @Override public Object call() throws Exception {
        tx.suspend();

        return null;
    }
}, TransactionTimeoutException.class);
```
Timeout checked like that:

IgniteTxAdapter#remainingTime

```
@Override public long remainingTime() {
    if (timeout() <= 0)
        return 0;

    long timeLeft = timeout() - (U.currentTimeMillis() - startTime());

    return timeLeft <= 0 ? -1 : timeLeft;

}

```


2017-08-09 15:26 GMT+03:00 Vladimir Ozerov <[hidden email]>:

> Guys,
>
> Are you really suggesting change the product for newcomer needs? This is
> not an argument for the change. Can someone explain me what is currently
> broken from product's perspective? Yes, we can get stale values for some
> time, we know that. Does it break something?
>
> On Wed, Aug 9, 2017 at 3:11 PM, Dmitry Pavlov <[hidden email]>
> wrote:
>
> > +1 to Nikolay, this is very often question from newcomers.
> > It is not clear that current* method may return outdated value from some
> > moment in past.
> >
> > Nikolay, how long outdated value can be returned by method?
> >
> > ср, 9 авг. 2017 г. в 15:00, Николай Ижиков <[hidden email]>:
> >
> > > Vladimir,
> > >
> > > > There is nothing wrong with U.currentTimeMillis() at the
> > > moment.
> > >
> > > I think we can't rely on the return value for time measurement.
> > > Is it true? Is it OK for you?
> > >
> > > It very counterintuitive for me as newcomer.
> > >
> > >
> > > 2017-08-09 14:55 GMT+03:00 Vladimir Ozerov <[hidden email]>:
> > >
> > > > You cannot check it with benchmarks, because behavior of this method
> > will
> > > > vary between different JVMs, OSes and hardware. It can be different
> > even
> > > > with the same OS depending on it's settings. Again - let's just avoid
> > > > unnecessary work. There is nothing wrong with U.currentTimeMillis()
> at
> > > the
> > > > moment.
> > > >
> > > > On Wed, Aug 9, 2017 at 2:52 PM, Dmitry Pavlov <[hidden email]
> >
> > > > wrote:
> > > >
> > > > > Vladimir, could we check it using benchmarks? Internet contains a
> lot
> > > of
> > > > > articles about this issue. But do we know if it is still actual for
> > new
> > > > > VMs?
> > > > >
> > > > > ср, 9 авг. 2017 г. в 14:50, Dmitry Pavlov <[hidden email]>:
> > > > >
> > > > > > It seems System.currentTimeMillis () is now in intrinsic list.
> This
> > > > means
> > > > > > on modern JVMs performance penalty will not be so significiant.
> > > > > >
> > > > > > Nickolay, could you please raise standalone ticket for
> > > > > U.currentTimeMillis
> > > > > > () ?
> > > > > >
> > > > > > Could you also please check if system.nanoTime /
> > system.currentTimeMs
> > > > can
> > > > > > fix https://issues.apache.org/jira/browse/IGNITE-5963 When you
> > > create
> > > > a
> > > > > > PR, I can start several run for Ignite Cache 6 suite to check if
> > > issue
> > > > is
> > > > > > still reprodacible.
> > > > > >
> > > > > > ср, 9 авг. 2017 г. в 14:41, Yakov Zhdanov <[hidden email]>:
> > > > > >
> > > > > >> Nickolay, IgniteUtils#currentTimeMillis() is some kind of an
> old
> > > > > heritage.
> > > > > >> I guess nobody remembers when this method has been introduced. I
> > > agree
> > > > > >> that
> > > > > >> we can use System.currentTimeMillis(). I would suggest you file
> a
> > > > ticket
> > > > > >> and replace this method calls with System.currentTimeMillis().
> > > Sounds
> > > > > >> good?
> > > > > >>
> > > > > >> As far as reliable elapsed time measurement I agree with you
> that
> > > > > >> nanoTime() is better here, but it is definitely not a reason for
> > > > > mentioned
> > > > > >> failure, since that test is launched in single JVM on a machine
> > that
> > > > > most
> > > > > >> probably does not do any ntp syncs during the test to make
> > Ignite's
> > > > > >> timeout
> > > > > >> machinery fail.
> > > > > >>
> > > > > >> Please file a ticket to switch Ignite's timeouts to nanoTime()
> at
> > > some
> > > > > >> point.
> > > > > >>
> > > > > >> --Yakov
> > > > > >>
> > > > > >
> > > > >
> > > >
> > >
> > >
> > >
> > > --
> > > Nikolay Izhikov
> > > [hidden email]
> > >
> >
>



--
Nikolay Izhikov
[hidden email]
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: IgniteUtils#currentTimeMillis

Vladimir Ozerov
I would propose to either increase TX_TIMEOUT or sleep multiplier to make
test more reliable.

On Wed, Aug 9, 2017 at 3:32 PM, Николай Ижиков <[hidden email]>
wrote:

> Vladimir,
>
> As far as I can understand behaviour of U.currentTimeMillis() breaks
> transaction timeout test:
>
> https://issues.apache.org/jira/browse/IGNITE-5963
>
> IgniteOptimisticTxSuspendResumeTest#testTxTimeoutOnSuspend :
>
> ```
> final Transaction tx = ignite.transactions().txStart(OPTIMISTIC,
> isolation,
> TX_TIMEOUT, 0);
>
> cache.put(1, 1);
>
> Thread.sleep(TX_TIMEOUT * 2);
>
> GridTestUtils.assertThrowsWithCause(new Callable<Object>() {
>     @Override public Object call() throws Exception {
>         tx.suspend();
>
>         return null;
>     }
> }, TransactionTimeoutException.class);
> ```
> Timeout checked like that:
>
> IgniteTxAdapter#remainingTime
>
> ```
> @Override public long remainingTime() {
>     if (timeout() <= 0)
>         return 0;
>
>     long timeLeft = timeout() - (U.currentTimeMillis() - startTime());
>
>     return timeLeft <= 0 ? -1 : timeLeft;
>
> }
>
> ```
>
>
> 2017-08-09 15:26 GMT+03:00 Vladimir Ozerov <[hidden email]>:
>
> > Guys,
> >
> > Are you really suggesting change the product for newcomer needs? This is
> > not an argument for the change. Can someone explain me what is currently
> > broken from product's perspective? Yes, we can get stale values for some
> > time, we know that. Does it break something?
> >
> > On Wed, Aug 9, 2017 at 3:11 PM, Dmitry Pavlov <[hidden email]>
> > wrote:
> >
> > > +1 to Nikolay, this is very often question from newcomers.
> > > It is not clear that current* method may return outdated value from
> some
> > > moment in past.
> > >
> > > Nikolay, how long outdated value can be returned by method?
> > >
> > > ср, 9 авг. 2017 г. в 15:00, Николай Ижиков <[hidden email]>:
> > >
> > > > Vladimir,
> > > >
> > > > > There is nothing wrong with U.currentTimeMillis() at the
> > > > moment.
> > > >
> > > > I think we can't rely on the return value for time measurement.
> > > > Is it true? Is it OK for you?
> > > >
> > > > It very counterintuitive for me as newcomer.
> > > >
> > > >
> > > > 2017-08-09 14:55 GMT+03:00 Vladimir Ozerov <[hidden email]>:
> > > >
> > > > > You cannot check it with benchmarks, because behavior of this
> method
> > > will
> > > > > vary between different JVMs, OSes and hardware. It can be different
> > > even
> > > > > with the same OS depending on it's settings. Again - let's just
> avoid
> > > > > unnecessary work. There is nothing wrong with U.currentTimeMillis()
> > at
> > > > the
> > > > > moment.
> > > > >
> > > > > On Wed, Aug 9, 2017 at 2:52 PM, Dmitry Pavlov <
> [hidden email]
> > >
> > > > > wrote:
> > > > >
> > > > > > Vladimir, could we check it using benchmarks? Internet contains a
> > lot
> > > > of
> > > > > > articles about this issue. But do we know if it is still actual
> for
> > > new
> > > > > > VMs?
> > > > > >
> > > > > > ср, 9 авг. 2017 г. в 14:50, Dmitry Pavlov <[hidden email]
> >:
> > > > > >
> > > > > > > It seems System.currentTimeMillis () is now in intrinsic list.
> > This
> > > > > means
> > > > > > > on modern JVMs performance penalty will not be so significiant.
> > > > > > >
> > > > > > > Nickolay, could you please raise standalone ticket for
> > > > > > U.currentTimeMillis
> > > > > > > () ?
> > > > > > >
> > > > > > > Could you also please check if system.nanoTime /
> > > system.currentTimeMs
> > > > > can
> > > > > > > fix https://issues.apache.org/jira/browse/IGNITE-5963 When you
> > > > create
> > > > > a
> > > > > > > PR, I can start several run for Ignite Cache 6 suite to check
> if
> > > > issue
> > > > > is
> > > > > > > still reprodacible.
> > > > > > >
> > > > > > > ср, 9 авг. 2017 г. в 14:41, Yakov Zhdanov <[hidden email]
> >:
> > > > > > >
> > > > > > >> Nickolay, IgniteUtils#currentTimeMillis() is some kind of an
> > old
> > > > > > heritage.
> > > > > > >> I guess nobody remembers when this method has been
> introduced. I
> > > > agree
> > > > > > >> that
> > > > > > >> we can use System.currentTimeMillis(). I would suggest you
> file
> > a
> > > > > ticket
> > > > > > >> and replace this method calls with System.currentTimeMillis().
> > > > Sounds
> > > > > > >> good?
> > > > > > >>
> > > > > > >> As far as reliable elapsed time measurement I agree with you
> > that
> > > > > > >> nanoTime() is better here, but it is definitely not a reason
> for
> > > > > > mentioned
> > > > > > >> failure, since that test is launched in single JVM on a
> machine
> > > that
> > > > > > most
> > > > > > >> probably does not do any ntp syncs during the test to make
> > > Ignite's
> > > > > > >> timeout
> > > > > > >> machinery fail.
> > > > > > >>
> > > > > > >> Please file a ticket to switch Ignite's timeouts to nanoTime()
> > at
> > > > some
> > > > > > >> point.
> > > > > > >>
> > > > > > >> --Yakov
> > > > > > >>
> > > > > > >
> > > > > >
> > > > >
> > > >
> > > >
> > > >
> > > > --
> > > > Nikolay Izhikov
> > > > [hidden email]
> > > >
> > >
> >
>
>
>
> --
> Nikolay Izhikov
> [hidden email]
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: IgniteUtils#currentTimeMillis

dsetrakyan
In reply to this post by Николай Ижиков
On Wed, Aug 9, 2017 at 5:32 AM, Николай Ижиков <[hidden email]>
wrote:

> Vladimir,
>
> As far as I can understand behaviour of U.currentTimeMillis() breaks
> transaction timeout test:
>

So, if you change the call to System.currentTimeMillis(), the test passes?
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: IgniteUtils#currentTimeMillis

Николай Ижиков
Dmitry,

> So, if you change the call to System.currentTimeMillis(), the test passes?

Yes

> I would propose to either increase TX_TIMEOUT or sleep multiplier to make
test more reliable.

Yes, I fix test in that way.

For me the goal of this discussion is to understand reasons to keep current
method implementation.




2017-08-09 15:45 GMT+03:00 Dmitriy Setrakyan <[hidden email]>:

> On Wed, Aug 9, 2017 at 5:32 AM, Николай Ижиков <[hidden email]>
> wrote:
>
> > Vladimir,
> >
> > As far as I can understand behaviour of U.currentTimeMillis() breaks
> > transaction timeout test:
> >
>
> So, if you change the call to System.currentTimeMillis(), the test passes?
>



--
Nikolay Izhikov
[hidden email]
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: IgniteUtils#currentTimeMillis

Vladimir Ozerov
In short, the reason is avoiding potential performance problems.

On Wed, Aug 9, 2017 at 3:49 PM, Николай Ижиков <[hidden email]>
wrote:

> Dmitry,
>
> > So, if you change the call to System.currentTimeMillis(), the test
> passes?
>
> Yes
>
> > I would propose to either increase TX_TIMEOUT or sleep multiplier to make
> test more reliable.
>
> Yes, I fix test in that way.
>
> For me the goal of this discussion is to understand reasons to keep current
> method implementation.
>
>
>
>
> 2017-08-09 15:45 GMT+03:00 Dmitriy Setrakyan <[hidden email]>:
>
> > On Wed, Aug 9, 2017 at 5:32 AM, Николай Ижиков <[hidden email]>
> > wrote:
> >
> > > Vladimir,
> > >
> > > As far as I can understand behaviour of U.currentTimeMillis() breaks
> > > transaction timeout test:
> > >
> >
> > So, if you change the call to System.currentTimeMillis(), the test
> passes?
> >
>
>
>
> --
> Nikolay Izhikov
> [hidden email]
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: IgniteUtils#currentTimeMillis

Alexey Kuznetsov
In reply to this post by Николай Ижиков
Nikolay,

As far as I understand U.currentTimeMillis() should be used where time is
not a major value (metrics for example).

But in test with transaction (that you are mentioned) we should use
System.currentTimeMillis().

In general we should think about U.currentTimeMillis() and avoid it usage
in places where result of this function could be compared with some value.

Vova, make sense?

On Wed, Aug 9, 2017 at 7:49 PM, Николай Ижиков <[hidden email]>
wrote:

> Dmitry,
>
> > So, if you change the call to System.currentTimeMillis(), the test
> passes?
>
> Yes
>
> > I would propose to either increase TX_TIMEOUT or sleep multiplier to make
> test more reliable.
>
> Yes, I fix test in that way.
>
> For me the goal of this discussion is to understand reasons to keep current
> method implementation.
>
>
>
>
> 2017-08-09 15:45 GMT+03:00 Dmitriy Setrakyan <[hidden email]>:
>
> > On Wed, Aug 9, 2017 at 5:32 AM, Николай Ижиков <[hidden email]>
> > wrote:
> >
> > > Vladimir,
> > >
> > > As far as I can understand behaviour of U.currentTimeMillis() breaks
> > > transaction timeout test:
> > >
> >
> > So, if you change the call to System.currentTimeMillis(), the test
> passes?
> >
>
>
>
> --
> Nikolay Izhikov
> [hidden email]
>



--
Alexey Kuznetsov
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: IgniteUtils#currentTimeMillis

yzhdanov
In reply to this post by Николай Ижиков
As Dmitry P mentioned System.currentTimeMillis() is JVM intrinsic.
Moreover, there is a daemon thread that updates the internal value which
will not be needed after the change.

If we remove U.currentTimeMillis() code will become more clear and
consistent. Why we think that we can implement this particular timer better
than JVM developers?

Vladmir, can you please share a benchmark that will show the problems? If
it will then it will be a strong argument to keep the current
implementation.

--Yakov
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: IgniteUtils#currentTimeMillis

Evgeniy Stanilovskiy
I assume that Vladimir mention this mesurements:
https://shipilev.net/blog/2014/nanotrusting-nanotime/
can we simple measure with JMH x86 and arm our realization vs system call?

> As Dmitry P mentioned System.currentTimeMillis() is JVM intrinsic.
> Moreover, there is a daemon thread that updates the internal value which
> will not be needed after the change.
>
> If we remove U.currentTimeMillis() code will become more clear and
> consistent. Why we think that we can implement this particular timer  
> better
> than JVM developers?
>
> Vladmir, can you please share a benchmark that will show the problems? If
> it will then it will be a strong argument to keep the current
> implementation.
>
> --Yakov
12
Loading...