Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wasm-tbb: increase stack size and add CI #1079

Merged
merged 12 commits into from
Dec 1, 2024
Merged

wasm-tbb: increase stack size and add CI #1079

merged 12 commits into from
Dec 1, 2024

Conversation

pca006132
Copy link
Collaborator

@pca006132 pca006132 commented Nov 28, 2024

Now run in "normal" CI as well with npm test.

Also updates dependency versions.

@pca006132
Copy link
Collaborator Author

Emscripten may soon fix the vite issue: emscripten-core/emscripten#22394

Copy link

codecov bot commented Nov 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.72%. Comparing base (11387fb) to head (7754c07).
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1079   +/-   ##
=======================================
  Coverage   91.72%   91.72%           
=======================================
  Files          30       30           
  Lines        5910     5910           
=======================================
  Hits         5421     5421           
  Misses        489      489           

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

@pca006132
Copy link
Collaborator Author

hmmm, the vite thing is annoying, I guess it is due to emscripten version issue...

@pca006132
Copy link
Collaborator Author

pca006132 commented Nov 28, 2024

And it seems that emscripten pthread support is not that stable for 3.1.64 ...

Copy link
Owner

@elalish elalish left a comment

Choose a reason for hiding this comment

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

So, is this one ready? Should I update our branch rules so we can merge it?

@elalish
Copy link
Owner

elalish commented Nov 29, 2024

Is this PR intended to fix this problem? https://github.com/elalish/manifold/actions/runs/12076790849/job/33678681856

@pca006132
Copy link
Collaborator Author

So, is this one ready? Should I update our branch rules so we can merge it?

Branch rule: Yes, the name should be stable.

I am not entirely sure if I want to experiment with newer versions of emscripten or not. iirc newer version runs fine in the CI (with only a few runs though, not sure if it is a lot more stable than what we have now), but because nixos haven't yet support the newer versions of emscripten, I will need to use it inside a container to figure out how to patch the js file to make vite happy.

Currently, I simply avoid running the tests on the CI, and just treat this as a compilation test, because we know that it is unstable for the emscripten version we have tested.

@pca006132
Copy link
Collaborator Author

Hmmm, for some reason vite did not complain about workerOptions issue with emscripten 3.1.73 when run inside a docker, idk what happened. I think we can merge this for now.

@pca006132
Copy link
Collaborator Author

But apparently 3.1.73 works fine without issue, at least on my machine. 3.1.61/64 will sometimes produce the issue.

@pca006132
Copy link
Collaborator Author

But apparently 3.1.73 works fine without issue, at least on my machine. 3.1.61/64 will sometimes produce the issue.

May break when I add some assertions, very non-deterministic. And sadly emscripten does not support address sanitizer on wasm workers for now...

@pca006132
Copy link
Collaborator Author

Added warnings to the readme for enabling parallelization with emscripten, as well as changed the emscripten CMAKE_BUILD_TYPE to MinSizeRel. This shrinks the size of the wasm from 559K to 470K for emscripten 3.1.64.

@elalish elalish merged commit 2162d8d into master Dec 1, 2024
25 checks passed
@elalish elalish deleted the emscripten-tbb branch December 1, 2024 07:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants