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

Use default allocator instead of wee_alloc in numbat-wasm #296

Merged
merged 1 commit into from Jan 29, 2024

Conversation

triallax
Copy link
Collaborator

@triallax triallax commented Jan 22, 2024

Crate has major unresolved issues such as memory leaks (rustwasm/wee_alloc#106). Its last commit at the moment was made shy of 3 years ago, and the latest release is over 4 years old. I see no reason to keep using it in light of this.

Note: untested

Crate has major unresolved issues such as memory leaks
(rustwasm/wee_alloc#106). Its last commit at
the moment was made shy of 3 years ago, and the latest release is over 4
years old. I see no reason to keep using it in light of this.
@sharkdp
Copy link
Owner

sharkdp commented Jan 24, 2024

The main reason for using wee_alloc is the file size of the WASM output. How much bigger is the generated file?

@eminence
Copy link
Contributor

Unless I'm doing something wrong, the wee_alloc build is 270 bytes bigger

achin@bbx in numbat/numbat-wasm (4efd0ea) is 📦 v0.1.0 🦀 
❯ ls -l www/pkg/
total 435
-rw-r--r-- 1 achin achin 870347 Jan 28 02:13 numbat_wasm_bg.wasm
-rw-r--r-- 1 achin achin   1496 Jan 28 02:13 numbat_wasm_bg.wasm.d.ts
-rw-r--r-- 1 achin achin   3410 Jan 28 02:13 numbat_wasm.d.ts
-rw-r--r-- 1 achin achin  12237 Jan 28 02:13 numbat_wasm.js
-rw-r--r-- 1 achin achin    309 Jan 28 02:13 package.json
-rw-r--r-- 1 achin achin    221 Jan 28 02:13 README.md
achin@bbx in numbat/numbat-wasm (053e08d) is 📦 v0.1.0 🦀 took 15s 
❯ ls -l www/pkg/
total 435
-rw-r--r-- 1 achin achin 870077 Jan 28 02:15 numbat_wasm_bg.wasm
-rw-r--r-- 1 achin achin   1496 Jan 28 02:14 numbat_wasm_bg.wasm.d.ts
-rw-r--r-- 1 achin achin   3410 Jan 28 02:14 numbat_wasm.d.ts
-rw-r--r-- 1 achin achin  12237 Jan 28 02:14 numbat_wasm.js
-rw-r--r-- 1 achin achin    309 Jan 28 02:15 package.json
-rw-r--r-- 1 achin achin    221 Jan 28 02:15 README.md

@sharkdp
Copy link
Owner

sharkdp commented Jan 29, 2024

Weird. I also tested it and the numbat_wasm_bg.wasm file has 872,448 bytes before and after. This matches neither of your numbers. Might be a difference in compiler/wasm-pack?

▶ rustc --version
rustc 1.75.0 (82e1608df 2023-12-21)

~                                                                                                                             
▶ wasm-pack --version
wasm-pack 0.12.1

In any case... I think we can move forward with this PR and merge it. Numbat can potentially be a long-running application, so probably we shouldn't use an allocator that's leaking memory :-)

@sharkdp sharkdp merged commit d4996a9 into master Jan 29, 2024
14 checks passed
@sharkdp sharkdp deleted the dont-use-wee_alloc branch January 29, 2024 08:09
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.

None yet

3 participants