windows/schedule: Add micropython.schedule to windows port. #7999

andrewleech · 2021-11-17T06:24:04Z

Thie PR adds the basic hooks to allow micropython.schedule() to work correctly on the windows port.

It works (runs commands immediately) when run from repl.

Both mingw and mscv build passes tests/micropython/schedule.py

stinos · 2021-11-17T07:18:17Z

ports/windows/windows_mphal.c

+        MICROPY_EVENT_POLL_HOOK
+    }
+    #else
+    // TODO: POSIX et al. define usleep() as guaranteedly capable only of 1s sleep:


Should delete the comment on line 265 if this is here

Or actually: this is Windows, not exactly known for its POSIX compliance, moreover the msvc port supplies its own implementation of usleep, so tldr: this comment was copied from the unix port but probably doesn't matter or is even incorrect here.

stinos · 2021-11-17T07:49:00Z

ports/windows/mpconfigport.h

@@ -128,6 +128,9 @@
 #define MICROPY_ERROR_PRINTER       (&mp_stderr_print)
 #define MICROPY_WARNINGS            (1)
 #define MICROPY_PY_STR_BYTES_CMP_WARN (1)
+#ifndef MICROPY_ENABLE_SCHEDULER
+#define MICROPY_ENABLE_SCHEDULER    (1)


In my opinion this shouldn't be on by default: it never was, and it adds code to hot paths. I admit I didnt measure the impact, but it's non-zero which just feels too much for a feature which has otherwise been unused for the past years.

When the scheduler queue is empty, it only adds a single if statement to the VM (conditional on MP_STATE_VM(sched_state)) and even then, only after a branch instruction.

I will measure it on Linux and hopefully that can help guide the decision.

@stinos I'm curious as to your use case of the windows port, are your usage performance sensitive?

Keeping things off be default is often an important strategy on embedded ports where size/speed is critical, vs the desktop ports where feautures like this would add an almost imeasurably negligent amount of performance hit?
That being said, this is also why I want to bring in the variant system from other ports too, to make it easier to create build profiles. That doesn't help msvc yet though...

micropython.schedule is used widely and regularly on other ports and is a key piece of architecture for subsystems like BLE. Sure, it's use in interrupts isn't going to translate to desktop any time soon, but it's still used in a lot other places.

In my opinion, the windows port is rarely used mainly because it's missing so so many features that are standard on pretty much every other port.
Most of the other parts of this PR are also needed to enable uselect, which is needed to use uasyncio, which is a standard recommended library for most users.

I'd rather use default enable/disable from other ports to guide new features, rather than history of this port.

I don't want comment on being for or against enabling the scheduler by default, but from a technical point of view enabling it does not add any branches to the VM, it just changes

if (MP_STATE_THREAD(mp_pending_exception) != MP_OBJ_NULL) {

to

if (MP_STATE_VM(sched_state) == MP_SCHED_PENDING) {

(and then if there is something pending, either some code to run or an exception like KeyboardInterrupt, it has a little bit more work to do)

I'm curious as to your use case of the windows port, are your usage performance sensitive?

Sort of. We use it in 'production' at the heart of scientific experiments and most of the time the applications run loops (depending on situation period is around 5mSec, sometimes less, sometimes more) and each iteration must finish in time either because it's bound to ADC/DAC devices or because it generates graphics. Most of the critical things are done in C++ but we also run microPython code in those loops. tldr; it's not super critical and after having a second (proper) look this particuler feature won't do much. Apart from the changes to mp_hal_delay perhaps, if someone would rely on that having a certain accuracy but then that's rather their mistake :)

In my opinion, the windows port is rarely used mainly because it's missing so so many features that are standard on pretty much every other port.

That's an interesting thought; I always assumed it's rarely used because there's simply not a lot of practical use for it (there are enogh other Python versions for PC) and the community which does use MicroPython mostly does that on microcontrollers anyway and/or seem to be leaning towards unix.

I'd rather use default enable/disable from other ports to guide new features, rather than history of this port.

Fair point. Would also be interesting to see if there'd be more use if there would be more features on the windows ports.

stinos · 2021-11-17T07:49:54Z

ports/windows/mpconfigport.h

@@ -201,6 +204,14 @@ extern const struct _mp_obj_module_t mp_module_time;

 #define MICROPY_MPHALPORT_H         "windows_mphal.h"

+#define MICROPY_EVENT_POLL_HOOK \


Shouldn't this be guarded by #if MICROPY_ENABLE_SCHEDULER, to not change what mp_hal_delay_ms does in case scheduler is off?

MICROPY_EVENT_POLL_HOOK is also needed for a uselect branch I'm about to push up, but yes it probably is cleaner to guard this here, even if it's not switched as such in the unix port. Perhaps I should add a similar compile guard there too...

jimmo · 2021-11-18T00:38:21Z

It's within the uncertainty limits of our performance testing on Linux (this is on a "quiet" PC I have set up specifically for this purpose).

$ ./run-perfbench.py -s ~/scheduler-disabled ~/scheduler-enabled 
diff of scores (higher is better)
N=2000 M=2000            /home/jimmo/scheduler-disabled -> /home/jimmo/scheduler-enabled         diff      diff% (error%)
bm_chaos.py                13705.54 ->   13974.58 :    +269.04 =  +1.963% (+/-5.96%)
bm_fannkuch.py                60.85 ->      61.77 :      +0.92 =  +1.512% (+/-1.16%)
bm_fft.py                 111114.84 ->  113617.54 :   +2502.70 =  +2.252% (+/-3.09%)
bm_float.py               248018.07 ->  252007.84 :   +3989.77 =  +1.609% (+/-0.70%)
bm_hexiom.py                 614.69 ->     634.75 :     +20.06 =  +3.263% (+/-1.11%)
bm_nqueens.py             212852.12 ->  219469.13 :   +6617.01 =  +3.109% (+/-3.30%)
bm_pidigits.py              8357.80 ->    8106.97 :    -250.83 =  -3.001% (+/-2.80%)
misc_aes.py                16241.98 ->   16465.14 :    +223.16 =  +1.374% (+/-0.62%)
misc_mandel.py            129784.73 ->  133406.01 :   +3621.28 =  +2.790% (+/-1.51%)
misc_pystone.py            83768.68 ->   85039.39 :   +1270.71 =  +1.517% (+/-1.25%)
misc_raytrace.py           22047.31 ->   21868.16 :    -179.15 =  -0.813% (+/-1.38%)

Just for reference, here's the same thing on STM32F7. Note both cases here are with all uses of the scheduler disabled, so it's really just measuring the cost of having that extra check in the VM hook.

$ ./run-perfbench.py -s ~/scheduler-pybd-disabled ~/scheduler-pybd-enabled
diff of scores (higher is better)
N=100 M=100              /home/jimmo/scheduler-pybd-disabled -> /home/jimmo/scheduler-pybd-enabled         diff      diff% (error%)
bm_chaos.py                  465.99 ->     477.15 :     +11.16 =  +2.395% (+/-0.04%)
bm_fannkuch.py               103.42 ->     101.00 :      -2.42 =  -2.340% (+/-0.05%)
bm_fft.py                   1982.24 ->    1962.05 :     -20.19 =  -1.019% (+/-0.02%)
bm_float.py                 7414.05 ->    7577.25 :    +163.20 =  +2.201% (+/-0.08%)
bm_hexiom.py                  63.28 ->      64.05 :      +0.77 =  +1.217% (+/-0.03%)
bm_nqueens.py               5873.95 ->    5775.81 :     -98.14 =  -1.671% (+/-0.02%)
bm_pidigits.py               993.90 ->     976.57 :     -17.33 =  -1.744% (+/-0.02%)
misc_aes.py                  590.85 ->     589.45 :      -1.40 =  -0.237% (+/-0.06%)
misc_mandel.py              4433.61 ->    4598.75 :    +165.14 =  +3.725% (+/-0.05%)
misc_pystone.py             3082.71 ->    3064.38 :     -18.33 =  -0.595% (+/-0.05%)
misc_raytrace.py             506.87 ->     522.49 :     +15.62 =  +3.082% (+/-0.03%)

(I'm also trying to convince myself we should enable by default on Unix :) )

stinos · 2021-11-18T15:12:41Z

It's within the uncertainty limits of our performance testing on Linux (this is on a "quiet" PC I have set up specifically for this purpose).

Thanks, seems fine. Just wondering: does any of these tests use exceptions heavily? Anyway had a better look at the code and it indeed looks like the impact should be really small.

dpgeorge · 2022-01-22T23:57:15Z

Merged in 30b6ce8

This is fully configurable, and with the new windows variants support can be disabled if needed.

stinos reviewed Nov 17, 2021

View reviewed changes

andrewleech force-pushed the windows-schedule branch from 442b520 to 6952704 Compare November 17, 2021 21:57

andrewleech force-pushed the windows-schedule branch from 6952704 to b3a80b5 Compare November 24, 2021 23:57

andrewleech mentioned this pull request Nov 25, 2021

windows/uasyncio: Add support for uasyncio to windows dev variant. #8030

Merged

andrewleech force-pushed the windows-schedule branch from b3a80b5 to 90625ba Compare November 25, 2021 03:14

windows/schedule: Add micropython.schedule to windows port.

33da7d3

andrewleech force-pushed the windows-schedule branch from 90625ba to 33da7d3 Compare November 25, 2021 04:38

dpgeorge added the port-windows label Nov 29, 2021

dpgeorge closed this Jan 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

windows/schedule: Add micropython.schedule to windows port. #7999

windows/schedule: Add micropython.schedule to windows port. #7999

andrewleech commented Nov 17, 2021

stinos Nov 17, 2021

stinos Nov 17, 2021

stinos Nov 17, 2021

jimmo Nov 17, 2021

andrewleech Nov 17, 2021 •

edited

Loading

dpgeorge Nov 18, 2021

stinos Nov 18, 2021

stinos Nov 17, 2021

andrewleech Nov 17, 2021

jimmo commented Nov 18, 2021

stinos commented Nov 18, 2021

dpgeorge commented Jan 22, 2022

		@@ -201,6 +204,14 @@ extern const struct _mp_obj_module_t mp_module_time;

		#define MICROPY_MPHALPORT_H "windows_mphal.h"

		#define MICROPY_EVENT_POLL_HOOK \

windows/schedule: Add micropython.schedule to windows port. #7999

windows/schedule: Add micropython.schedule to windows port. #7999

Conversation

andrewleech commented Nov 17, 2021

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

andrewleech Nov 17, 2021 • edited Loading

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

jimmo commented Nov 18, 2021

stinos commented Nov 18, 2021

dpgeorge commented Jan 22, 2022

andrewleech Nov 17, 2021 •

edited

Loading