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

Fix wasm issue with multiple instantiation #4

Merged
merged 2 commits into from
Mar 22, 2024

Conversation

EmNudge
Copy link
Collaborator

@EmNudge EmNudge commented Mar 21, 2024

This was all we needed to fix it, it seems!

I've moved all code outside of the function call itself so there's no chance for multiple calls to somehow result in multiple instantiations of the same wasm binary.

The test I added would not pass on the old version.

Closes #3

@kdheepak
Copy link
Owner

kdheepak commented Mar 22, 2024

This looks good to me!

If I understand correctly, I think there is still a case when this will cause issues though, right? With code like this:

let first = processor.process(`\`\`\`svgbob\nn\n\`\`\``);
let second = processor.process(`\`\`\`svgbob\nn\n\`\`\``);

await first;
await second;

But I'm fine with dealing with that if we run into that problem.

@kdheepak kdheepak merged commit 29ec476 into kdheepak:main Mar 22, 2024
@EmNudge
Copy link
Collaborator Author

EmNudge commented Mar 22, 2024

That was my original understanding of the issue which is why I started checking the allocator and memory size.

However, it seemed it was actually related to there being multiple calls to WebAssembly.instantiate() with the same buffer. I don't actually know why this was a problem - I can't find docs of it anywhere. However, the tests we run and the remark plugin are ran in weird contexts whereby this was happening and causing a very strange non-recoverable crash. This issue wouldn't happen normally, even with multiple calls to the same buffer.

The 2nd test there could actually just have a single test and it would have still failed (since the first test was the first instantiation), but I added a bunch of process calls just in case the memory issue I thought of actually becomes a problem at some point.

The one case where it might be a problem is if we somehow use up 272kb (17 pages of 16kb) of memory, which I think is generally unlikely considering Rust cleans up after itself, so subsequent calls shouldn't be a problem. It's only when rendering some extremely massive SVG (which I don't think is ever likely)

@kdheepak
Copy link
Owner

If the current code, if you call the function and then call await later, wouldn't multiple calls to instantiate happen? It might be a race condition type of situation but it is low probability so I think we can refactor it later.

@EmNudge
Copy link
Collaborator Author

EmNudge commented Mar 22, 2024

Not in the current code.
The WASM API was

const render = await getRenderer() // instatiate wasm
render("\`\`\`svgbob\nn\n\`\`\`") // call wasm export

But was changed to

const render = await getRenderer() // wait for instatiate wasm to be finished
render("\`\`\`svgbob\nn\n\`\`\`") // call wasm export

so multiple calls to

await processor.process(`\`\`\`svgbob\nn\n\`\`\``)

Are guaranteed to not instantiate the wasm multiple times. I thought it was guaranteed before since I used a local variable to check, but that wasn't enough for some reason (still a bit blurry on this one).

Wasm is now instantiated upon import. Once you import the module, it has begun. Calling the getRenderer function only waits for it to complete, but it doesn't itself do any work. If it has already been instantiated it would be like

export const getRenderer = async () => {
  if (hasLoaded) return render;

  await Promise.resolve();
  
  return render;
};

We can await a resolved promise an infinite amount of times without issues.

@kdheepak
Copy link
Owner

Of course! I misread the usage, my bad. Thanks for clarifying!

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.

Wasm OOM error
2 participants