Implement WASI poll_oneoff #716

electrum · 2024-12-18T11:10:01Z

Fixes #592

andreaTP

Thanks for taking a stab at this!
A few comments to better understand what's going on.

Have you tested this implementation with actual code using it?
Go, Zig and even Rust should produce this call in some circumstances ...

wasi/src/main/java/com/dylibso/chicory/wasi/WasiPreview1.java

andreaTP · 2024-12-18T11:19:48Z

wasi/src/main/java/com/dylibso/chicory/wasi/WasiPreview1.java

+                    nevents++;
+                }
+            }
+            if (nevents > 0) {


What's the reference implementation you started from?

I cannot find this condition in the wazero implementation.

I wrote this myself based on the specification. The wazero implementation seems incorrect as it writes out all clock events, when per the specification it should only be the clocks that reached the timeout:

The time value of clock subscription_clock::id has reached timestamp subscription_clock::timeout.

This condition is actually incorrect, as it will prevent writing out clocks if there were any triggered descriptors. Instead, I'll change loop to skip sleeping if it already wrote events.

I wrote this myself based on the specification.

Can you add a comment with the relevant (perma-) link?

This is just the specification for WASI. We could add links for all the functions as a separate commit if you think that is valuable.

wasi/src/main/java/com/dylibso/chicory/wasi/WasiPreview1.java

andreaTP · 2024-12-18T11:27:13Z

wasi/src/main/java/com/dylibso/chicory/wasi/WasiPreview1.java

+
+        long minTimeout = clockEvents.stream().mapToLong(Entry::getKey).min().orElse(0);
+        long start = System.nanoTime();
+        while (true) {


A while (true) with so much code afterwords make me slightly uncomfortable.

In this case, if I'm not missing an invariant:

nsubscriptions == 0

readEvents is an empty List

clockEvents is an empty List

the loop doesn't end

is it the expected behavior?

+1 I think just a few test cases would be useful to clarify the behavior :)

there's a few here

unit tests https://github.com/tetratelabs/wazero/blob/610c202ec48f3a7c729f2bf11707330127ab3689/imports/wasi_snapshot_preview1/poll_test.go

integration:

https://github.com/tetratelabs/wazero/blob/610c202ec48f3a7c729f2bf11707330127ab3689/imports/wasi_snapshot_preview1/wasi_stdlib_test.go#L275

https://github.com/tetratelabs/wazero/blob/610c202ec48f3a7c729f2bf11707330127ab3689/imports/wasi_snapshot_preview1/wasi_stdlib_test.go#L501

(it doesn't need to be all, because this is not really covering all of them, just a source of inspiration)

You're right, I had a typo on the check at the start. It should be

If nsubscriptions is 0, returns errno::inval.

So we will never enter the loop in that case.

Thanks for updating this, now it looks safer, in all honesty, I still need to squint a lot my eyes to understand what's going on.

Adding unit tests for the implementation is an option to better clarify the expected behavior, does it sound feasible without a major amount of additional work?

I added tests for all cases except for invalid input and IOException, and verified this with code coverage in the IDE. I also added comments which hopefully makes the logic easier to follow.

electrum · 2024-12-19T15:37:13Z

I verified the implementation with Python, which uses this function for some modules such as socket (which fail later on but call this function first).

andreaTP · 2024-12-19T16:50:56Z

I verified the implementation with Python

Thanks!
Before merging, for diligence, we should test on one additional language.

On top of my head, those are the easiest to try:

Go: c2w
Zig: testsuite

electrum · 2024-12-20T09:45:34Z

Agreed, I'll test both of those above.

andreaTP

Great improvements here, thanks @electrum for addressing the comments, it reads much better now!

One additional note on the use of System.nanoTime and I'll wait for the results of running Go and/or Zig 🙂

andreaTP · 2024-12-20T11:28:21Z

wasi/src/main/java/com/dylibso/chicory/wasi/WasiPreview1.java

            case WasiClockId.MONOTONIC:
-                memory.writeLong(resultPtr, System.nanoTime());
+                memory.writeLong(resultPtr, clockTime(clockId));


Would it be possible to use clockTime(or something similar) everywhere instead of System.nanotime()?

This method is a helper method that is shared with clock_time_get() to get the realtime or monotonic clock. The other usages of System.nanoTime() are in poll_oneoff() to measure how long we have been sleeping. As I mentioned in the other comment, mocking that seems insufficient as we directly call Thread.sleep(). I'd need to think more about how those interact.

andreaTP · 2024-12-20T11:33:00Z

wasi-tests/src/test/java/com/dylibso/chicory/wasi/WasiPreview1Test.java

+
+    @Test
+    public void wasiPollOneoffClockAbstime() {
+        long deadline = System.nanoTime() + MILLISECONDS.toNanos(25);


Instead of using System.nanotime it would be great to use a "mocked" clock.
i.e.:

set a clock source that can be controlled from the test

trigger pollOneOff and verify that the deadline has not been reached

increase the clock to be > than the expected deadline

verify that the deadline is reached

Do you think it's doable?

We support mocking for the realtime Clock via WasiOptions. However, I don't think that simply mocking System.nanoTime() is sufficient, as it interacts with the way we use Thread.sleep().

Remember that poll_oneoff() waits until there is at least one event. So in order to test that a clock event doesn't trigger, we need a different event to trigger. But then we won't actually sleep, unless we mock stdin with something that starts off unreadable and becomes readable later.

I can look at this more if you feel strongly, but I think the testing we have is sufficient, especially for something that likely won't be used for more than a basic sleep function.

electrum · 2024-12-23T06:02:54Z

The c2w test works. The Zig test runs until this point:

716/2552 test.GUID... OK
717/2552 test.chdir smoke test... SKIP
718/2552 test.open smoke test... SKIP

com.dylibso.chicory.wasm.UninstantiableException: Trapped on unreachable instruction

	at com.dylibso.chicory.runtime.Instance.initialize(Instance.java:159)
	at com.dylibso.chicory.runtime.Instance.<init>(Instance.java:100)
	at com.dylibso.chicory.runtime.Instance$Builder.build(Instance.java:722)
	at com.dylibso.chicory.wasi.WasiPreview1Test.shouldRunZigStdlibTestsuite(WasiPreview1Test.java:239)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1597)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1597)
Caused by: com.dylibso.chicory.runtime.TrapException: Trapped on unreachable instruction
	at com.dylibso.chicory.runtime.InterpreterMachine.eval(InterpreterMachine.java:150)
	at com.dylibso.chicory.runtime.InterpreterMachine.call(InterpreterMachine.java:81)
	at com.dylibso.chicory.runtime.InterpreterMachine.CALL(InterpreterMachine.java:1637)
	...

andreaTP

I can look at this more if you feel strongly, but I think the testing we have is sufficient, especially for something that likely won't be used for more than a basic sleep function.

I agree with this evaluation, it would be nice to have more isolated tests not using global resources like nanoTime and sleep, but the current implementation and testing is definitely sufficient.

LGTM, thanks for the effort and bearing with me.

Additional notes:

yes please, add the link to the WASI spec you are looking at (I'm always confused about the right link)
I recall the zig testsuite to progress a bit more, but it can be a number of factors ...

The reference implementation does not support or validate stderr. For stdout, it treats a default specification for a missing JSON file as if the field is not present, rather than an empty string (thus ignoring anything that the test outputs to stdout).

andreaTP reviewed Dec 18, 2024

View reviewed changes

electrum force-pushed the wasi-poll branch from 93f1a72 to 5fd8286 Compare December 19, 2024 15:34

electrum force-pushed the wasi-poll branch 2 times, most recently from 5921c9a to 1002328 Compare December 20, 2024 09:42

andreaTP reviewed Dec 20, 2024

View reviewed changes

electrum requested a review from andreaTP December 23, 2024 10:08

andreaTP approved these changes Dec 23, 2024

View reviewed changes

electrum force-pushed the wasi-poll branch from 1002328 to 42a5771 Compare December 24, 2024 04:15

electrum force-pushed the wasi-poll branch 2 times, most recently from 75e6f4e to 914f67f Compare December 24, 2024 04:30

Implement WASI poll_oneoff

c51cba6

electrum force-pushed the wasi-poll branch from 914f67f to c51cba6 Compare December 24, 2024 04:31

electrum merged commit c09374c into dylibso:main Dec 24, 2024
15 checks passed

electrum deleted the wasi-poll branch December 24, 2024 04:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement WASI poll_oneoff #716

Implement WASI poll_oneoff #716

electrum commented Dec 18, 2024 •

edited

Loading

andreaTP left a comment

andreaTP Dec 18, 2024

electrum Dec 19, 2024

andreaTP Dec 19, 2024

electrum Dec 21, 2024

andreaTP Dec 18, 2024

evacchi Dec 19, 2024

electrum Dec 19, 2024

andreaTP Dec 19, 2024

electrum Dec 20, 2024

electrum commented Dec 19, 2024

andreaTP commented Dec 19, 2024

electrum commented Dec 20, 2024

andreaTP left a comment

andreaTP Dec 20, 2024

electrum Dec 20, 2024

andreaTP Dec 20, 2024

electrum Dec 20, 2024

electrum commented Dec 23, 2024

andreaTP left a comment

Implement WASI poll_oneoff #716

Implement WASI poll_oneoff #716

Conversation

electrum commented Dec 18, 2024 • edited Loading

andreaTP left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

electrum commented Dec 19, 2024

andreaTP commented Dec 19, 2024

electrum commented Dec 20, 2024

andreaTP left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

electrum commented Dec 23, 2024

andreaTP left a comment

Choose a reason for hiding this comment

electrum commented Dec 18, 2024 •

edited

Loading