Merged
Conversation
Codecov Report
@@ Coverage Diff @@
## master #554 +/- ##
=========================================
Coverage 73.87% 73.87%
Complexity 2968 2968
=========================================
Files 507 507
Lines 12004 12006 +2
Branches 672 672
=========================================
+ Hits 8868 8870 +2
Misses 2909 2909
Partials 227 227 |
Contributor
Author
|
@armiol PTAL. |
armiol
requested changes
Aug 6, 2020
Collaborator
armiol
left a comment
armiol
left a comment
There was a problem hiding this comment.
@nick-dolgiy as discussed, let's bring up the performance by using System.currentTimeMillis() directly instead of using the same data through the Instant API and transformation logic.
Also, we need to document the changes. In particular, describe the context of the problem we solve and reference the JDK issue.
Contributor
Author
|
@armiol PTAL again |
armiol
requested changes
Aug 11, 2020
Collaborator
armiol
left a comment
armiol
left a comment
There was a problem hiding this comment.
@nick-dolgiy LGTM in general. Please see my comments.
| * <p>Starting from Java 9 the precision of time may differ from platform to platform and | ||
| * depends on the underlying clock used by the JVM, the | ||
| * <a href='https://bugs.openjdk.java.net/browse/JDK-8068730'>issue</a> on this in the | ||
| * JDK bug system. To be consistent on different platforms and use {@link IncrementalNanos} |
| @VisibleForTesting | ||
| static class SystemTimeProvider implements Provider { | ||
|
|
||
| public static final int NANOSECONDS_IN_MILLISECOND = 1_000_000; |
Collaborator
There was a problem hiding this comment.
Let's hide this constant.
Also, please look around. I would think there is a constant like this somewhere, already defined for you.
| * <p>The nanosecond value is reset for each new passed {@link Instant} value. | ||
| * <p>The nanosecond value is reset for each new passed {@code seconds} and {@code nanos} | ||
| * values. | ||
| * That allows to receive {@code 1 000} distinct time values per millisecond. |
Collaborator
There was a problem hiding this comment.
Please move this line to the previous one.
armiol
reviewed
Aug 11, 2020
| /** | ||
| * {@inheritDoc} | ||
| * | ||
| * <p>Starting from Java 9 the precision of time may differ from platform to platform and |
Collaborator
There was a problem hiding this comment.
@nick-dolgiy
Please see my suggested edit below.
<p>Starting from Java 9 the precision of time may differ from platform to platform and depends
on the underlying clock used by the JVM. See the corresponding
<a href='https://bugs.openjdk.java.net/browse/JDK-8068730'>issue</a> on this matter.
<p>In order to provide consistent behavior on different platforms and avoid the overflow
of the emulated nanosecond value, this method intentionally uses the system time
with the millisecond precision. Therefore even if the system clock may offer more precise values,
the {@code System.currentTimeMillis()} is used as a base for the returned values.
Contributor
Author
|
@armiol PTAL again. |
armiol
approved these changes
Aug 12, 2020
yuri-sergiichuk
added a commit
to SpineEventEngine/chat-bot
that referenced
this pull request
Aug 14, 2020
The new `base` version has the Java time fix related directly to Java9+ (see SpineEventEngine/base-libraries#554 for details). We must force dependencies because the `Bootstrap` plugin configured Spine dependencies for us with the versions pre-configured in the plugin itself.
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
If we run a spine-based project under the Java 11 platform we randomly may encounter the issue when
Time.currentTime()returns an invalid timestamp.The issue is that
Timeuses theSystemTimeProviderby default, which rely on the fact thatInstant.now()returns time in millisecond precision, that was true for Java 8. TheSystemTimeProvideremulates nanoseconds in order to split in time events that happened in the same millisecond, but if we have time with increased precision it leads us to the situation when the resulting amount of nanoseconds with emulation is bigger than allowed. This happens because starting from Java 9Instant.now()may return time with increased precision if the underlying clock supports it. This issue also described in #553.In this PR I made the
SystemTimeProviderexplicitly truncate time to the millisecond precision.