Remove mp_js_stdout requirement in webassembly port #9752

ntoll · 2022-10-26T16:30:45Z

This PR introduces four changes:

Remove need for an element with the id mp_js_stdout in the DOM.
Dispatch the print event via the document object.
Namespace the event as micropython-print.
More efficient compilation flag (-Os).

dpgeorge · 2022-10-27T02:08:05Z

ports/webassembly/Makefile


 CFLAGS += -std=c99 -Wall -Werror -Wdouble-promotion -Wfloat-conversion
-CFLAGS += -O3 -DNDEBUG
+CFLAGS += -Os -DNDEBUG


What's the reason behind this change?

As far as I'm aware, when using -s ASYNCIFY (which you may not actually want/need?), you should enable -O3 otherwise the generated code is too slow.

See https://emscripten.org/docs/porting/asyncify.html#sleeping-yielding-to-the-event-loop, it says:

It’s very important to optimize (-O3 here) when using Asyncify, as unoptimized builds are very large.

But reading that carefully maybe they mean that you should use some level of optimisation, and maybe -Os is indeed better.

If we switch to -Os we need to check:

change in size of generated output

change in performance/execution speed, possibly measured by the performance benchmark test suite

both for WASM and non-WASM output, ie -s WASM=0)

both for ASYNCIFY enabled and disabled

I was experimenting to see how size of the resulting firmware was changed (see discussion on discord). However, "experimenting" is too grand a word... more like blundering around. This change is the lowest possible priority from my point of view.

I had a look at this, here are my results:

flags firmware.wasm micropython.js perf -O3 -s ASYNCIFY 1342003 212845 0 (baseline) -O3 -s ASYNCIFY -s WASM=0 - 7064750 -30% -O3 367131 196569 +140% -O3 -s WASM=0 - 2818260 +30% -Os -s ASYNCIFY 1135450 213064 +40% -Os -s ASYNCIFY -s WASM=0 - 6239768 -30% -Os 295028 196569 +180% -Os -s WASM=0 - 2271358 +30%

The first row is current master. The second and third columns show firmware size (you need to add them to get the total size). The fourth column shows the approximate change in performance compared to the master/baseline.

The performance was measured using run-perfbench.py. The error was large, like up to 20%. But still you could see general trends.

In summary:

using -Os instead of -O3 makes it a little bit faster in all cases, and smaller output (wasm/js) in all cases

ASYNCIFY makes the output much bigger, and run much slower

WASM=0 makes the output much bigger, and run much slower

So I think we should go with this change to -Os, it's good all round!

And we should probably add a comment that ASYNCIFY only be used if necessary.

Aha... I was meaning to take a look into this (but didn't know how, so thanks for the heads up).

LGTM.

dlech · 2022-10-27T02:30:26Z

ports/webassembly/library.js

-                var print = new Event('print');
-                print.data = c;
-                mp_js_stdout.dispatchEvent(print);
+                var printEvent = new Event('micropython-print');


Should we use CustomEvent here?

Hmm... depends on the context.

The CustomEvent interface represents events initialized by an application for any purpose.

(from MDN)

Is MicroPython an "application"..? My intuition is CustomEvent is more appropriate from a purely semantic point of view.

Shall we use CustomEvent then?

Yes. That's the semantically correct object to use. I'll change/push.

dlech · 2022-10-27T02:34:23Z

ports/webassembly/library.js

-                mp_js_stdout.dispatchEvent(print);
+                var printEvent = new Event('micropython-print');
+                printEvent.data = c;
+                document.dispatchEvent(printEvent);


Should we change the check for typeof window === 'undefined' above to check document instead?

Do we also want to support web workers that don't have a document or window object?

Yeah... so running MicroPython in web-workers is the next blip on my radar. ;-) There are ways to detect if something is running inside a web worker (in which case we should postMessage(foo) instead?). I guess yet another branch in this conditional.

I don't think we should assume that stdout should be tied to postMessage in a web worker. Someone might want to keep the stdout in the worker thread.

Thinking about it more, using an event probably makes sense in nodejs as well so that stdout from MicroPython could be redirected somewhere else besides stdout of nodejs.

So maybe no branches are needed?

It might also be more efficient to collect all characters into one event, rather than having a single event per character. And better to make that change now, along with this event change, since all these changes are backwards incompatible.

So the idea would be:

remove mp_js_stdout DOM element

remove special case for node, just always emit an event (then when running node it hooks the event to forward the data to process.stdout)

put all characters in the one event

I know that's creeping the scope of this PR, but I think it's the right direction to move in. Let me know what you think @ntoll.

I agree this is the right direction.

I done in this PR.

I think we should proceed with care and wrap this in another PR. There is yet another sort of behaviour to account for: when MicroPython is run within a web worker context (i.e. we're in a browser, but there's no browser object to use for dispatching events).

This makes sense, but should probably be done in a separate PR just to keep things tidy/avoid scope creep.

I forgot to mention I'm currently at a conference/travelling, but can look at 2 and 3 next week. (Greetings from Sweden 🇸🇪 )

ntoll · 2022-10-27T11:28:26Z

So, this PR can safely be ignored in terms of the code changes. But the discussions here will, I hope, point to a better way forward. 👍

dpgeorge · 2022-11-07T06:22:50Z

OK, merged in db978d7 with corresponding updates to the README in 1ed740b, and the change to -Os split out into a separate commit 5987130 with info in the git commit about the profiling/testing I did.

dlech · 2022-11-07T15:11:56Z

ports/webassembly/library.js

+                var printEvent = new CustomEvent('micropython-print');
+                printEvent.data = c;


CustomEvent already has a detail property that should be used instead of adding a custom property.

Suggested change

var printEvent = new CustomEvent('micropython-print');

printEvent.data = c;

var printEvent = new CustomEvent('micropython-print', { detail: c });

This was already merged. That change will need to go in a new PR (maybe @ntoll will get to it with the above-discussed improvements).

Explicitly invalidate cache lines on RP2350

ntoll force-pushed the wasm-startup branch 2 times, most recently from c0690a2 to f4eff03 Compare October 26, 2022 16:41

dpgeorge reviewed Oct 27, 2022

View reviewed changes

dpgeorge added the port-webassembly label Oct 27, 2022

dlech reviewed Oct 27, 2022

View reviewed changes

webassembly: Dispatch micropython-print via document not mp_js_stdout.

bfcf63e

ntoll force-pushed the wasm-startup branch from f4eff03 to bfcf63e Compare November 4, 2022 07:23

dpgeorge closed this Nov 7, 2022

dlech reviewed Nov 7, 2022

View reviewed changes

dlech mentioned this pull request Nov 9, 2022

ports/webassembly/library: Make use of CustomEvent detail property. #9904

Closed

RetiredWizard pushed a commit to RetiredWizard/micropython that referenced this pull request Nov 5, 2024

Merge pull request micropython#9752 from tannewt/rp2350_invalidate

0ebbe97

Explicitly invalidate cache lines on RP2350

		var printEvent = new CustomEvent('micropython-print');
		printEvent.data = c;

Uh oh!

Remove mp_js_stdout requirement in webassembly port #9752

Remove mp_js_stdout requirement in webassembly port #9752

Uh oh!

Conversation

ntoll commented Oct 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

dlech Oct 27, 2022 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ntoll commented Oct 27, 2022

Uh oh!

dpgeorge commented Nov 7, 2022

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!

3 participants

ntoll commented Oct 26, 2022 •

edited

Loading

dlech Oct 27, 2022 •

edited

Loading