GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — src,lib,buffer: improve atob / btoa performance by XadillaX · Pull Request #38433 · nodejs/node

Conversation

@XadillaX

@XadillaX XadillaX commented Apr 27, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Extract modp_b64 dependence from Chromium to improve performance of atob() and btoa().

Refs: https://github.com/chromium/chromium/tree/92.0.4490.1/third_party/modp_b64

Benchmark

  • 12 Intel(R) Core(TM) i7-8700 CPU @ 3.20GHz
  • MemTotal: 32657380 kB

Old logic

$ ./node benchmark/run.js --filter buffer-atob.js buffers

buffers/buffer-atob.js
buffers/buffer-atob.js n=32 len=65536: 487.7280162769471

$ ./node benchmark/run.js --filter buffer-btoa.js buffers

buffers/buffer-btoa.js
buffers/buffer-btoa.js n=32 len=67108864: 2.4594857665824494

Logic of this PR

$ ./node benchmark/run.js --filter buffer-atob.js buffers

buffers/buffer-atob.js
buffers/buffer-atob.js n=32 len=65536: 10,588.320156389489

$ ./node benchmark/run.js --filter buffer-btoa.js buffers

buffers/buffer-btoa.js
buffers/buffer-btoa.js n=32 len=67108864: 3.503510145848807

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bricss reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@XadillaX
src,lib,buffer: improve atob / btoa performance
a3f184d
Extract `modp_b64` dependence from Chromium to improve performance of
`atob()` and `btoa()`.

Refs: https://github.com/chromium/chromium/tree/92.0.4490.1/third_party/modp_b64
@github-actions github-actions Bot added lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run. labels Apr 27, 2021
XadillaX added 2 commits April 27, 2021 18:31
@XadillaX
f
beceed4
@XadillaX
f
5cb24c8
@XadillaX XadillaX added the buffer Issues and PRs related to the buffer subsystem. label Apr 27, 2021
@XadillaX
f
b42b4ba
jasnell
jasnell reviewed Apr 27, 2021
View reviewed changes
Comment thread benchmark/buffers/buffer-atob.js Outdated
@@ -0,0 +1,44 @@
// Copyright Joyent, Inc. and other Node contributors.

@jasnell jasnell Apr 27, 2021

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New files added should never have a copyright header added to them.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
jasnell
jasnell requested changes Apr 27, 2021
View reviewed changes

@jasnell jasnell left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Despite the performance boost, I'm generally -1 on introducing a new dependency and a different codepath for base64 encoding/decoding only for atob/btoa. If anything, we should be looking at improving the base64/base64url encoding performance for Buffer. /cc @addaleax

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

addaleax reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
addaleax
addaleax requested changes Apr 27, 2021
View reviewed changes

@addaleax addaleax left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, this is far too much optimization for methods that should never be used.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@XadillaX

XadillaX commented Apr 27, 2021

Copy link
Copy Markdown
Contributor Author

Despite the performance boost, I'm generally -1 on introducing a new dependency and a different codepath for base64 encoding/decoding only for atob/btoa. If anything, we should be looking at improving the base64/base64url encoding performance for Buffer. /cc @addaleax

I think I can try to use modp b64 in Buffer's Base64 if it's really faster.

This dep is extracted from Chromium.

addaleax and BridgeAR reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@legendecas

legendecas commented Apr 27, 2021

Copy link
Copy Markdown
Member

According to https://github.com/nodejs/node/blob/master/doc/guides/contributing/pull-requests.md#dependencies policy, changes on deps/* should be sent to upstream first. In that case, as Node.js already have their own implementation of base64 functionality, adding a new dependency for base64 solely doesn't seem to be the best effort to reduce the burden of collaborators. While the base64 codebase isn't quite large, are there any improvements that are worth to be made on Node.js own base64 implementation?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

XadillaX added 2 commits April 28, 2021 20:29
@XadillaX
f
63b9a49
@XadillaX
f
ff1d141
@XadillaX

XadillaX commented Apr 28, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

@addaleax @jasnell I've updated, removing modp_b64 and using base64_encode. And got the new benchmark:

$ ./node benchmark/run.js --filter buffer-btoa.js buffers

buffers/buffer-btoa.js
buffers/buffer-btoa.js n=32 len=67108864: 3.3501380480392817

$ ./node benchmark/run.js --filter buffer-atob.js buffers

buffers/buffer-atob.js
buffers/buffer-atob.js n=32 len=65536: 6,966.175950365997

And I've found that modp_b64's decoding is faster then code in Node.js'.

Maybe we can merge this PR first (if possible), and then use modp_b64 to instead of Node.js' current Base64 code.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@XadillaX
f
6b5eeec
@addaleax

addaleax commented Apr 28, 2021

Copy link
Copy Markdown
Member

@XadillaX In order to merge this, I think we should:

  • Remove the C++-backed atob and btoa implementations
  • Remove their benchmarks
  • Undo the stylistic changes that are happening to unrelated code
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@XadillaX

XadillaX commented Apr 28, 2021

Copy link
Copy Markdown
Contributor Author

@XadillaX In order to merge this, I think we should:

  • Remove the C++-backed atob and btoa implementations
  • Remove their benchmarks
  • Undo the stylistic changes that are happening to unrelated code

The C++-backed implementations is just fasten the function to resolve:

// TODO(@jasnell): The implementation here has not been performance
// optimized in any way.

And this is not a large change, just call base64_encode directly to avoid do unnecessary Buffer call.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

XadillaX added 2 commits April 28, 2021 20:44
@XadillaX
f
ba56cb9
@XadillaX
f
5200196
@addaleax

addaleax commented Apr 28, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

The C++-backed implementations is just fasten the function to resolve:

Then let’s remove the TODO comment, or edit it to make clear that there’s not actually anything to do there.

Making these functions faster tells people that it’s okay to use them. It’s not, therefore let’s not worry about their performance at all.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@XadillaX
f
074c82c
legendecas
legendecas reviewed Apr 29, 2021
View reviewed changes
Comment thread src/node_errors.h
V(ERR_INVALID_ARG_VALUE, TypeError) \
V(ERR_OSSL_EVP_INVALID_DIGEST, Error) \
V(ERR_INVALID_ARG_TYPE, TypeError) \
V(ERR_INVALID_CHARACTER, RangeError) \

@legendecas legendecas Apr 29, 2021

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DOMException are expected in atob/btoa per https://html.spec.whatwg.org/multipage/webappapis.html#dom-atob

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@XadillaX XadillaX Apr 29, 2021

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's wrapped in JavaScript side.

btw, this PR seems won't be merged according to @addaleax's suggestion.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@XadillaX

XadillaX commented May 1, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

@addaleax I've tried to use modp_b64 to instead of current and found the performance is almost the same (c975dff...XadillaX:base64-perf).

So the way to improve this PR is just to make atob() / btoa() using base64_encode / base64_decode directly.

But as you said before, there's no need to improve them.

That means the only thing I should do is to remove @jasnell's TODO comment?

/cc @jasnell

addaleax reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@XadillaX XadillaX mentioned this pull request May 5, 2021
buffer: remove TODOs in atob / btoa #38548
Merged
aduh95 pushed a commit to XadillaX/node that referenced this pull request May 9, 2021
@XadillaX @aduh95
buffer: remove TODOs in atob / btoa
0b6f0a0
Refs: nodejs#38433 (comment)

PR-URL: nodejs#38548
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Zijian Liu <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
targos pushed a commit that referenced this pull request May 17, 2021
@XadillaX @targos
buffer: remove TODOs in atob / btoa
dcae032
Refs: #38433 (comment)

PR-URL: #38548
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Zijian Liu <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
targos pushed a commit that referenced this pull request May 30, 2021
@XadillaX @targos
buffer: remove TODOs in atob / btoa
ed76154
Refs: #38433 (comment)

PR-URL: #38548
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Zijian Liu <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
targos pushed a commit that referenced this pull request Jun 5, 2021
@XadillaX @targos
buffer: remove TODOs in atob / btoa
536e23f
Refs: #38433 (comment)

PR-URL: #38548
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Zijian Liu <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
targos pushed a commit that referenced this pull request Jun 5, 2021
@XadillaX @targos
buffer: remove TODOs in atob / btoa
6628297
Refs: #38433 (comment)

PR-URL: #38548
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Zijian Liu <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
targos pushed a commit that referenced this pull request Jun 11, 2021
@XadillaX @targos
buffer: remove TODOs in atob / btoa
3c886e0
Refs: #38433 (comment)

PR-URL: #38548
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Zijian Liu <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
This was referenced Jun 16, 2022
fix: support decoding data URL in Node < v16 Menci/vite-plugin-wasm#3
Merged
fix(wasm): support inlined WASM in Node < v16 (fix #8620) vitejs/vite#8622
Merged
@Trott
Trott force-pushed the main branch from 2d76238 to ca3ed36 Compare November 12, 2022 01:49
@mscdex mscdex mentioned this pull request Nov 21, 2022
draft: implement bufferToString #45562
Closed
@dcousens

dcousens commented Feb 5, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

We are thinking about using atob and btoa in feross/buffer#339 for web compatibility.
We have seen quite quickly that the node implementations are poorly optimised.

Although the buffer module is directed at browsers, if user code unintentionally finds it's way to using the buffer module in a node environment and subsequently ends up using atob or btoa, this is only a loss for users of node, not our library.

chjj, jonkoops, and H4ad reacted with thumbs up emoji
All reactions
  • 👍 3 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@dcousens dcousens mentioned this pull request Feb 5, 2024
Remove dependency on base64-js feross/buffer#339
Open
@mpodwysocki mpodwysocki mentioned this pull request Feb 6, 2024
[core] Upgrade to ES-Modules for core Azure/azure-sdk-for-js#26238
Merged
3 tasks
@H4ad H4ad mentioned this pull request Feb 6, 2024
buffer: fix DoS vector in atob #51670
Closed
@lemire

lemire commented Feb 6, 2024

Copy link
Copy Markdown
Member

Note that it is planned that simdutf could provide fast routines for base64 in the near future.

See
nodejs/performance#128

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@lemire lemire mentioned this pull request Mar 16, 2024
Base64 simdutf/simdutf#375
Merged
@lemire

lemire commented Mar 16, 2024

Copy link
Copy Markdown
Member

The simdutf library (already part of Node.js) will have accelerated base64 support in the next release. Decoding will follow the WHATWG forgiving-base64 specification so that ASCII white spaces are allowed in base64 content.

simdutf/simdutf#375

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@lemire lemire mentioned this pull request Mar 18, 2024
deps: update simdutf to 5.0.0 #52138
Merged
@lemire lemire mentioned this pull request Mar 30, 2024
Base64 part2 : base64url, UTF-8 inputs and base64_to_binary_safe simdutf/simdutf#382
Merged
@MattIPv4 MattIPv4 mentioned this pull request Apr 5, 2024
buffer: improve atob performance #52381
Closed
@github-actions github-actions Bot mentioned this pull request Apr 24, 2024
Adding VersionStream for nodejs-22 wolfi-dev/os#17646
Merged
@github-actions

github-actions Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

This pull request has been marked as stale due to 90 days of inactivity.
It will be automatically closed in 30 days if no further activity occurs. If this is still relevant, please leave a comment or update it to keep it open.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@github-actions github-actions Bot added the stale Issues and PRs marked stale due to inactivity and scheduled for automatic closure. label Jul 28, 2026
@avivkeller avivkeller closed this Aug 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@legendecas legendecas legendecas left review comments
@jasnell jasnell jasnell requested changes
@addaleax addaleax addaleax requested changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

buffer Issues and PRs related to the buffer subsystem. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run. stale Issues and PRs marked stale due to inactivity and scheduled for automatic closure.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

7 participants

@XadillaX @legendecas @addaleax @dcousens @lemire @jasnell @avivkeller
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.