You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@lucenenet.apache.org by GitBox <gi...@apache.org> on 2021/06/03 19:11:40 UTC
[GitHub] [lucenenet] Shazwazza opened a new issue #492: Time calculation issue in ControlledRealTimeReopenThread
Shazwazza opened a new issue #492:
URL: https://github.com/apache/lucenenet/issues/492
I have been trying to figure out why ControlledRealTimeReopenThread.Run is in a constant waiting state. It is blocking at the [`reopenCond.WaitOne`](https://github.com/apache/lucenenet/blob/b03b93683c4d3a7128e54692525cb7d1315637f5/src/Lucene.Net/Search/ControlledRealTimeReopenThread.cs#L244) call.
I ended up copying all the code into LinqPad that determines the amount of wait time and it seems to me that these calculations aren't correct and it ends up making the wait times very long.
Here's some code that does the same calculations as in ControlledRealTimeReopenThread to determine how long the timeout will be in the WaitOne call:
```cs
// These are the values that would be passed into the ctor of ControlledRealTimeReopenThread
var targetMaxStaleSec = 5.0; // this should just wait a max of 5 seconds
var targetMinStaleSec = 0.1; // this should wait a min of 100 milliseconds
// Then the calculations go like this:
var targetMaxStaleNS = (long)(1000000000 * targetMaxStaleSec); // = 5000000000
var targetMinStaleNS = (long)(1000000000 * targetMinStaleSec); // = 100000000
var hasWaiting = false;
long lastReopenStartNS = DateTime.UtcNow.Ticks * 100;
long nextReopenStartNS = lastReopenStartNS + (hasWaiting ? targetMinStaleNS : targetMaxStaleNS);
long sleepNS = nextReopenStartNS - Time.NanoTime(); // = 25204999858200
var wait = TimeSpan.FromMilliseconds(sleepNS / Time.MILLISECONDS_PER_NANOSECOND);
// This is :
// 07:00:04.9990000
// which is more than 7 hours !
// Used for the Time methods above
internal static class Time
{
public const long MILLISECONDS_PER_NANOSECOND = 1000000;
public const long TICKS_PER_NANOSECOND = 100;
public static long NanoTime()
{
return DateTime.Now.Ticks * TICKS_PER_NANOSECOND;
// LUCENENET TODO: Change to
// return (Stopwatch.GetTimestamp() / Stopwatch.Frequency) * 1000000000;
// for better accuracy that is not affected by the system clock
}
}
```
The issue is: `DateTime.UtcNow.Ticks * 100;`
In the Time.NanoTime() method, this calculation is:
`DateTime.Now.Ticks * TICKS_PER_NANOSECOND;`
The difference being `UtcNow` vs `Now`.
Shouldn't this line: https://github.com/apache/lucenenet/blob/b03b93683c4d3a7128e54692525cb7d1315637f5/src/Lucene.Net/Search/ControlledRealTimeReopenThread.cs#L228 use `Time.NanoTime();` just like this line does? https://github.com/apache/lucenenet/blob/b03b93683c4d3a7128e54692525cb7d1315637f5/src/Lucene.Net/Search/ControlledRealTimeReopenThread.cs#L257
It also seems like all of this logic could be hugely simplified for just converting the different of "now" and how long to wait based on the supplied seconds, but I could be overlooking something.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [lucenenet] NightOwl888 commented on issue #492: Time calculation issue in ControlledRealTimeReopenThread
Posted by GitBox <gi...@apache.org>.
NightOwl888 commented on issue #492:
URL: https://github.com/apache/lucenenet/issues/492#issuecomment-854248770
Thanks Shannon.
This goes to the heart of one of the issues that I have been meaning to post for awhile now - namely, that because J2N didn't exist when we ported Lucene.NET, we need to go back and review all of the code that depends on system timing. I created a different implementation of [`Time.NanoTime()`](https://github.com/NightOwl888/J2N/blob/c7b578fd9b881f98b4f0c99331a7ec760903e1c9/src/J2N/Time.cs#L17-L38) in J2N than the implementation in Lucene.Net.Support, but being hyper-aware of the fact that the concurrency code may break by swapping the implementation, migrating to this new method has been put this off until a proper review can be done.
During writing tests and benchmarks, I noticed that `Time.NanoTime()` isn't as accurate as using a `Stopwatch` instance, so this needs another pass to ensure that implementation is solid before migrating to it. We might need to use some `AsyncLocal` approach to fix it so it uses 1 `Timestamp` instance per parallel process rather than using the static `Stopwatch.GetTimestamp()`.
This is of course compounded by the facts that:
1. The code was inconsistently translated from Java between `DateTime.Now`, `DateTime.UtcNow`,`Time.NanoTime()`, and `Time.CurrentTimeMilliseconds()` thoughout the project.
2. In Java, they are [supposed to be using `System.nanoTime()` for the most accurate timing](https://stackoverflow.com/a/351571), but they inconsistently switch between `System.nanoTime()` and `System.currentTimeMillis()` throughout the project.
We could fix this by analyzing each use of timing case-by-case and switching them all to using individual `Stopwatch` instances. I don't think that is so bad if the code is in isolation, but we still should fix `J2N.Time.NanoTime()` and `J2N.Time.CurrentTimeMilliseconds()` so they work right so we can also easily translate these lines during future porting efforts without making the same mistakes again.
In the latter case, we may be able to assume that the code works well enough to use `CurrentTimeMilliseconds()`, but it means we need 2 methods instead of just `NanoTime()`.
> `System.currentTimeMillis()` in Java returns the [number of milliseconds that elapsed since January 1, 1970](https://docs.oracle.com/javase/7/docs/api/java/lang/System.html#currentTimeMillis()). So we should fix the J2N implementation to do the same to ensure something isn't thrown off by this. Do note that neither of these implementations are based off of the Java code, so we might want to look there and at the .NET implementation at a low level to ensure there are no additional timing gaps to look out for.
> Tip: It may also help to look at the [NodaTime project](https://github.com/nodatime/nodatime).
Whatever the case, we should aim to factor out the `Time` class in Lucene.Net.Support and use a common timing method everywhere (from J2N) except in special isolated cases. And ultimately use `Stopwatch` instead of `DateTime.Now` or `DateTime.UtcNow` consistently for timing code.
> NOTE: If you wish to fix this in J2N, please base your work off of the [release/v2.0](https://github.com/NightOwl888/J2N/tree/release/v2.0) branch and target that same branch when submitting the PR.
>
> Note also that if you don't have Xamarin setup on your dev machine, you can workaround the issue by unloading all of the Xamarin projects and tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [lucenenet] NightOwl888 closed issue #492: Time calculation issue in ControlledRealTimeReopenThread
Posted by GitBox <gi...@apache.org>.
NightOwl888 closed issue #492:
URL: https://github.com/apache/lucenenet/issues/492
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [lucenenet] NightOwl888 edited a comment on issue #492: Time calculation issue in ControlledRealTimeReopenThread
Posted by GitBox <gi...@apache.org>.
NightOwl888 edited a comment on issue #492:
URL: https://github.com/apache/lucenenet/issues/492#issuecomment-854248770
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [lucenenet] NightOwl888 commented on issue #492: Time calculation issue in ControlledRealTimeReopenThread
Posted by GitBox <gi...@apache.org>.
NightOwl888 commented on issue #492:
URL: https://github.com/apache/lucenenet/issues/492#issuecomment-854248770
Thanks Shannon.
This goes to the heart of one of the issues that I have been meaning to post for awhile now - namely, that because J2N didn't exist when we ported Lucene.NET, we need to go back and review all of the code that depends on system timing. I created a different implementation of [`Time.NanoTime()`](https://github.com/NightOwl888/J2N/blob/c7b578fd9b881f98b4f0c99331a7ec760903e1c9/src/J2N/Time.cs#L17-L38) in J2N than the implementation in Lucene.Net.Support, but being hyper-aware of the fact that the concurrency code may break by swapping the implementation, migrating to this new method has been put this off until a proper review can be done.
During writing tests and benchmarks, I noticed that `Time.NanoTime()` isn't as accurate as using a `Stopwatch` instance, so this needs another pass to ensure that implementation is solid before migrating to it. We might need to use some `AsyncLocal` approach to fix it so it uses 1 `Timestamp` instance per parallel process rather than using the static `Stopwatch.GetTimestamp()`.
This is of course compounded by the facts that:
1. The code was inconsistently translated from Java between `DateTime.Now`, `DateTime.UtcNow`,`Time.NanoTime()`, and `Time.CurrentTimeMilliseconds()` thoughout the project.
2. In Java, they are [supposed to be using `System.nanoTime()` for the most accurate timing](https://stackoverflow.com/a/351571), but they inconsistently switch between `System.nanoTime()` and `System.currentTimeMillis()` throughout the project.
We could fix this by analyzing each use of timing case-by-case and switching them all to using individual `Stopwatch` instances. I don't think that is so bad if the code is in isolation, but we still should fix `J2N.Time.NanoTime()` and `J2N.Time.CurrentTimeMilliseconds()` so they work right so we can also easily translate these lines during future porting efforts without making the same mistakes again.
In the latter case, we may be able to assume that the code works well enough to use `CurrentTimeMilliseconds()`, but it means we need 2 methods instead of just `NanoTime()`.
> `System.currentTimeMillis()` in Java returns the [number of milliseconds that elapsed since January 1, 1970](https://docs.oracle.com/javase/7/docs/api/java/lang/System.html#currentTimeMillis()). So we should fix the J2N implementation to do the same to ensure something isn't thrown off by this. Do note that neither of these implementations are based off of the Java code, so we might want to look there and at the .NET implementation at a low level to ensure there are no additional timing gaps to look out for.
> Tip: It may also help to look at the [NodaTime project](https://github.com/nodatime/nodatime).
Whatever the case, we should aim to factor out the `Time` class in Lucene.Net.Support and use a common timing method everywhere (from J2N) except in special isolated cases. And ultimately use `Stopwatch` instead of `DateTime.Now` or `DateTime.UtcNow` consistently for timing code.
> NOTE: If you wish to fix this in J2N, please base your work off of the [release/v2.0](https://github.com/NightOwl888/J2N/tree/release/v2.0) branch and target that same branch when submitting the PR.
>
> Note also that if you don't have Xamarin setup on your dev machine, you can workaround the issue by unloading all of the Xamarin projects and tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org