Skip to content

src: avoid allocation in the Size method for BASE64URL and BASE64 - #53550

Closed
lemire wants to merge 16 commits into
nodejs:mainfrom
lemire:fix_problem_with_size_base64
Closed

src: avoid allocation in the Size method for BASE64URL and BASE64#53550
lemire wants to merge 16 commits into
nodejs:mainfrom
lemire:fix_problem_with_size_base64

Conversation

@lemire

@lemirelemire commented Jun 22, 2024

Copy link
Copy Markdown
Member

For large base64 strings, this PR multiplies the performance of Buffer.from(..., "base64"); by making the Size function simpler and non-allocating.

This will reduce the gap with Bun from being over 3x slower to being about only 40% slower for large inputs. For modest inputs, Node.js is still about 2x slower than Bun. Note that both Bun and Node.js use the same underlying library (simdutf) for base64 decoding so any difference is entirely due to the runtime (not to the base64 decoding per se).

Benchmark (from bun):

import{bench,run}from"mitata";functionmakeBenchmark(size,isToString){constbase64Input=Buffer.alloc(size,"latin1").toString("base64");constbase64From=Buffer.from(base64Input,"base64");bench(`Buffer. from(${size} bytes, 'base64')`,()=>{Buffer.from(base64Input,"base64");});}[128,1024,32*1024,1024*1024*8].forEach(s=>makeBenchmark(s,false));awaitrun();

Node.js 22:

cpu: Apple M2
runtime: node v22.3.0 (arm64-darwin)
benchmark time (avg) (min … max) p75 p99 p999
----------------------------------------------------------------------------- -----------------------------
Buffer. from(128 bytes, 'base64') 110 ns/iter (101 ns … 457 ns) 110 ns 140 ns 208 ns
Buffer. from(1024 bytes, 'base64') 306 ns/iter (239 ns … 453 ns) 323 ns 426 ns 453 ns
Buffer. from(32768 bytes, 'base64') 9'087 ns/iter (7'333 ns … 317 µs) 8'709 ns 27'750 ns 51'041 ns
Buffer. from(8388608 bytes, 'base64') 3'273 µs/iter (3'144 µs … 3'564 µs) 3'387 µs 3'546 µs 3'564 µs

Node.js with this PR:

cpu: Apple M2
runtime: node v23.0.0-pre (arm64-darwin)
benchmark time (avg) (min … max) p75 p99 p999
----------------------------------------------------------------------------- -----------------------------
Buffer. from(128 bytes, 'base64') 111 ns/iter (101 ns … 470 ns) 112 ns 145 ns 224 ns
Buffer. from(1024 bytes, 'base64') 308 ns/iter (256 ns … 475 ns) 312 ns 452 ns 475 ns
Buffer. from(32768 bytes, 'base64') 7'384 ns/iter (7'091 ns … 12'399 ns) 7'268 ns 11'472 ns 12'399 ns
Buffer. from(8388608 bytes, 'base64') 1'431 µs/iter (1'358 µs … 2'034 µs) 1'453 µs 1'627 µs 2'034 µs

Bun canary (upcoming release)

cpu: Apple M2
runtime: bun 1.1.16 (arm64-darwin)
benchmark time (avg) (min … max) p75 p99 p999
----------------------------------------------------------------------------- -----------------------------
Buffer. from(128 bytes, 'base64') 115 ns/iter (89.42 ns … 1'054 ns) 92.67 ns 880 ns 1'034 ns
Buffer. from(1024 bytes, 'base64') 226 ns/iter (174 ns … 923 ns) 184 ns 720 ns 913 ns
Buffer. from(32768 bytes, 'base64') 4'045 ns/iter (3'904 ns … 4'504 ns) 4'083 ns 4'404 ns 4'504 ns
Buffer. from(8388608 bytes, 'base64') 998 µs/iter (861 µs … 1'590 µs) 1'165 µs 1'460 µs 1'590 µs

To get the Bun results, you need the canary which you may get with bun upgrade --canary.

@nodejs-github-botnodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Jun 22, 2024
@lemirelemire added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 22, 2024
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 22, 2024
@nodejs-github-bot

This comment was marked as outdated.

@anonrig
anonrig requested review from jasnell and mcollinaJune 22, 2024 17:35
@lemire

Copy link
Copy Markdown
MemberAuthor

For people who care about history, Size has been expensive since the very beginning (we trace it back to the initial commit by @isaacs in 2013):

caseBASE64: {
String::AsciiValue value(str);
data_size = base64_decoded_size(*value, value.length());
break;
}

@lemire
lemire requested a review from anonrigJune 22, 2024 18:52

@mcollinamcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

@mcollinamcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 22, 2024
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 22, 2024
@nodejs-github-bot

This comment was marked as outdated.

@mildsunrise

mildsunrise commented Jun 22, 2024

Copy link
Copy Markdown
Contributor

for the record, Size() wasn't just doing an unnecessary allocation, it was also returning the wrong size, causing us to allocate a much larger buffer than necessary. simdutf::base64_length_from_binary calculates the length after encoding; what we need here is the reverse. @lemire has replaced the code with this calculation:

str->Length() % 4 <= 1 ? str->Length() / 4 * 3
: str->Length() / 4 * 3 + (str->Length() % 4) - 1)

which is only an upper bound, as it doesn't look at the actual data, and so it assumes the worst case (all characters are data characters, i.e. no padding or whitespace). the math looks correct to me.

bear in mind that while Size() is allowed to return an upper bound, it is expected to return an exact prediction most of the time. if this is not the case, we do another allocation + memcpy to a new backing store with the actual size. for base64url the prediction is usually correct as it rarely has padding, but for base64 it will miss 2/3 of the time (or always, if the input contains whitespace).

@lemire

Copy link
Copy Markdown
MemberAuthor

@mildsunrise

As far as I can tell, Node never returned the exact size. Doing so requires scanning the entire input, checking for characters to discard.

@mildsunrise

Copy link
Copy Markdown
Contributor

wasn't base64_decoded_size supposed to do exactly that?

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@jasnell

Copy link
Copy Markdown
Member

PR is currently blocked from landing due to unreliable CI

@nodejs-github-bot

This comment was marked as outdated.

@lemire

Copy link
Copy Markdown
MemberAuthor

@jasnell Indeed.

@anonrig

Copy link
Copy Markdown
Member

@lemire can you rebase and force-push if you don't mind?

@lemire

Copy link
Copy Markdown
MemberAuthor

@anonrig I synced.

@nodejs-github-bot

This comment was marked as outdated.

@anonriganonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 8, 2024
@lemire

Copy link
Copy Markdown
MemberAuthor

@anonrig Looks like it is turning green... what did you do???? ❤️

@anonrig

Copy link
Copy Markdown
Member

@anonrig Looks like it is turning green... what did you do???? ❤️

set bunch of tests as flaky - #54802 🫡

@lemire

Copy link
Copy Markdown
MemberAuthor

@anonrig It still won't complete the tests though.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@lemire

Copy link
Copy Markdown
MemberAuthor

@anonrig Stuck.

@anonrig

Copy link
Copy Markdown
Member

@anonrig Stuck.

It seems that all macOS machines are down/offline at the moment. nodejs/build#3887

@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 9, 2024
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@lemire

Copy link
Copy Markdown
MemberAuthor

@anonrig This will never go through, will it?

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@aduh95aduh95 removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 17, 2024
@aduh95

Copy link
Copy Markdown
Contributor

This needs a rebase.

@lemire

Copy link
Copy Markdown
MemberAuthor

Made obsolete by 8191e1f (@anonrig).

I am dropping this PR.

@lemirelemire closed this Sep 17, 2024
@anonrig

Copy link
Copy Markdown
Member

This PR avoids calling simdutf on base64 encodings. It is not obsolete!

@lemire

Copy link
Copy Markdown
MemberAuthor

@anonrig Here is what the PR was...

Screenshot 2024-09-17 at 12 04 24 PM

It looks like this was resolved in...

8191e1f

@lemire

Copy link
Copy Markdown
MemberAuthor

@anonrig It is possible I missed something, if so, let me know.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bufferIssues and PRs related to the buffer subsystem.c++Issues and PRs that require attention from people who are familiar with C++.needs-ciPRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants

@lemire@nodejs-github-bot@mildsunrise@kurtextrem@LeszekSwirski@jasnell@anonrig@aduh95@mcollina