Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove dummy java.time implementation from core repo. #2087

Merged

Conversation

WojciechMazur
Copy link
Contributor

This PR continues the topic of removal of dummy implementations from SN core repository started in #2079, by removal of java.time package, or exactly Instant and Duration.
It adopts only usage of Instant of core repo inside FileTime to be not directly dependent from Instant.

After merging this PR SN users should use a custom implementation of java.time, eg. scala-java-time. Alternatively, dummy implementation should be moved to a separate repo and published.

This PR needs to be rebased on top of merged #2079, as it bootstraps javalib-ext-dummies and testsExt projects.

Copy link
Collaborator

@sjrd sjrd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we still need java.time.Duration and java.time.temporal.TemporalAmount at all? Can't we entirely remove them, instead of moving them to javalib-ext-dummies?

Comment on lines 6 to 7
final class FileTime private (private val epochMillis: Long,
private val nanos: Int)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we store epochMillis and nanos, we will not be able to represent all the values that FileTime is supposed to hold. From looking at the JavaDoc, it's not clear, but I got the intuition that the canonical representation is a value: Long and unit: TimeUnit. Experimentation in a REPL suggests that my intuition is correct:

scala> import java.nio.file.attribute.FileTime
import java.nio.file.attribute.FileTime

scala> import java.util.concurrent.TimeUnit
import java.util.concurrent.TimeUnit

scala> FileTime.from(Long.MaxValue - 10L, TimeUnit.DAYS)
val res10: java.nio.file.attribute.FileTime = 219250468-12-04T15:30:07Z

scala> res10.to(TimeUnit.DAYS)
val res11: Long = 9223372036854775797 // exact number of days kept

scala> res10.to(TimeUnit.MILLISECONDS)
val res12: Long = 9223372036854775807 // overflowed to Long.MaxValue

scala> FileTime.from(Long.MaxValue - 10L, TimeUnit.NANOSECONDS)
val res13: java.nio.file.attribute.FileTime = 2262-04-11T23:47:16.854775797Z

scala> res13.to(TimeUnit.DAYS)
val res15: Long = 106751

scala> res13.to(TimeUnit.NANOSECONDS)
val res16: Long = 9223372036854775797 // exact number of nanoseconds kept

Therefore, I strongly believe that we should store value: Long and unit: TimeUnit, and adapt everything in terms of that canonical representation.

The above REPL session can be a good source of additional test cases.

Comment on lines 49 to 51
val secondsAsNanos =
TimeUnit.NANOSECONDS.convert(instant.getEpochSecond, TimeUnit.SECONDS)
new FileTime(secondsAsNanos + instant.getNano, TimeUnit.NANOSECONDS)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will not work for large Instants that cannot be represented in a Long value of nanoseconds. We must start with the Seconds, and only keep more precision (in milliseconds or nanoseconds) if the seconds are <= Long.MaxValue / 1000 (or 1000000 for nanoseconds).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Guess it's outdated, as now, after conversion to (epochDays, dayNanos) we are having a greater range than Instant. Now we need to make sure conversion toInstant does not overexcites Instant limits. To check this additional tests were added to FileTimeExtTest

Instant.ofEpochSecond(timestampAsSeconds, nanosAdjustment))
}

@Test def fromInstantMaxPrecision(): Unit = {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to me that this test is overengineered, to the point where it is difficult to understand what it tests, and how. Consider using "dumb" tests instead, using known input/output pairs and relying on FileTime.equals:

assertEquals(FileTime.from(1L, DAYS), FileTime.from(Instant.ofEpochSecond(86400L, 0)))

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Playing more with that, I think I was wrong before. Storing value and unit is not enough. Apparently FileTime has strictly more precision than Instant. I could not find an Instant for which FileTime.from(instant).toInstant() == instant was false.

So, in fact, it seems the reality is much simpler: a TimeUnit stores a pair of days: Long and nanos: Long. Probably with nanos capped at 86400000000000L - 1L (so working with a floorMod(_, 86400000000000L)), the number of nanoseconds in a day, to make things easier.

That would make the implementation of all methods, in particular equals and compareTo, much easier.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually FileTime.from(instant).toInstant() eq instant also would be true. I belive JDK stores original Instant or if it's missing creates new one based on other parameters.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, but we can't do that (because a field of type Instant would not link when we don't have Instant in the codebase), and there's certainly no requirement to offer eq. We should be able to guarantee ==, though.

Copy link
Collaborator

@sjrd sjrd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, except two tiny details below.

@WojciechMazur WojciechMazur deleted the refactor_remove_ju_instant branch February 15, 2021 09:09
ekrich pushed a commit to ekrich/scala-native that referenced this pull request May 21, 2021
)

This PR continues the topic of removal of dummy javalib implementations from SN core repository started in scala-native#2079, by removal of `java.time` package.
It adopts only usage of `Instant` in core repo inside `FileTime` to be not directly dependent from Instant.

After merging this PR SN users should use a custom implementation of java.time, eg. scala-java-time. Alternatively, dummy implementation should be moved to a separate repo and published.
WojciechMazur added a commit to WojciechMazur/scala-native that referenced this pull request Aug 25, 2021
)

This PR continues the topic of removal of dummy javalib implementations from SN core repository started in scala-native#2079, by removal of `java.time` package.
It adopts only usage of `Instant` in core repo inside `FileTime` to be not directly dependent from Instant.

After merging this PR SN users should use a custom implementation of java.time, eg. scala-java-time. Alternatively, dummy implementation should be moved to a separate repo and published.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants