refactor(wasm/examples): support export models without web worker #822

hi-ogawa · 2024-05-24T03:13:41Z

Hi, just started to looking through the code and it's very interesting project! Thanks for integrating Vitest here.

This is one idea I had to restructure current worker.ts to be free from Web worker integration. It's possible that I missed something important in terms of overall architecture, but I thought it's still worth sharing what I was thinking with the concrete code.

Essentially the idea is to expose exportModels directly as function without going through postMessage/onmessage. I also found code (CAD code?) evaluation can be extracted as utility, so I added things like evaluateCADToModel.

Please let me know if this approach aligns with what you had in mind and also whether you see it as an improvement of code base. If so, I can try to make it to finish line with this approach.

google-cla · 2024-05-24T03:13:46Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

elalish

Thank you! Yes, I think this is definitely an improvement. And thank you for Vitest - Vite and Vitest have dramatically improved my TS developer experience.

elalish · 2024-05-24T16:25:11Z

bindings/wasm/examples/worker.test.js

@@ -85,7 +85,8 @@ async function runExample(name) {
 }

 suite('Examples', () => {
-  test('Intro', async () => {
+  // TODO: somehow "Intro" is actually fine with vitest web-worker?


I noticed this too, but it makes no sense to me. I was actually wondering if that was being skipped somehow.

Hmm, it looks like TypeError: Invalid URL error happens when running multiple examples. When running a single test like npm run test worker.test.ts -- -t Rounded, it actually goes fine:

$ npm run test worker.test -- -t Rounded > [email protected] test > vitest run worker.test -t Rounded RUN v1.6.0 /home/hiroshi/code/others/manifold/bindings/wasm/examples ✓ worker.test.js (9) 9330ms ✓ Examples (9) 9330ms ↓ Intro [skipped] ↓ Tetrahedron Puzzle [skipped] ✓ Rounded Frame 9330ms

Maybe @vitest/web-worker might not be that bad after all, but personally investigating this further feels tougher than rewriting it without @vitest/web-worker.

elalish · 2024-05-24T16:26:02Z

bindings/wasm/examples/worker-wrapper.ts

+
+self.onmessage = async (e) => {
+  try {
+    const result = await evaluateCADToModel(e.data);


Agreed, this looks like a better architecture.

elalish · 2024-05-24T16:28:07Z

bindings/wasm/examples/worker.ts

+  return { globalDefaults, manifold };
+}
+
+export async function evaluateCADToModel(code: string) {


I like this because we can probably publish this to npm, as it's more general purpose than our worker was.

hi-ogawa

Thanks for the review. I'm glad you liked it!

hi-ogawa · 2024-05-25T05:52:43Z

bindings/wasm/examples/worker.test.js

@@ -85,7 +85,8 @@ async function runExample(name) {
 }

 suite('Examples', () => {
-  test('Intro', async () => {
+  // TODO: somehow "Intro" is actually fine with vitest web-worker?


Hmm, it looks like TypeError: Invalid URL error happens when running multiple examples. When running a single test like npm run test worker.test.ts -- -t Rounded, it actually goes fine:

$ npm run test worker.test -- -t Rounded > [email protected] test > vitest run worker.test -t Rounded RUN v1.6.0 /home/hiroshi/code/others/manifold/bindings/wasm/examples ✓ worker.test.js (9) 9330ms ✓ Examples (9) 9330ms ↓ Intro [skipped] ↓ Tetrahedron Puzzle [skipped] ✓ Rounded Frame 9330ms

Maybe @vitest/web-worker might not be that bad after all, but personally investigating this further feels tougher than rewriting it without @vitest/web-worker.

hi-ogawa · 2024-05-25T06:10:30Z

bindings/wasm/examples/worker2.test.ts

+  test('Torus Knot', async () => {
+    const result = await runExample('Torus Knot');
+    expect(result.genus).to.equal(1, 'Genus');
+    expect(result.volume).to.be.closeTo(20786, 1, 'Volume');
+    expect(result.surfaceArea).to.be.closeTo(11177, 1, 'Surface Area');
+  });


I'm seeing something odd here. When running this test alone, it's passing, but when running it together with Rounded Frame, it fails. I'm wondering if this is because I forgot to cleanup some resource in between tests. (test duration seems also affected depending on how tests are run.)

$ npm run test worker2.test -- -t 'Torus' ... ✓ worker2.test.ts (9) 6646ms ✓ Examples (9) 6644ms ... ✓ Torus Knot 6642ms

$ npm run test worker2.test -- -t 'Rounded|Torus' RUN v1.6.0 /home/hiroshi/code/others/manifold/bindings/wasm/examples ❯ worker2.test.ts (9) 28180ms ❯ Examples (9) 28178ms ... ✓ Rounded Frame 8700ms × Torus Knot 19477ms ⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯ Failed Tests 1 ⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯ FAIL worker2.test.ts > Examples > Torus Knot AssertionError: Volume: expected 20960.73046875 to be close to 20786 +/- 1 ❯ worker2.test.ts:79:33 77| const result = await runExample('Torus Knot'); 78| expect(result.genus).to.equal(1, 'Genus'); 79| expect(result.volume).to.be.closeTo(20786, 1, 'Volume'); | ^ 80| expect(result.surfaceArea).to.be.closeTo(11177, 1, 'Surface Area'); 81| });

I would appreciate if you can spot something I did wrong. I mostly copied worker.test.ts but maybe one notable difference is that previously it has a separate Module initialization and it's used to instantiate Manifold

manifold/bindings/wasm/examples/worker.test.js

Lines 27 to 28 in 81454c2

const wasm = await Module();

wasm.setup();

manifold/bindings/wasm/examples/worker.test.js

Lines 71 to 73 in 81454c2

const manifold = wasm.Manifold(mesh);

const prop = manifold.getProperties();

const genus = manifold.genus();

Good catch - I was neglecting to reset our static properties, which was probably creating a similar bug in our app. The old test wasn't catching this because it created and destroyed a worker for each test (unlike how the app works), so this new architecture is definitely better.

…-web-worker

codecov · 2024-05-28T20:04:05Z

Codecov Report

Attention: Patch coverage is 0% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 89.34%. Comparing base (d437097) to head (9584173).
Report is 33 commits behind head on master.

Files	Patch %	Lines
src/utilities/include/public.h	0.00%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #822      +/-   ##
==========================================
- Coverage   91.84%   89.34%   -2.50%     
==========================================
  Files          37       61      +24     
  Lines        4976     8570    +3594     
  Branches        0      939     +939     
==========================================
+ Hits         4570     7657    +3087     
- Misses        406      913     +507

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

elalish

Thanks for the help on this! I think it's all working well now.

hi-ogawa added 4 commits May 24, 2024 11:38

refactor(wasm/examples): support evaluateCADToModel without web worker

043fc22

test: copy test

8b31ada

chore: update vitest

92be22b

chore: revert some

0b49e52

hi-ogawa added 5 commits May 24, 2024 12:21

chore: revert more

76be330

refactor: simplify worker-wrapper.ts

d0c18b3

chore: revert format

14889b8

test: fix cleanup

a620d5f

refactor: single module

ac44834

elalish reviewed May 24, 2024

View reviewed changes

hi-ogawa added 7 commits May 25, 2024 13:14

chore: format

099834f

chore: revert .vscode/settings.json

3a844e6

chore: format

9535d3c

test: tweak

be0f7d6

chore: format

612517c

chore: rename

81454c2

refactor: minor

ef9eea2

hi-ogawa commented May 25, 2024

View reviewed changes

hi-ogawa and others added 3 commits May 25, 2024 15:41

chore: remove wrong lock file

58ee4ef

Merge branch 'master' of github.com:elalish/manifold into refactor-js…

12a5b0a

…-web-worker

added resetToCircularDefaults

425fbb0

elalish marked this pull request as ready for review May 28, 2024 19:22

elalish added 2 commits May 28, 2024 12:51

cleanup

0ef7555

added headers

a14b224

removed vitest/web-worker

9584173

elalish mentioned this pull request May 28, 2024

Vitest update problem repro #808

Closed

elalish approved these changes May 28, 2024

View reviewed changes

elalish merged commit 4fb3520 into elalish:master May 28, 2024
20 checks passed

hi-ogawa deleted the refactor-js-web-worker branch May 28, 2024 23:43

elalish mentioned this pull request Jun 12, 2024

v2.5 #836

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(wasm/examples): support export models without web worker #822

refactor(wasm/examples): support export models without web worker #822

hi-ogawa commented May 24, 2024 •

edited

Loading

google-cla bot commented May 24, 2024

elalish left a comment

elalish May 24, 2024

hi-ogawa May 25, 2024

elalish May 24, 2024

elalish May 24, 2024

hi-ogawa left a comment

hi-ogawa May 25, 2024

hi-ogawa May 25, 2024

elalish May 28, 2024

codecov bot commented May 28, 2024 •

edited

Loading

elalish left a comment

	const manifold = wasm.Manifold(mesh);
	const prop = manifold.getProperties();
	const genus = manifold.genus();

refactor(wasm/examples): support export models without web worker #822

refactor(wasm/examples): support export models without web worker #822

Conversation

hi-ogawa commented May 24, 2024 • edited Loading

google-cla bot commented May 24, 2024

elalish left a comment

Choose a reason for hiding this comment

elalish May 24, 2024

Choose a reason for hiding this comment

hi-ogawa May 25, 2024

Choose a reason for hiding this comment

elalish May 24, 2024

Choose a reason for hiding this comment

elalish May 24, 2024

Choose a reason for hiding this comment

hi-ogawa left a comment

Choose a reason for hiding this comment

hi-ogawa May 25, 2024

Choose a reason for hiding this comment

hi-ogawa May 25, 2024

Choose a reason for hiding this comment

elalish May 28, 2024

Choose a reason for hiding this comment

codecov bot commented May 28, 2024 • edited Loading

Codecov Report

elalish left a comment

Choose a reason for hiding this comment

hi-ogawa commented May 24, 2024 •

edited

Loading

codecov bot commented May 28, 2024 •

edited

Loading