Skip to content

Comments

Reduce initial WASM memory and auto-grow it#122

Merged
khaledhosny merged 2 commits intoharfbuzz:mainfrom
chearon:ch/117
Apr 7, 2025
Merged

Reduce initial WASM memory and auto-grow it#122
khaledhosny merged 2 commits intoharfbuzz:mainfrom
chearon:ch/117

Conversation

@chearon
Copy link
Contributor

@chearon chearon commented Apr 6, 2025

Emscripten updates the views it exposes (Module.HEAPU8 etc) whenever memory resizes so, compiler flags aside, all this does is remove aliasing. We can still have aliases, but they have to be re-created after any calls that could have resized memory. It's easier and safer to just write the whole thing out, and JS engines are really good at inline caching anyways.

Fixes #117

chearon added 2 commits April 6, 2025 19:14
hb_sets are always u32s, so there's no need for this to try and
support anything else

(this is prep work for making the wasm memory resizeable)
@khaledhosny khaledhosny merged commit 9736ccf into harfbuzz:main Apr 7, 2025
1 check passed
@khaledhosny
Copy link
Contributor

Thanks!

@yisibl
Copy link
Contributor

yisibl commented Apr 7, 2025

Does build-subset.sh need to be tweaked as well?

@khaledhosny
Copy link
Contributor

Does build-subset.sh need to be tweaked as well?

If I do so, I get the following when trying to run node examples/hb-subset.example.node.js, so it needs someone to investigate it:

node:internal/process/promises:394
    triggerUncaughtException(err, true /* fromPromise */);
    ^

[TypeError: WebAssembly.instantiate(): Imports argument must be present and must be an object]

@jlarmstrongiv
Copy link

@khaledhosny oh, that’s the same error I ran into with the regular hb.wasm instantiation in #124

It seems like harfbuzzjs 4.x is broken in Node.js

@chearon
Copy link
Contributor Author

chearon commented Apr 8, 2025

Does build-subset.sh need to be tweaked as well?

It does not generate a wrapper file currently so we would need to do that first.

I see that the README shows fetching .wasm and instantiating manually, but some emscripten features like the one this PR added rely on the wrapper file. I think we should only support loading it through the wrapper, like the tests and examples do.

@khaledhosny oh, that’s the same error I ran into with the regular hb.wasm instantiation in #124

To be clear, examples/hb-subset.example.node.js runs fine currently. @khaledhosny was showing what happens if you add -sALLOW_MEMORY_GROWTH to build-subset.sh and then try to run it.

@khaledhosny
Copy link
Contributor

I see that the README shows fetching .wasm and instantiating manually, but some emscripten features like the one this PR added rely on the wrapper file. I think we should only support loading it through the wrapper, like the tests and examples do.

Please submit a PR that updates the README file. I think I attempted to do that once, but I don’t remember why I didn’t.

@chearon
Copy link
Contributor Author

chearon commented Apr 9, 2025

Please submit a PR that updates the README file. I think I attempted to do that once, but I don’t remember why I didn’t.

Trying to. That example uses require, so would need a bundler. I tried converting that example to require('harfbuzzjs') (-> index.js) and bundling with Vite and Bun but they just don't work with commonjs. I did get Vite and Bun to work with the "advanced" method I just posted here which I will also add to the README. I'll try webpack next, but that takes more setup.

To get the simple index.js import working in Vite and Bun, probably the first step is to convert everything to use ESM, which the JS world has mostly moved to anyways.

@jlarmstrongiv
Copy link

To get the simple index.js import working in Vite and Bun, probably the first step is to convert everything to use ESM, which the JS world has mostly moved to anyways.

ESM would be nice 👍

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.

INITIAL_MEMORY is too large.

4 participants