Implementation for the wasi_clock_time/res by fadumina1 · Pull Request #2637 · bytecodealliance/wasm-micro-runtime

fadumina1 · 2023-10-12T11:50:00Z

No description provided.

core/shared/platform/windows/win_clock.c

core/iwasm/libraries/libc-wasi/sandboxed-system-primitives/src/posix.c

core/shared/platform/include/platform_common.h

core/shared/platform/windows/win_clock.c

core/shared/utils/bh_time.c

product-mini/platforms/windows/CMakeLists.txt

loganek · 2023-10-17T12:37:35Z

core/shared/platform/include/platform_api_vmcore.h


+/* Get current resolution of clock id */
+int
+os_clock_res_get(bh_clock_id_t clock_id, uint64 *resolution);


we already have the declaration in platform_common.h so the declaration should probably be removed from this file.

core/shared/platform/include/platform_common.h

loganek · 2023-10-17T12:39:20Z

core/shared/platform/include/platform_common.h

+ * @return BHT_OK if success; otherwise, BHT_ERROR
+ */
+int
+os_clock_res_get(bh_clock_id_t clock_id, uint64 *resolution);


I'm actually wondering if we should move those two declarations to platform_api_extension.h file.

core/shared/platform/windows/win_clock.c

product-mini/platforms/windows/CMakeLists.txt

zoraaver · 2023-10-17T14:17:02Z

core/iwasm/libraries/libc-wasi/sandboxed-system-primitives/src/posix.c

 }

 __wasi_errno_t
 wasmtime_ssp_clock_time_get(__wasi_clockid_t clock_id,


Sorry for the late comment, did we consider implementing these functions (wasmtime_ssp_clock_time_get and wasmtime_ssp_clock_res_get) directly in each platform? Unlike the filesystem or socket functions, they don't contain any logic to lookup a fd object from the fd table so it seems they are very small wrapper around the platform implementation anyway.

Also, since os_clock_time_get and os_clock_res_get are only used in the WASI libc implementation, we could probably define the signature for these functions in terms of WASI types (__wasi_clockid_t etc.) to avoid some boilerplate. What do you think?

I think that's not been done mainly due to the circular dependency as discussed in #2585 (comment)

I think we can merge this as it is now, but once your change @zoraaver is merged, we'll make an update to this code and simplify it if possible. How does it sound to you?

Agree, I think the PR is in a good state to merge now and we can simplify the code later once #2585 is merged.

loganek · 2023-10-18T11:01:21Z

@wenyongh would you be able to have a look at this PR?

core/iwasm/libraries/libc-wasi/sandboxed-system-primitives/src/posix.c

core/shared/platform/common/posix/posix_clock.c

wenyongh · 2023-10-19T01:12:15Z

core/shared/utils/bh_time.c

+#include "bh_time.h"
+
+uint64
+convert_timespec_to_nanoseconds(const struct timespec *ts)


How about renaming to bh_timespec_to_nanoseconds? Adding bh_ prefix is because that APIs in core/shared/utils usually start with it.

wenyongh · 2023-10-19T10:59:06Z

core/iwasm/libraries/libc-wasi/sandboxed-system-primitives/src/posix.c

+{
+}


Should not change the code here. It will lead to CI code guideline check failure.

wenyongh · 2023-10-19T11:00:58Z

core/iwasm/libraries/libc-wasi/sandboxed-system-primitives/src/posix.c

 }
+#ifndef BH_PLATFORM_WINDOWS
+/*
+ * wasi_clockid_to_clockid is a plceholder in this file so that poll_oneoff complies,


Should be placeholder

wenyongh · 2023-10-19T11:01:51Z

core/iwasm/libraries/libc-wasi/sandboxed-system-primitives/src/posix.c

+ */
+bool
+wasi_clockid_to_clockid(__wasi_clockid_t in, clockid_t* out) 
+    {


Invalid coding style, could you use clang-format-12 to format the code?
https://github.com/bytecodealliance/wasm-micro-runtime/actions/runs/6573605221/job/17856981299

wenyongh · 2023-10-19T11:03:48Z

core/shared/platform/windows/win_clock.c

+        *out_counter = counter.QuadPart;
+        return BHT_OK;
+    }
+


Had better remove unused line?

core/shared/platform/windows/win_clock.c

core/iwasm/libraries/libc-wasi/sandboxed-system-primitives/src/posix.c

core/shared/platform/common/posix/posix_clock.c

core/shared/platform/windows/win_clock.c

core/shared/platform/common/posix/posix_clock.c

core/iwasm/libraries/libc-wasi/sandboxed-system-primitives/src/posix.c

core/shared/platform/common/posix/posix_clock.c

core/iwasm/libraries/libc-wasi/sandboxed-system-primitives/src/posix.c

core/shared/platform/common/posix/posix_clock.c

core/shared/platform/windows/win_clock.c

zoraaver

LGTM

wenyongh

LGTM

loganek

Good job!

Refactoring the clock functions to use WASI types so we can simplify the code and remove some unnecessary boilerplate. See bytecodealliance#2637 (comment) for details.

Refactoring the clock functions to use WASI types so we can simplify the code and remove some unnecessary boilerplate. See #2637 (comment) for details.

Add os_clock_res_get and os_clock_time_get in platform_api_extension.h, and implement them in posix like platforms and windows platform.

Refactoring the clock functions to use WASI types so we can simplify the code and remove some unnecessary boilerplate. See bytecodealliance#2637 (comment) for details.

zoraaver reviewed Oct 12, 2023

View reviewed changes

loganek reviewed Oct 12, 2023

View reviewed changes

core/iwasm/libraries/libc-wasi/sandboxed-system-primitives/src/posix.c Outdated Show resolved Hide resolved

loganek reviewed Oct 12, 2023

View reviewed changes

core/shared/platform/include/platform_common.h Outdated Show resolved Hide resolved

loganek reviewed Oct 12, 2023

View reviewed changes

core/shared/platform/windows/win_clock.c Outdated Show resolved Hide resolved

loganek reviewed Oct 12, 2023

View reviewed changes

core/shared/platform/windows/win_clock.c Outdated Show resolved Hide resolved

loganek reviewed Oct 12, 2023

View reviewed changes

core/shared/utils/bh_time.c Outdated Show resolved Hide resolved

loganek reviewed Oct 12, 2023

View reviewed changes

product-mini/platforms/windows/CMakeLists.txt Show resolved Hide resolved

loganek reviewed Oct 12, 2023

View reviewed changes

product-mini/platforms/windows/CMakeLists.txt Outdated Show resolved Hide resolved

fadumina1 force-pushed the dev/wasi-libc-windows branch 3 times, most recently from beb4921 to dbd8f53 Compare October 17, 2023 09:19

loganek requested changes Oct 17, 2023

View reviewed changes

fadumina1 force-pushed the dev/wasi-libc-windows branch from dbd8f53 to ffaf3d6 Compare October 17, 2023 12:54

zoraaver reviewed Oct 17, 2023

View reviewed changes

fadumina1 force-pushed the dev/wasi-libc-windows branch 2 times, most recently from d1b4d12 to cc86166 Compare October 18, 2023 10:27

wenyongh reviewed Oct 19, 2023

View reviewed changes

fadumina1 force-pushed the dev/wasi-libc-windows branch 6 times, most recently from 105abc9 to bf15d84 Compare October 19, 2023 10:27

loganek mentioned this pull request Oct 19, 2023

Abstract POSIX filesystem functions #2585

Merged

fadumina1 force-pushed the dev/wasi-libc-windows branch 2 times, most recently from 33b4b61 to e1aacc0 Compare October 19, 2023 10:59

wenyongh reviewed Oct 19, 2023

View reviewed changes

fadumina1 force-pushed the dev/wasi-libc-windows branch from e1aacc0 to 0408808 Compare October 19, 2023 11:11

loganek reviewed Oct 19, 2023

View reviewed changes

core/iwasm/libraries/libc-wasi/sandboxed-system-primitives/src/posix.c Outdated Show resolved Hide resolved

fadumina1 force-pushed the dev/wasi-libc-windows branch 5 times, most recently from db4fa6c to 04820c0 Compare October 19, 2023 14:18

loganek mentioned this pull request Oct 19, 2023

Implementation for the wasi_clock_time/res loganek/wasm-micro-runtime#1

Closed

Zzzabiyaka reviewed Oct 19, 2023

View reviewed changes

core/shared/platform/common/posix/posix_clock.c Outdated Show resolved Hide resolved

zoraaver reviewed Oct 19, 2023

View reviewed changes

wenyongh reviewed Oct 20, 2023

View reviewed changes

core/shared/platform/common/posix/posix_clock.c Outdated Show resolved Hide resolved

core/shared/platform/windows/win_clock.c Outdated Show resolved Hide resolved

core/shared/platform/windows/win_clock.c Outdated Show resolved Hide resolved

fadumina1 force-pushed the dev/wasi-libc-windows branch from 04820c0 to 884de74 Compare October 20, 2023 14:20

Implementation for the wasi_clock_time/res

330fda5

fadumina1 force-pushed the dev/wasi-libc-windows branch from 884de74 to 330fda5 Compare October 20, 2023 15:21

zoraaver approved these changes Oct 20, 2023

View reviewed changes

wenyongh reviewed Oct 21, 2023

View reviewed changes

loganek approved these changes Oct 22, 2023

View reviewed changes

wenyongh merged commit a874bf0 into bytecodealliance:dev/wasi-libc-windows Oct 22, 2023

zoraaver mentioned this pull request Oct 24, 2023

Refactor clock functions to use WASI types #2666

Merged

wenyongh pushed a commit that referenced this pull request Oct 25, 2023

Refactor clock functions to use WASI types (#2666)

e7a62d2

Refactoring the clock functions to use WASI types so we can simplify the code and remove some unnecessary boilerplate. See #2637 (comment) for details.

Conversation

fadumina1 commented Oct 12, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

loganek commented Oct 18, 2023

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zoraaver left a comment

Choose a reason for hiding this comment

Uh oh!

wenyongh left a comment

Choose a reason for hiding this comment

Uh oh!

loganek left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants