Implement suspend flags as atomic variable by loganek · Pull Request #2361 · bytecodealliance/wasm-micro-runtime

loganek · 2023-07-14T17:01:21Z

We have observed a significant performance degradation after merging #1991 Instead of protecting suspend flags with a mutex, we implement the flags as atomic variable and only use mutex when atomics are not available on a given platform.

loganek · 2023-07-17T16:28:17Z

@wenyongh the build is passing now and the PR is ready for review. I'd appreciate your team's review. Many thanks

yamt · 2023-07-18T05:06:22Z

core/iwasm/common/wasm_suspend_flags.h

@@ -0,0 +1,79 @@
+/*
+ * Copyright (C) 2023 Amazon Inc.  All rights reserved.


most of the logic in this file seems not specific to suspend flags. how about using a bit more generic name?

i guess the default implementation should be c11 atomics.

Yes, initially I've had an implementation based on stdatomics, but there are two problems with that:

The size of the atomic variable is not guaranteed to be the size of the base type - and this is a requirement for JIT. We could potentially workaround it with set of ifs on code generation phase though

this file is included (indirectly) by some c++ files (fast jit) and removing the dependency would require major refactoring. C11 stdatomics are not available in c++.

For those reasons I decided to go for a simple solution for now, but happy to iterate on that.

how about "most of the logic in this file seems not specific to suspend flags. how about using a bit more generic name?"

The file right now includes the following:

definitions of the suspend flag values (WASM_SUSPEND_FLAG_* definitions)

WASMSuspendFlags data structure

macros for operating on the suspend flag data structure (WASM_SUSPEND_FLAGS_* macros)

information whether the suspend flag is atomic (WASM_SUSPEND_FLAGS_IS_ATOMIC definition)

macros for locking the flag if it's not atomic (WASM_SUSPEND_FLAGS_(UN)LOCK macros)

They all seem pretty related to suspend flags.

availability check and wrappers on atomic builtins are generic.

nchamzn

Have profiled this change and performance is now comparable to before the locks were added.

yamt · 2023-07-19T07:51:16Z

core/iwasm/interpreter/wasm_interp_classic.c

+               & WASM_SUSPEND_FLAG_SUSPEND) {                         \
+            /* suspend current thread */                              \
+            SUSPENSION_LOCK()                                         \
+            os_cond_wait(&exec_env->wait_cond, &exec_env->wait_lock); \


check WASM_SUSPEND_FLAG_SUSPEND with the lock held.
otherwise wasm_cluster_resume_thread can fail to wake up the thread.

not updating it for now as per @wenyongh 's comment

yamt · 2023-07-19T07:56:20Z

core/iwasm/libraries/thread-mgr/thread_manager.c

-    exec_env->suspend_flags.flags &= ~0x02;
+    WASM_SUSPEND_FLAGS_FETCH_AND(exec_env->suspend_flags,
+                                 ~WASM_SUSPEND_FLAG_SUSPEND);
    os_cond_signal(&exec_env->wait_cond);


forgot to take wait_lock? (not a fault of this PR though)

The suspend/resume APIs (wasm_cluster_suspend_xxx and wasm_cluster_resume_xxx) are not called currently, and the current implementation is incomplete, I think let's ignore it here. It is a little complex to implement a full suspend/resume mechanism, we are trying to enable it, see #2320.

wenyongh · 2023-07-19T10:11:53Z

core/iwasm/common/wasm_suspend_flags.h

+#define _WASM_SUSPEND_FLAGS_H
+
+#include "bh_platform.h"
+#include "gnuc.h"


Could you include "gnuc.h" in "bh_platform.h" instead? We often only include "bh_platfrom.h" to include all the required APIs in core/shared/platform and core/shared/utils.

wenyongh · 2023-07-19T10:14:27Z

core/iwasm/common/wasm_suspend_flags.h

+#if WASM_SUSPEND_FLAGS_IS_ATOMIC != 0
+#define WASM_SUSPEND_FLAGS_LOCK(lock) \
+    do {                              \
+    } while (0)


How about (void)0?

wenyongh · 2023-07-19T10:25:16Z

core/iwasm/libraries/thread-mgr/thread_manager.c

-    exec_env->suspend_flags.flags &= ~0x02;
+    WASM_SUSPEND_FLAGS_FETCH_AND(exec_env->suspend_flags,
+                                 ~WASM_SUSPEND_FLAG_SUSPEND);
    os_cond_signal(&exec_env->wait_cond);


The suspend/resume APIs (wasm_cluster_suspend_xxx and wasm_cluster_resume_xxx) are not called currently, and the current implementation is incomplete, I think let's ignore it here. It is a little complex to implement a full suspend/resume mechanism, we are trying to enable it, see #2320.

wenyongh · 2023-07-19T10:28:00Z

core/iwasm/libraries/thread-mgr/thread_manager.c

    os_mutex_lock(&exec_env->wait_lock);
    bool is_thread_terminated =
-        (exec_env->suspend_flags.flags & 0x01) ? true : false;
+        (WASM_SUSPEND_FLAGS_GET(exec_env->suspend_flags) & 0x01) ? true : false;


Had better change 0x01 to WASM_SUSPEND_FLAG_TERMINATE

We have observed a significant performance degradation after merging bytecodealliance#1991 Instead of protecting suspend flags with a mutex, we implement the flags as atomic variable and only use mutex when atomics are not available on a given platform.

loganek force-pushed the main branch 24 times, most recently from dba41b7 to fe64376 Compare July 17, 2023 14:31

yamt reviewed Jul 18, 2023

View reviewed changes

nchamzn approved these changes Jul 18, 2023

View reviewed changes

yamt reviewed Jul 19, 2023

View reviewed changes

wenyongh reviewed Jul 19, 2023

View reviewed changes

loganek force-pushed the main branch from fe64376 to a40638d Compare July 20, 2023 05:43

wenyongh merged commit 0f4edf9 into bytecodealliance:main Jul 21, 2023

		@@ -0,0 +1,79 @@
		/*
		* Copyright (C) 2023 Amazon Inc. All rights reserved.

Conversation

loganek commented Jul 14, 2023

Uh oh!

loganek commented Jul 17, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nchamzn left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wenyongh Jul 19, 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

wenyongh Jul 19, 2023 •

edited

Loading