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.

Sign up for GitHub

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

Jump to bottom

0.5.x: use higher resolution file stat fields #3049

Merged
merged 7 commits into from
Jan 10, 2023
Merged

0.5.x: use higher resolution file stat fields #3049

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
55c0b58
use high resolution file stat fields
i10416 Dec 25, 2022
b38df8b
fix: remove wrong type declaration
i10416 Dec 28, 2022
1c22d84
fix: put scalanative timespec back
i10416 Dec 28, 2022
99aad5d
update: use statOps to access fields
i10416 Dec 29, 2022
7ec4eac
minor fix: address code review
i10416 Jan 3, 2023
174e9df
add test and inline docs
i10416 Jan 4, 2023
4e2096f
docs: mention breaking changes in next release note
i10416 Jan 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 3 additions & 2 deletions javalib/src/main/scala/java/io/File.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -415,7 +415,7 @@ class File(_path: String) extends Serializable with Comparable[File] {
} else {
val buf = alloc[stat.stat]()
if (stat.stat(toCString(path), buf) == 0) {
buf._8 * 1000L
buf._8._1 * 1000L
i10416 marked this conversation as resolved.
Show resolved Hide resolved
} else {
0L
}
Expand Down Expand Up @@ -473,7 +473,8 @@ class File(_path: String) extends Serializable with Comparable[File] {
val statbuf = alloc[stat.stat]()
if (stat.stat(toCString(path), statbuf) == 0) {
val timebuf = alloc[utime.utimbuf]()
timebuf._1 = statbuf._8
import scala.scalanative.posix.sys.statOps.statOps
timebuf._1 = statbuf.st_mtime
timebuf._2 = time.toSize / 1000
utime.utime(toCString(path), timebuf) == 0
} else {
Expand Down
19 changes: 10 additions & 9 deletions javalib/src/main/scala/java/nio/file/attribute/PosixFileAttributeViewImpl.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,10 +35,10 @@ final class PosixFileAttributeViewImpl(path: Path, options: Array[LinkOption])
val buf = alloc[utime.utimbuf]()
buf._1 =
if (lastAccessTime != null) lastAccessTime.to(TimeUnit.SECONDS).toSize
else sb._7
else sb._7._1
buf._2 =
if (lastModifiedTime != null) lastModifiedTime.to(TimeUnit.SECONDS).toSize
else sb._8
else sb._8._1
// createTime is ignored: No posix-y way to set it.
if (utime.utime(toCString(path.toString), buf) != 0)
throwIOException()
Expand Down Expand Up @@ -98,15 +98,16 @@ final class PosixFileAttributeViewImpl(path: Path, options: Array[LinkOption])

Zone { implicit z =>
val buf = getStat()
import scala.scalanative.posix.sys.statOps.statOps

// Copy only what is referenced below. Save runtime cycles.
st_ino = buf._3
st_uid = buf._4
st_gid = buf._5
st_size = buf._6
st_atime = buf._7
st_mtime = buf._8
st_mode = buf._13
st_ino = buf.st_ino
st_uid = buf.st_uid
st_gid = buf.st_gid
st_size = buf.st_size
st_atime = buf.st_atime
st_mtime = buf.st_mtime
st_mode = buf.st_mode
i10416 marked this conversation as resolved.
Show resolved Hide resolved
}

override def fileKey() = st_ino.asInstanceOf[Object]
Expand Down
21 changes: 15 additions & 6 deletions posixlib/src/main/resources/scala-native/sys/stat.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,9 @@ struct scalanative_stat {
length in bytes. For a typed memory object,
the length in bytes. For other file types,
the use of this field is unspecified. */
scalanative_time_t _st_atime; /** Time of last access. */
scalanative_time_t _st_mtime; /** Time of last data modification. */
scalanative_time_t _st_ctime; /** Time of last status change. */
scalanative_timespec st_atim; /** Time of last access. */
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do not want to make things so difficult that nothing ever gets done.
I suspect that it is possible to introduce this PR in a way which causes
less breakage to existing users.

Hmm, how can we introduce public api changes in a less breaking way?
Is there way better than deprecation warning?

Copy link
Contributor

Choose a reason for hiding this comment

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

SN 0.5.0 is advertised/envisioned as requiring a re-build from sources and possibly requiring source changes.
That gives us some degrees of freedom to do things
a 'right' way.

So;

  1. This PR should be marked in base topic as "0.5.0"
  2. There should be an entry in `docs/changelog/0.5.0.md"
    briefly describing the required source change and
    pointing to an entry in javalib.rst
  3. There should be a section in `docs/lib/javalib.rst'
    giving a fuller description and examples of the
    required source change (i.e. change foo._8 to
    foo.a_time to use seconds, foo.a_tim to use timespec).

Once the dust settles on this PR, I can either help you
set up the Sphinx environment on your system to build .rst (use "Makefile html" not 'scripts/makedoc") and .md files or do a followup PR for you to review.

Copy link
Contributor

Choose a reason for hiding this comment

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

stat.c scalanative_stat_init() is exactly the kind of
glue code I have been trying to expunge from Scala Native.
After a bunch of network code, I had hoped to get to this one before it changed.

I think this section of code is correct/OK and will try
to queue up a follow on PR to optimize (probably to
optimize for Linux passthru with appropriate _Static guards.)
stat is supposed to be fast (but must always be correct).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There should be an entry in `docs/changelog/0.5.0.md" briefly describing the required source change and pointing to an entry in javalib.rst

There should be a section in `docs/lib/javalib.rst' giving a fuller description and examples of the required source change

Though I changed java File internal, it doesn't leak posixlib implementation for users.

In terms of source change, I think you mean docs/lib/posixlib, don't you?

Of course, we should mention that java File metadata will support nanosecond resolution in the future(probably in 0.5.x?).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added docs here
174e9df
4e2096f

scalanative_timespec st_mtim; /** Time of last data modification. */
scalanative_timespec st_ctim; /** Time of last status change. */
scalanative_blkcnt_t st_blocks; /** Number of blocks allocated for this
object. */
scalanative_blksize_t st_blksize; /** A file system-specific preferred I/O
Expand All @@ -42,9 +42,18 @@ void scalanative_stat_init(struct stat *stat,
my_stat->st_uid = stat->st_uid;
my_stat->st_gid = stat->st_gid;
my_stat->st_size = stat->st_size;
my_stat->_st_atime = stat->st_atime;
my_stat->_st_mtime = stat->st_mtime;
my_stat->_st_ctime = stat->st_ctime;
// see https://linux.die.net/man/2/stat
#if defined(_BSD_SOURCE) || defined(_SVID_SOURCE) || \
defined(_POSIX_C_SOURCE) && _POSIX_C_SOURCE >= 200809L || \
defined(_XOPEN_SOURCE) && _XOPEN_SOURCE >= 700
my_stat->st_atim = stat->st_atim;
my_stat->st_mtim = stat->st_mtim;
my_stat->st_ctim = stat->st_ctim;
#else
i10416 marked this conversation as resolved.
Show resolved Hide resolved
my_stat->st_atim = stat->st_atimespec;
i10416 marked this conversation as resolved.
Show resolved Hide resolved
my_stat->st_mtim = stat->st_mtimespec;
my_stat->st_ctim = stat->st_ctimespec;
#endif
my_stat->st_blksize = stat->st_blksize;
my_stat->st_blocks = stat->st_blocks;
my_stat->st_nlink = stat->st_nlink;
Expand Down
1 change: 1 addition & 0 deletions posixlib/src/main/resources/scala-native/types.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ typedef unsigned int scalanative_uid_t;
typedef unsigned int scalanative_gid_t;
typedef long long scalanative_off_t;
typedef long int scalanative_time_t;
typedef struct timespec scalanative_timespec;
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this line is improper. POSIX 2018 (and many before, I believe) specify timespec as being defined
in time.h, not types.h ( probably time was well established before types.h was specified.)

Some else in this PR there is probably an import which
needs to change or happen.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for feedback! I will address this later.

Copy link
Contributor Author

@i10416 i10416 Dec 28, 2022 •
edited

Choose a reason for hiding this comment

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

Hmm, could you tell me why scalanative_timespec introduced here?(link below) If we use timespec from time.h, I don't think scalanative_timespec is needed. Do you mean I should create another time.h file and put scalanative_timespec in it?

https://github.com/scala-native/scala-native/pull/2713/files#diff-9db9f7da0930f16af30649420a8fd8857d1b9f1e07366491539a6d5ef0c0eeccR36

By the way, reading the comment in sys/stat.c, I guess src/main/resources/scala-native/types.h intentionally violates spec for portability.

// We don't use the "standard" types such as `dev_t` for instance
// because these have different sizes on eg. Linux and OSX. We use the
// smallest type that can hold all the possible values for the different
// systems.
struct scalanative_stat {
    scalanative_dev_t st_dev;     /** Device ID of device containing file. */
    scalanative_dev_t st_rdev;    /** Device ID (if file is character or block
                                      special). */
    scalanative_ino_t st_ino;     /** File serial number. */

These types above were introduced by Martin Duhem at 2017. Is this statement "these have different sizes on eg. Linux and OSX" not the case for recent versions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

NOTE: I'm not in a hurry😉 Have a nice holiday!

Copy link
Contributor

Choose a reason for hiding this comment

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

i10416 You ask good questions. Let me finish my
highest level concern, so the PR can progress, and return to the trickery at play here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Quick answer to 1st question. I'll get back to second later (tomorrow?)

Lines 92-102 check that Scala Natives scala timespec can be passed thru
to C with no "copy-in/copy-out" code.

// struct timespec
_Static_assert(sizeof(struct scalanative_timespec) == sizeof(struct timespec),
               "Unexpected size: struct timespec");

_Static_assert(offsetof(struct scalanative_timespec, tv_sec) ==
                   offsetof(struct timespec, tv_sec),
               "offset mismatch: timespec.tv_sec");
// and so on.

From software engineering view, it is unfortunate that the C declaration of scalanative_timespec
is hand edited and hand synchronized with the corresponding Scala struct. The test would
be stronger if the former where generated from the latter. For now, the costly safeguards
are education & review. In someplaces there are "If you change .scala, also change .c" comments.

Copy link
Contributor Author

@i10416 i10416 Jan 3, 2023 •
edited

Choose a reason for hiding this comment

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

Thank you!
So,

  1. there exist scalanative_timespec and other scalanative_* structs so that we can verify Scala struct is the same format as c struct.

From software engineering view, it is unfortunate that the C declaration of scalanative_timespec
is hand edited and hand synchronized with the corresponding Scala struct. The test would
be stronger if the former where generated from the latter. For now, the costly safeguards
are education & review. In someplaces there are "If you change .scala, also change .c" comments.

  1. I guess you mean it is better to put typedef struct timespec scalanative_timespec; in time.h for future maintenance, though the original PR Fix #1610: add nanosecond resolution in javalib FileTime & posixlib sys/stat; requires previous PRs #1635 puts it in types.h.

Am I right?

typedef long long scalanative_blkcnt_t;
typedef long scalanative_blksize_t;
typedef unsigned long scalanative_nlink_t;
Expand Down
43 changes: 40 additions & 3 deletions posixlib/src/main/scala/scala/scalanative/posix/sys/stat.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,9 +51,9 @@ object stat {
uid_t, // st_uid
gid_t, // st_gid
off_t, // st_size
time_t, // st_atime
time_t, // st_mtime
time_t, // st_ctime
timespec, // st_atim or st_atimespec
timespec, // st_mtim or st_mtimespec
timespec, // st_ctim or st_ctimespec
blkcnt_t, // st_blocks
blksize_t, // st_blksize
nlink_t, // st_nlink
Expand Down Expand Up @@ -132,3 +132,40 @@ object stat {
def S_IXOTH: mode_t = extern

}

object statOps {
i10416 marked this conversation as resolved.
Show resolved Hide resolved
implicit class statOps(val c: Ptr[stat.stat]) extends AnyVal {
def st_dev: stat.dev_t = c._1
def st_dev_=(dev_t: stat.dev_t): Unit = c._1 = dev_t
def st_rdev: stat.dev_t = c._2
def st_rdev_=(dev_t: stat.dev_t): Unit = c._2 = dev_t
def st_ino: stat.ino_t = c._3
def st_ino_=(ino_t: stat.ino_t): Unit = c._3 = ino_t
def st_uid: uid_t = c._4
def st_uid_=(uid: uid_t): Unit = c._4 = uid
def st_gid: gid_t = c._5
def st_gid_=(gid: gid_t): Unit = c._5 = gid
def st_size: stat.off_t = c._6
def st_size_=(size: stat.off_t): Unit = c._6 = size
def st_atim: timespec = c._7
def st_atim_=(t: timespec): Unit = c._7 = t
def st_atime: time_t = c._7._1
def st_atime_=(t: time_t): Unit = c._7._1 = t
def st_mtim: timespec = c._8
def st_mtim_=(t: timespec): Unit = c._8 = t
def st_mtime_=(t: time_t): Unit = c._8._1 = t
def st_mtime: time_t = c._8._1
def st_ctim: timespec = c._9
def st_ctim_=(t: timespec): Unit = c._9 = t
def st_ctime: time_t = c._9._1
def st_ctime_=(t: time_t): Unit = c._9._1 = t
def st_blocks: stat.blkcnt_t = c._10
def st_blocks_=(blc: stat.blkcnt_t): Unit = c._10 = blc
def st_blksize: blksize_t = c._11
def st_blksize_=(blcsize: blksize_t): Unit = c._11 = blcsize
def st_nlink: nlink_t = c._12
def st_nlink_=(nlink: nlink_t): Unit = c._12 = nlink
def st_mode: mode_t = c._13
def st_mode_=(mode: mode_t): Unit = c._13 = mode
}
}