WasmFS JS API: Implement utime #19561

jameshu15869 · 2023-06-08T20:05:47Z

This PR implements utime for the JS API. I didn't find any tests for utime in the test directory, so added tests to test_fs_js_api.

I also attempted to add support for atime, mtime, and ctime nanoseconds in WasmFS. I am still a little new to C++ programming, so please let me know about any improvements I need to make to the implementation. One thing I wasn't sure about was how to get the nanoseconds - is using #include <time.h> the best way?

sbc100 · 2023-06-08T20:25:03Z

test/fs/test_fs_js_api.c

        assert(ex.name === 'ErrnoError' && ex.errno == 8 /* EBADF */);
    );

+    /********** test FS.utime() **********/


I wonder if its worth splitting this function up. (That way the ones that are JS-only can also be written using EM_JS instead of EM_ASM)

(Can be a followup PR for that)

Just to clarify, do you mean splitting up by moving each test section into an EM_JS function? Or should the code be split some other way?

Yup thats what I meant. Each section could be its own function (either a regular C function or an EM_JS function)

src/library_wasmfs.js

kripken · 2023-06-08T20:42:39Z

system/lib/wasmfs/file.h

+
+    struct timespec ts;
+    clock_gettime(CLOCK_REALTIME, &ts);
+    atime_nsec = mtime_nsec = ctime_nsec = ts.tv_nsec;


It is more comprehensive to support nanoseconds as well, and I think this the right way to do it (but we could also get the non-nanosecond times from this as well, to save the previous time(..) call).

However, I think we need to consider if we want to add ns. Technically a POSIX-compliant FS doesn't need that (I don't think POSIX guarantees a particular precision). And on the Web the timing info we get from Date.now() etc. may have limited resolution anyhow.

And, adding ns support has a cost in terms of code size. So overall I lean towards not adding it, and just documenting that decision.

@tlively what do you think?

+1 to removing the previous time call.

I would lean toward keeping this. I don't think it's too much of a stretch to imagine an Emscripten application that would want at least millisecond precision in the time stamps. For example, anything doing something similar to make to figure out when file changes mean something needs to be recomputed would need more than second precision.

I also expect that the code size increase would be fairly small.

tlively · 2023-06-09T15:35:19Z

system/lib/wasmfs/file.h

  time_t getATime() { return file->atime; }
+  long getATimeNs() { return file->atime_nsec; }
  void setATime(time_t time) { file->atime = time; }
+  void setATimeNs(long time) { file->atime_nsec = time; }


Maybe we can combine these getters and setters? I imagine that usually the seconds and nanoseconds will always be set or retrieved at the same time.

I think I was worried about compatibility with any code that used the old getters/setters by getting just the seconds with time(NULL). Would it be worth switching all of them to use a combined getter/setter? If so, would it be better to pass in the seconds and nanoseconds together as a pointer to a struct or two separate time_t and long values respectively?

Yes, switching all the use sites would be fine. There shouldn't be many of them, and this is private API so this wouldn't be a breaking change. I would implement it by passing the timeval struct by value (rather than by pointer) since it is so small.

tlively · 2023-06-12T14:26:07Z

system/lib/wasmfs/file.h

+    ts.tv_sec = file->ctime;
+    ts.tv_nsec = file->ctime_nsec;


If we stored the times on the file as timespecs, then this could just be return file->ctime;.

…9/emscripten into library-wasmfs-utime

sbc100 · 2023-06-12T19:29:36Z

test/fs/test_fs_js_api.c

+    test_fs_utime();
+    test_fs_rmdir();
+    test_fs_close();
+    test_fs_mknod();


Nice, I think this makes the test much easier to read.

jameshu15869 · 2023-06-12T19:54:47Z

Currently my implementation has failed test_metadce_files_wasmfs and test_metadce_cxx_wasmfs. I was able to reproduce the issue locally, but am not sure of the cause. Could it be that using #include <time.h> in files.h and files.cpp is causing the error?

sbc100 · 2023-06-12T20:06:04Z

Can you run those tests with --rebase and then add the regressions to this PR. Its not unexpected that we will see slight regressions in wasmfs codesize test as you work on this stuff.

kripken · 2023-06-13T16:42:08Z

system/lib/wasmfs/file.cpp

-  setMTime(time(NULL));
+  struct timespec ts;
+  clock_gettime(CLOCK_REALTIME, &ts);
+  setMTime(ts);


It would be good to avoid this repeated code pattern somehow. If we did that then we would have a single place to change if we decided to get time information in another way in the future.

One option might be an updateMTime method which would mean "update to the current time". There might be a better name though?

I agree - would it also be helpful to have these "update to current time" functions for atime and ctime? Or is this only really needed for mtime?

For symmetry it might be nice to have it for them all, even if mtime is the most-repeating pattern.

Another option might be "updateTime" without mentioning mtime at all. Basically "this file was modified, update whatever time changes need to be done".

I thinking making one "update to current time" for each time might be a good option here, since there were a few other spots where only atime and ctime also had to be set to the current time. Would that be fine for now?

sbc100 · 2023-06-14T14:36:43Z

src/library_wasmfs.js

    },
    // TODO: utime
+    utime: (path, atime, mtime) => {
+      return FS.handleError(withStackSave(() => {


You can remove the return here and the curly brace before it. Similar to #19614

kripken · 2023-06-14T17:03:25Z

test/fs/test_fs_js_api.c

 #include <assert.h>
 #include <fcntl.h>

+EM_JS(void, test_fs_open, (), {


Maybe make a separate PR for the test refactorings of existing tests?

kripken · 2023-06-14T17:05:07Z

test/other/metadce/test_metadce_files_wasmfs.size

@@ -1 +1 @@
-51960
+52403


This is the cost of supporting nanoseconds, I suppose?

I am not sure it's worth it, personally. 500 bytes isn't a lot, but the benefit is unclear to me... but if you feel strongly @tlively I don't object.

Regardless, if we decide we want nanoseconds let's do it in its own PR?

Yes, I think having better than second precision is worth 500 bytes. I'm surprised it costs that much, though.

This PR refactors test_fs_js_api by placing each test into a separate method. This PR was split from #19561

tlively

LGTM % stale TODOs.

src/library_wasmfs.js

kripken · 2023-06-15T22:05:17Z

Is this ready to land? (it's still marked "draft")

jameshu15869 · 2023-06-16T01:31:35Z

Oops, I forgot to set it as "ready" but the branch is ready to land.

This PR adds nanosecond support for file times in WasmFS, specifically when used for utime and stat. This PR was separated from #19561

jameshu15869 and others added 5 commits June 2, 2023 13:33

implement utime and test, need to clarify details

fe411a3

debug comments

af6d66e

add nanosecond support in wasmfs

9a05071

fix comment

fbf1c46

Merge branch 'main' into library-wasmfs-utime

15472d4

jameshu15869 marked this pull request as draft June 8, 2023 20:09

sbc100 reviewed Jun 8, 2023

View reviewed changes

kripken reviewed Jun 8, 2023

View reviewed changes

tlively reviewed Jun 9, 2023

View reviewed changes

jameshu15869 and others added 4 commits June 9, 2023 14:41

fix merge conflict

a958cb8

refactor test_fs_js_api

bcf6630

clean up and combine getters/setters

e9b362f

Merge branch 'main' into library-wasmfs-utime

04d816c

tlively reviewed Jun 12, 2023

View reviewed changes

jameshu15869 and others added 3 commits June 12, 2023 15:04

change times to timespec

9bb61f9

Merge branch 'library-wasmfs-utime' of https://github.com/jameshu1586…

d5cbd35

…9/emscripten into library-wasmfs-utime

Merge branch 'main' into library-wasmfs-utime

86843e5

sbc100 reviewed Jun 12, 2023

View reviewed changes

jameshu15869 added 3 commits June 12, 2023 16:33

update metadce files

f2670fb

Merge branch 'main' into library-wasmfs-utime

3ceae4c

update metadata

261d52c

kripken reviewed Jun 13, 2023

View reviewed changes

jameshu15869 marked this pull request as ready for review June 13, 2023 17:20

jameshu15869 marked this pull request as draft June 13, 2023 17:21

add update to current time methods

45db5be

jameshu15869 marked this pull request as ready for review June 13, 2023 19:41

fix merge conflicts

8148d76

jameshu15869 mentioned this pull request Jun 14, 2023

WasmFS JS API: Implement allocate #19602

Merged

fix merge conflicts

0bfe926

sbc100 reviewed Jun 14, 2023

View reviewed changes

kripken reviewed Jun 14, 2023

View reviewed changes

jameshu15869 added 2 commits June 14, 2023 15:37

go back to only utime and original tests

4277a57

remove ns and test refactoring for separate PR

b7c8fde

jameshu15869 mentioned this pull request Jun 14, 2023

WasmFS JS API: Refactor tests #19628

Merged

jameshu15869 marked this pull request as draft June 14, 2023 20:34

jameshu15869 added 3 commits June 14, 2023 16:43

revert ns changes

ddd274e

Merge branch 'main' into library-wasmfs-utime

bee6c97

revert metadata changes

b27df2e

jameshu15869 mentioned this pull request Jun 14, 2023

WasmFS: Add nanosecond support to WasmFS #19629

Merged

clean up:

5684efd

jameshu15869 changed the title ~~WasmFS JS API: Implement utime and add support for nanseconds~~ WasmFS JS API: Implement utime Jun 14, 2023

kripken pushed a commit that referenced this pull request Jun 14, 2023

[NFC] WasmFS JS API: Refactor tests (#19628)

b3e64ee

This PR refactors test_fs_js_api by placing each test into a separate method. This PR was split from #19561

tlively approved these changes Jun 15, 2023

View reviewed changes

src/library_wasmfs.js Outdated Show resolved Hide resolved

jameshu15869 added 2 commits June 15, 2023 15:22

fix merge conflicts and clean up

2b69c94

clean merge resolution

c56b3e2

kripken approved these changes Jun 15, 2023

View reviewed changes

jameshu15869 marked this pull request as ready for review June 16, 2023 01:31

kripken merged commit 51b99e1 into emscripten-core:main Jun 16, 2023

kripken pushed a commit that referenced this pull request Jun 21, 2023

WasmFS: Add nanosecond support to WasmFS (#19629)

d25f32e

This PR adds nanosecond support for file times in WasmFS, specifically when used for utime and stat. This PR was separated from #19561

		@@ -1 +1 @@
		51960
		52403

WasmFS JS API: Implement utime #19561

WasmFS JS API: Implement utime #19561

Uh oh!

Conversation

jameshu15869 commented Jun 8, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jameshu15869 commented Jun 12, 2023

Uh oh!

sbc100 commented Jun 12, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jameshu15869 Jun 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tlively left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kripken commented Jun 15, 2023

Uh oh!

jameshu15869 commented Jun 16, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jameshu15869 Jun 13, 2023 •

edited

Loading