8000 webassembly/api: Allocate code data on C heap when running Python code. by dpgeorge · Pull Request #14318 · micropython/micropython · GitHub
[go: up one dir, main page]
Skip to content

webassembly/api: Allocate code data on C heap when running Python code. #14318

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Apr 24, 2024
Merged

webassembly/api: Allocate code data on C heap when running Python code. #14318

merged 2 commits into from
Apr 24, 2024

Conversation

dpgeorge
Copy link
Member

Otherwise Emscripten allocates it on the Emscripten C stack, which will overflow for large amounts of code.

Fixes issue #14307.

@dpgeorge
Copy link
Member Author

@WebReflection this should fix the issue you are seeing when trying to run large code strings.

@dpgeorge
Copy link
Member Author
dpgeorge commented Apr 17, 2024 •
edited
Loading

Find a compiled version of this PR here for testing: https://micropython.org/resources/wasm/micropython.mjs (also has https://micropython.org/resources/wasm/micropython.wasm).

WebReflection
WebReflection reviewed Apr 17, 2024
View reviewed changes
ports/webassembly/api.js
return proxy_convert_mp_to_js_obj_jsside_with_free(value);
},
runPythonAsync(code) {
const len = Module.lengthBytesUTF8(code);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if len always needs a + 1 (for null code \x00 I suppose) why isn't this just const len = Module.lengthBytesUTF8(code) + 1; instead? Not a blocker, just a silly question!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Module.stringToUTF8 will always add a null byte at the end, so there must be enough room for it.

But, below in the call to mp_js_do_exec_async, we pass just len.

@WebReflection
Copy link
WebReflection commented Apr 17, 2024 •
edited
Loading

I can confirm this fixes the presented use-case/issue we had 🙏

index.txt

rename this file as index.html in your test folder, download both https://micropython.org/resources/wasm/micropython.mjs and https://micropython.org/resources/wasm/micropython.wasm in it, change eventually the host port in the <mpy-config> if different from 8080, bootstrap a server and see no issue whatsoever, with the page showing a Thank You! text in the body.

@ntoll
Copy link
Contributor
ntoll commented Apr 17, 2024

Bravo

@dpgeorge dpgeorge added this to the release-1.23.0 milestone Apr 19, 2024
@dpgeorge dpgeorge force-pushed the webassembly-allow-large-strings-to-run-python branch from 542fd1c to a5ad308 Compare April 24, 2024 02:37
@codecov Codecov
Copy link
codecov bot commented Apr 24, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.39%. Comparing base (49ce7a6) to head (7c7e07e).

❗ Current head 7c7e07e differs from pull request most recent head 9c7f065. Consider uploading reports for the commit 9c7f065 to get more accurate results

Additional details and impacted files 
@@           Coverage Diff           @@
##           master   #14318   +/-   ##
=======================================
  Coverage   98.39%   98.39%           
=======================================
  Files         161      161           
  Lines       21204    21204           
=======================================
  Hits        20864    20864           
  Misses        340      340

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@github-actions GitHub Actions
Copy link

Code size report:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64:    +0 +0.000% standard
      stm32:    +0 +0.000% PYBV10
     mimxrt:    +0 +0.000% TEENSY40
        rp2:    +0 +0.000% RPI_PICO
       samd:    +0 +0.000% ADAFRUIT_ITSYBITSY_M4_EXPRESS

@dpgeorge dpgeorge force-pushed the webassembly-allow-large-strings-to-run-python branch from e4d6a16 to 7c7e07e Compare April 24, 2024 03:02
dpgeorge added 2 commits April 24, 2024 13:15
@dpgeorge
webassembly/api: Fix waiting for Emscripten module to be loaded.
45848f7 
In modularize mode, the `_createMicroPythonModule()` constructor must be
await'ed on, before `Module` is ready to use.

Signed-off-by: Damien George <damien@micropython.org>
@dpgeorge
webassembly/api: Allocate code data on C heap when running Python code.
9c7f065 
Otherwise Emscripten allocates it on the Emscripten C stack, which will
overflow for large amounts of code.

Fixes issue micropython#14307.

Signed-off-by: Damien George <damien@micropython.org>
@dpgeorge dpgeorge force-pushed the webassembly-allow-large-strings-to-run-python branch from 7c7e07e to 9c7f065 Compare April 24, 2024 03:16
@dpgeorge dpgeorge merged commit 9c7f065 into micropython:master Apr 24, 2024
7 checks passed
@dpgeorge
Copy link
Member Author

Also added a fix for initialising Module with the latest Emscripten.

@dpgeorge dpgeorge deleted the webassembly-allow-large-strings-to-run-python branch April 24, 2024 03:21
@WebReflection
Copy link

@dpgeorge do you mind publishing this latest version to npm so we can also bump it? Thanks!

@WebReflection WebReflection mentioned this pull request Apr 24, 2024
Merged
3 tasks
@dpgeorge
Copy link
Member Author

The latest master with this PR merged is available here: https://www.npmjs.com/package/@micropython/micropython-webassembly-pyscript/v/1.22.0-335

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@WebReflection WebReflection WebReflection left review comments

Assignees
No one assigned
Labels
port-webassembly
Projects
None yet
Milestone
release-1.23.0
Development

Successfully merging this pull request may close these issues.
3 participants
@dpgeorge @WebReflection @ntoll
0