NotificationsYou must be signed in to change notification settings
Fork3.8k
Star8.4k

wasm disable mutex usage, wasm CI updates#3203

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.

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

Already on GitHub?Sign in to your account

Jump to bottom

Merged

Starbuck5 merged 10 commits intomainfromankith26-wasm

Jun 18, 2022

Merged

wasm disable mutex usage, wasm CI updates#3203

Starbuck5 merged 10 commits intomainfromankith26-wasm

Jun 18, 2022

Conversation

Copy link

Contributor

ankith26 commentedMay 29, 2022•
edited
Loading

I added a mutex for event module in#3177 and this was not guarded on emscripten. This PR fixes this and also comments unneeded mutex create/destroy in imageext module.

The PR also makes it so thatset_timer always fails on WASM now (because threading is an issue on WASM). This needs more work in the future, and possibly an alternate implementation to work better with the constraints of browsers.

Following#3185, this makes WASM CI run only on source changes.

I (along with a lot of help from@pmp-p) also changed the CI to usepython-wasm-sdk prebuilts to save a lot of compile time on pygame CI, and added some cython (latest) build caching too. After this PR, our WASM builds should take around a minute, previously it took anywhere between 25 and 40 minutes (usually averaging out at 30 min)

Copy link

ContributorAuthor

ankith26 commentedMay 29, 2022

This might be a good place to make more WASM CI improvements like caching.

@pmp-p please do review this PR

pmp-p approved these changes

May 29, 2022

View reviewed changes

pmp-p reviewed

Jun 3, 2022

View reviewed changes

.github/workflows/build-emsdk.yml OutdatedShow resolvedHide resolved

Ankithand others added6 commits

June 6, 2022 23:05

Disable set_timer completely on wasm for now

ad2cd3e

disable event.c mutex usage on wasm

1802eb6

Comment out unused mutex in imageext.c

7ed2d7f

Add cython_only option to setup.py

4eeffc0

wasm CI: run only when needed, use sdk prebuilts

24cea5c

Co-Authored-By: Paul m. p. Peny <16009100+pmp-p@users.noreply.github.com>

Use version 0.0.2 of wasm-sdk, use /opt paths

6efd593

pmp-p reviewed

Jun 9, 2022

View reviewed changes

.github/workflows/build-emsdk.yml OutdatedShow resolvedHide resolved

Bump wasm sdk version to 0.2.0, artifact uploads

02d7edd

Pass EMCC_CFLAGS, bump cython commit, generate libpygame.a too

pmp-p reviewed

Jun 9, 2022

View reviewed changes

.github/workflows/build-emsdk.yml OutdatedShow resolvedHide resolved

Fix libpygame.a build and cython caching

b5dc50f

Copy link

Contributor

pmp-p commentedJun 10, 2022•
edited
Loading

Great ! CI seems to work very fast.
but i noted that in "Build WASM wtih emsdk" stage emcc.py still want to rebuildzlib, libpng, libjpeg, sdl2 which is not much but if we could find why : it could save more computing power and time.

note : could they been non-pic ones ?https://github.com/pygame-web/python-wasm-sdk/blob/30145fd56a48c1608dae65c1cdf156f99467ea9b/scripts/emsdk-fetch.sh#L80

pmp-p approved these changes

Jun 10, 2022

View reviewed changes

pmp-p approved these changes

Jun 10, 2022

View reviewed changes

pmp-p reviewed

Jun 10, 2022

View reviewed changes

.github/workflows/build-emsdk.yml OutdatedShow resolvedHide resolved

Use -j3 on setup.py build

99e1422

pmp-p approved these changes

Jun 10, 2022

View reviewed changes

pmp-p reviewed

Jun 13, 2022

View reviewed changes

.github/workflows/build-emsdk.yml OutdatedShow resolvedHide resolved

Use sdk 0.3.3, lz4 decompression

38e7c34

pmp-p approved these changes

Jun 14, 2022

View reviewed changes

MyreMylar approved these changes

Jun 14, 2022

View reviewed changes

Copy link

Contributor

MyreMylar left a comment

Choose a reason for hiding this comment

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

LGTM on a read through. Tricky for me to test it fully right now, so I'm leaving the details in the capable hands of our emscripten expert.

Starbuck5 approved these changes

Jun 18, 2022

View reviewed changes

Copy link

Contributor

Starbuck5 left a comment

Choose a reason for hiding this comment

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

Everything looks good on read through

Tricky for me to test it fully right now, so I'm leaving the details in the capable hands of our emscripten expert.

Same, same.

illume added the bug label

Aug 28, 2022

illume reviewed

Aug 28, 2022

View reviewed changes

src_c/imageext.c


		#ifdefWITH_THREAD
		/*
		static void

Copy link

Member

illumeAug 28, 2022

Choose a reason for hiding this comment

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

Probably this commented code should have been removed.

Copy link

Member

illume commentedAug 28, 2022

I thought threads were supported on emscripten? I vaguely remember using them a long time ago...

Copy link

Contributor

pmp-p commentedAug 28, 2022•
edited
Loading

@illume in a way they are, but using them means crashes and most important losing dynamic linking ( but we still want numpy/pymunk and maybe others as shared modules and hopefully built and tested by upstream )

Copy link

Member

illume commentedAug 28, 2022

Ok. So I guess the guard should be slightly different to account for emscripten with threads?
(Please correct me if I'm wrong, but it seems like a build with threads should be possible.)

Do you have any links re the crashes or losing dynamic linking?

Copy link

Contributor

pmp-p commentedAug 28, 2022•
edited
Loading

it'spossible but at the moment dynamic linking with preload plugins is quite broken on emscripten ( probably missing trampolines for 64bits return values from js ) : it happens quite often and patience is the key. I don't intend to even try threading again until wasm64 is out, stable and pyodide has battle tested against its sci-stack.

Also actually using threading would mean lose real time input and sound fx (not music i've found a way) since to allow use spinlock+atomics in pygame thread it would have to run in a worker unless i'm wrong and a correct example is provided.

old mobile devices dont like workers and atomics at all so i prefer actual solution to get wider adoption.

imho threading + simd/neon should come last because it's the next stage of evolution and rely too much on host : webgpu and proper sound come first.