windows/thread: Add support for win32 micropython _thread. #7796

andrewleech · 2021-09-14T23:00:16Z

This PR adds support for thread to the windows port with both mingw and msvc compilers.

Passes the test suite:

C:\Users\andrewleech\src\micropython\tests>py run-tests.py -d thread
pass  thread/mutate_bytearray.py
pass  thread/mutate_instance.py
pass  thread/stress_recurse.py
skip  thread/stress_schedule.py
pass  thread/thread_exc1.py
pass  thread/mutate_list.py
pass  thread/mutate_dict.py
pass  thread/thread_gc1.py
pass  thread/thread_heap_lock.py
pass  thread/thread_ident1.py
pass  thread/thread_lock1.py
pass  thread/mutate_set.py
pass  thread/stress_create.py
pass  thread/thread_exc2.py
pass  thread/thread_lock5.py
pass  thread/thread_lock3.py
pass  thread/thread_shared1.py
pass  thread/thread_shared2.py
pass  thread/thread_exit1.py
pass  thread/thread_stacksize1.py
pass  thread/thread_exit2.py
pass  thread/thread_lock2.py
pass  thread/thread_lock4.py
pass  thread/thread_qstr1.py
pass  thread/thread_start1.py
pass  thread/thread_start2.py
pass  thread/stress_heap.py
pass  thread/thread_sleep1.py
pass  thread/stress_aes.py
28 tests performed (141 individual testcases)
28 tests passed
1 tests skipped: stress_schedule

stinos · 2021-09-15T07:52:56Z

Looks good. The thread tests don't run in CI though, would be best to add that.

thread_exc2 fails here for all msvc builds:

@@ -1,5 +1,5 @@
-Unhandled exception in thread started by <function thread_entry at 0x\[0-9a-f\]\+>
+Unhandled exception in thread started by <function thread_entry at 0x3061a0>
 Traceback (most recent call last):
-  File \.\+, line 7, in thread_entry
+  File "./thread/thread_exc2.py", line 7, in thread_entry
 ValueError:
 done

and thread_thread_stacksize1.py hangs forever for Debug/x64 but does print

0
True
0

For ease of maintainance it's tempting to incorporate this into unix/mpthreadport though since the main difference is pthread vs win32 and the rest of the code is non-trivial, have you considered that?

andrewleech · 2021-09-15T09:01:17Z

That's curious, the unit test results above were from a local msvc build on my laptop, I wonder why there's a difference.

I guess I could possibly turn what I've done into a custom wrapper/shim to work with the unix driver, not sure how much value is in that now though.

I didn't previously think it was worth it, there really are a fair few differences - the signalling method for thread gc needed a new architecture, the entry needed a wrapper and the waiting for events is a common function vs different functions for each type of object.
I also needed to extend the _thread struct used for the linked list.

I had previously tried using a win32-pthreads
(wrapper style) library to provide compatibility with the existing unix driver but that wasn't trivial either, didn't resolve the signalling issue and brought in more dependencies.

andrewleech · 2021-09-15T09:30:39Z

Ah, thread_exc2 is in a special_tests category that is skipped by default - I'm not entirely sure why.

In terms of ci - yeah it turns out the thread tests aren't run by default on any platform I think - it's not run on unix port ci tests at least! Again, it seems like something that should be run on any platform that supports it...

micropython/tests/run-tests.py

Line 806 in 2a7b6d6

if len(args.files) == 0:

stinos · 2021-09-16T12:49:32Z

Yeah i wouldn't go for win32-pthreads especially if you have something which works already.

stinos · 2021-11-24T08:08:35Z

ports/windows/Makefile

@@ -49,6 +50,9 @@ ifeq ($(MICROPY_USE_READLINE),1)
 CFLAGS_MOD += -DMICROPY_USE_READLINE=1
 SRC_C += shared/readline/readline.c
 endif
+# ifeq ($(MICROPY_PY_THREAD),1)


I'd think MicroPython's policy is to not check in dead code?

Whoops yeah, thanks for that. I initially expected to need MICROPY_PY_THREAD to be defined in makefile snippet like other ports, ended up not turning out that way as it doesn't need extra linker argurments.

I did push this branch along with the other windows ones I'm working on in a rush before the meetup so didn't do a personal git review properly... sorry for the extra noise :-/

stinos · 2021-11-24T08:10:24Z

ports/windows/mpthreadport.c

+// This value seems to be about right for both 32-bit and 64-bit builds.
+#define THREAD_STACK_OVERFLOW_MARGIN (8192)
+
+// this structure forms a linked list, one node per active thread


Minor nitpick; perhaps this should be enforced by the formatting as well, but it looks a bit off to have half of the comments like // This is a comment. and the others // this is a comment

That's a good point really, I guess it's possibly asking too much to parse for grammer in the auto format checker, but things like this all contribute to the consistency of quality in a project.

andrewleech · 2021-11-24T22:31:43Z

FWIW my main use case for threads on unix/windows is to enable easy mocking of Timers and similar for running unit tests, a loop in a Thread behaves quite similar to an interrupt in embedded code,m especially if you enforce a gc lock in the mocked function.

stinos · 2021-11-25T07:03:07Z

easy mocking of Timers

I assume you're aware that most likely all sleep/wait implementations on windows by default use a 16mSec (or close to that) resolution? Just mentioning because it's a common source of problems in tests which deal with timing.

andrewleech · 2021-11-25T07:35:32Z

I didn't realise that, do you know if that applies to the SleepEx command I've used here?
I assumed it'd be more efficient than the custom usleep function, though if SleepEx can't do better than 16ms resolution perhaps I should go back to using usleep (assuming it does work as described).

My mock timers are generally less about testing specific exact timing (it's always hard to compare embedded to unix for timing) but more about being able to run the same code on embedded as on desktop and have background tasks run regularly that can interrupt the main application at any point (unlike background async tasks).

stinos · 2021-11-25T07:52:51Z

do you know if that applies to the SleepEx command I've used here?

Yes, see https://docs.microsoft.com/en-us/windows/win32/api/synchapi/nf-synchapi-sleepex#remarks

My mock timers are generally less about testing specific exact timing

Yeah as long as there's nothing which explicitly relies on wait/sleep being accurate (there shouldn't be ideally) it is usually ok, but still, especially with smaller timeouts, things sometimes don't go well. But in any cas: if tests pass there's not much to worry about.

ports/windows/mpconfigport.h

dpgeorge · 2022-01-23T00:06:04Z

@andrewleech can you please rebase this on latest master? And also enable thread tests in ports/windows/.appveyor.yml by adding thread to the end of line 56.

dpgeorge · 2022-02-07T05:17:20Z

ports/windows/mpthreadport.c

+
+void mp_thread_mutex_init(mp_thread_mutex_t *mutex) {
+    // The win32 Mutex is re-entrant, unlike the posix equivalent.
+    // To avoid this a sepaphore with size of 1 is used.


spelling: "semaphore"

dpgeorge · 2022-02-07T05:21:54Z

ports/windows/sleep.c

+// mingw and the likes provide their own usleep() which we need to
+// replace with one that can be interrupted by APC function if needed
+// (used in mp_thread_gc)
+#undef usleep


Is this the right way to do things? If usleep is #define'd to something then that define will be active in all other compilation units. So only the code in this file will pick up this custom usleep implementation.

Why not just keep it as usleep_impl()?

Yeah that was a bit strange / wrong. usleep_impl() was only being used in the single usleep() wrapper function below on msvc, it was ignored elsewhere. I thought I needed it to be the usleep used everywhere - but this wouldn't have actually been working as you suggest.

Turns out we don't need to provide usleep in any particular global sense, it was only being used in unix/mpconfigport.h to provide mp_hal_delay_us() so I think it's better to directly provide mp_hal_delay_us() here for win32 only.

dpgeorge · 2022-02-08T01:21:38Z

This change has broken some tests on AppVeyor:

1 tests failed: utime_time_ns
--- "C:/projects/micropython/tests/results\\extmod_utime_time_ns.py.exp"	2022-02-07 23:35:19.943524600 +0000
+++ "C:/projects/micropython/tests/results\\extmod_utime_time_ns.py.out"	2022-02-07 23:35:19.944525900 +0000
@@ -1,2 +1,2 @@
-True
-True
+False
+1644276919941525000 1644276919941525000 0
FAILURE C:/projects/micropython/tests/results\extmod_utime_time_ns.py

projectgus · 2024-03-07T23:52:33Z

This is an automated heads-up that we've just merged a Pull Request
that removes the STATIC macro from MicroPython's C API.

See #13763

A search suggests this PR might apply the STATIC macro to some C code. If it
does, then next time you rebase the PR (or merge from master) then you should
please replace all the STATIC keywords with static.

Although this is an automated message, feel free to @-reply to me directly if
you have any questions about this.

dpgeorge added the port-windows label Oct 15, 2021

andrewleech force-pushed the windows-thread branch from 2a7b6d6 to 2c121c3 Compare November 24, 2021 06:13

stinos reviewed Nov 24, 2021

View reviewed changes

andrewleech force-pushed the windows-thread branch from 2c121c3 to 903daa4 Compare November 24, 2021 22:30

andrewleech force-pushed the windows-thread branch 3 times, most recently from 3f3d8fb to bed2436 Compare November 25, 2021 06:32

dpgeorge reviewed Jan 23, 2022

View reviewed changes

ports/windows/mpconfigport.h Show resolved Hide resolved

andrewleech force-pushed the windows-thread branch from bed2436 to 363a561 Compare January 24, 2022 00:43

andrewleech force-pushed the windows-thread branch 3 times, most recently from 1a9f29c to 08544d1 Compare February 7, 2022 05:10

andrewleech changed the title ~~windows/thread: add support for win32 micropython _thread.~~ windows/thread: Add support for win32 micropython _thread. Feb 7, 2022

andrewleech force-pushed the windows-thread branch from 08544d1 to 9f0a4ed Compare February 7, 2022 05:13

dpgeorge reviewed Feb 7, 2022

View reviewed changes

andrewleech force-pushed the windows-thread branch from 9f0a4ed to 9d55587 Compare February 7, 2022 23:28

windows/thread: Add support for win32 micropython _thread.

694b941

andrewleech closed this Feb 7, 2022

andrewleech force-pushed the windows-thread branch from 9d55587 to 6f7d6c5 Compare February 7, 2022 23:33

andrewleech reopened this Feb 7, 2022

stinos mentioned this pull request Feb 14, 2022

uasyncio tests can fail on windows ports #8301

Closed

Gadgetoid mentioned this pull request Feb 29, 2024

global: Remove the STATIC macro. #13763

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

windows/thread: Add support for win32 micropython _thread. #7796

windows/thread: Add support for win32 micropython _thread. #7796

andrewleech commented Sep 14, 2021

stinos commented Sep 15, 2021

andrewleech commented Sep 15, 2021

andrewleech commented Sep 15, 2021

stinos commented Sep 16, 2021

stinos Nov 24, 2021

andrewleech Nov 24, 2021

stinos Nov 24, 2021

andrewleech Nov 24, 2021

andrewleech commented Nov 24, 2021

stinos commented Nov 25, 2021

andrewleech commented Nov 25, 2021

stinos commented Nov 25, 2021

dpgeorge commented Jan 23, 2022

dpgeorge Feb 7, 2022

dpgeorge Feb 7, 2022

andrewleech Feb 7, 2022 •

edited

Loading

dpgeorge commented Feb 8, 2022

projectgus commented Mar 7, 2024

windows/thread: Add support for win32 micropython _thread. #7796

Are you sure you want to change the base?

windows/thread: Add support for win32 micropython _thread. #7796

Conversation

andrewleech commented Sep 14, 2021

stinos commented Sep 15, 2021

andrewleech commented Sep 15, 2021

andrewleech commented Sep 15, 2021

stinos commented Sep 16, 2021

stinos Nov 24, 2021

Choose a reason for hiding this comment

andrewleech Nov 24, 2021

Choose a reason for hiding this comment

stinos Nov 24, 2021

Choose a reason for hiding this comment

andrewleech Nov 24, 2021

Choose a reason for hiding this comment

andrewleech commented Nov 24, 2021

stinos commented Nov 25, 2021

andrewleech commented Nov 25, 2021

stinos commented Nov 25, 2021

dpgeorge commented Jan 23, 2022

dpgeorge Feb 7, 2022

Choose a reason for hiding this comment

dpgeorge Feb 7, 2022

Choose a reason for hiding this comment

andrewleech Feb 7, 2022 • edited Loading

Choose a reason for hiding this comment

dpgeorge commented Feb 8, 2022

projectgus commented Mar 7, 2024

andrewleech Feb 7, 2022 •

edited

Loading