-
Notifications
You must be signed in to change notification settings - Fork 200
add btoa()/atob() builtins #1143
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
base: master
Are you sure you want to change the base?
Conversation
b8bd46d
to
690d752
Compare
Nice! Quick q: don't these throw DOMException on error? |
a7b66a2
to
96c34a2
Compare
@saghul I pushed some fixes: less code, no more cross-platform build errors from the LUTs. I didn’t add SIMD acceleration yet since I’m not sure if QuickJS would accept it. We can easyly replace my scalar impl by this one https://github.com/powturbo/Turbo-Base64 |
Counter point: for users that do need the web compat, a btoa/atob without DOMException is worse than no atob/btoa at all, since now I have to undo the incomplete implementation in favor of my complete one. bun/deno gets away with it because they are runtimes, not engines - here, quickjs.c is the engine, quickjs-libc the runtime. Feel free to reuse my PR (#1040), I'll just close it then. |
@bptato I can reuse your PR for the error exceptions, is it okay for you? |
Yes, this is what I made it for in the first place. |
@bptato have you pull the master and fix the conflicts? |
Implemented separately from the other errors because it is defined in terms of WebIDL, where members of an interface are getters on their prototype. See the difference between `JSON.stringify(Object.getOwnPropertyDescriptors(new TypeError()))` vs `JSON.stringify(Object.getOwnPropertyDescriptors(new DOMException()))`. Note: the standard doesn't specify where to put "stack". We follow existing practice which imitates node instead of browsers.
OK, I've rebased it. |
@bptato Pushed, I rebased my branch on yours, fix the conflicts, change the error messages and change the "throw" test with the new DOMException object |
7bdce32
to
bb9cbed
Compare
@bptato how would you like to handle the merge? Should this go into the engine or the runtime? In practice, 99% of runtimes expose btoa/atob. Even though they’re not in the official spec, almost every JS library that deals with base64 browser or server-side relies on them. Same with performance intrinsics: they’re not part of ECMAScript either, but QuickJS ships them by default. If we follow the same logic, btoa/atob should be included as well. Some features come from a formal spec, others become standards simply because everyone uses them. btoa/atob are in that second category. We can ship a correct, efficient, clean implementation so people don’t have to reinvent it. Power users (like Bun) can still swap in their own for performance, but for 99% of cases a solid default is what people want. |
Nice, thanks. You could also unify JS_AddIntrinsicDOMException and JS_AddIntrinsicBase64, they make little sense separately. (I don't know a good name, esp. if we want structuredClone in the same bracket eventually - I guess JS_AddIntrinsicWeb, but it feels a bit too broad... any ideas?)
Whatever is more convenient for the maintainers (Saúl & Ben).
#16 says it should be in the engine, so I think you put it in the right place. |
@bptato I'm a bit busy, in a few hours I will check to give a better name, so you want to unify both together? I was thinking of separating them like I did so the developers can choose to activate some APIs based on their needs |
@bptato JS_AddIntrinsicWinterTC_Fetch(ctx);
JS_AddIntrinsicWinterTC_URL(ctx);
JS_AddIntrinsicWinterTC_File(ctx); JS_AddIntrinsicFetch(ctx);
JS_AddIntrinsicURL(ctx);
JS_AddIntrinsicCrypto(ctx); If someone build a browser on top of quickjs he will want to follow different standard but majority of our developers base focus server-side. |
Chopping it up too granularly just makes the API harder to use.
But it doesn't add the entire spec, so these still feel wrong. How about we just keep calling the unified function JS_AddIntrinsicBase64? Then if we ever get structuredClone, it can be renamed to JS_AddIntrinsicSerialization or something. |
Note that Uint8Array recently got base64 added into it, so I guess we could piggiback on this implementation for that, and as such, make it builtin? Thoughts @bnoordhuis ? |
@saghul It’s something I can implement, I need first, to find the beautiful double free |
- Introduce global btoa() and atob() functions - Encoder: fast 12-bit pair-LUT, ~3.6 GB/s - Decoder: branchless streaming form, ~0.65 GB/s scalar - Tolerant to whitespace, validates padding and invalid input - Minimal allocations: only one malloc if input is wide-char - Fully compliant with DOMException
Edit: I had endianness problem, I sent a fix, let's see what say the test I don't have hardware to try on my side |
How much extra performance is obtained from this? It adds 8KB of data, makes the code less readable and prone to endianness issues. My own benchmarks (M2) actually show 2 to 5% slower times compared to the naive code:
|
Regarding decoding, here is a faster function (20-25% faster) that uses a single 256-byte table:
|
@chqrlie I’ve also force-pushed/rebased and fixed my implementation, so the encoder should be in better shape now. ;) I'll look at the decoder, and I'm open to proposals. From what I see, the main bottleneck isn’t in the encoder/decoder itself but in the btoa/atob wrappers. If you have ideas for optimizing atob, I'd love to hear them. Targeting at least 1 GB/s seems realistic, later we can swap with a faster base64 implementation without touching the wrappers |
The
My version is faster because it handles 3 bytes at a time in the main case and flushes the pending bytes in the I have not looked at the wrappers but the code of the conversion is performed in the lower level functions. I just wanted to underscore that the 12-bit LUT tables are actually slower than the simpler naive implementation on my CPU so I was wondering if you could test on your own CPU (presumably intel based). |
@aabbdev DOMException landed, can you please rebase your PR on top of master? Thanks! |
QJS(without SIMD):



Node 24.3.0 (with SIMD):
Deno 2.4.5(with SIMD):