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

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

Closed
wants to merge 16 commits into from

Conversation

lemire
Copy link
Member

@lemire lemire commented Jun 22, 2024

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";
function makeBenchmark(size, isToString) {
  const base64Input = Buffer.alloc(size, "latin1").toString("base64");
  const base64From = 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)) ;
await run();

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-bot nodejs-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
@lemire lemire added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 22, 2024
@github-actions github-actions bot 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 mcollina and jasnell June 22, 2024 17:35
@lemire
Copy link
Member Author

lemire commented Jun 22, 2024

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):

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

@lemire lemire requested a review from anonrig June 22, 2024 18:52
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 22, 2024
@github-actions github-actions bot 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
Copy link
Member

mildsunrise commented Jun 22, 2024

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
Member Author

lemire commented Jun 22, 2024

@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
Member

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
Member

jasnell commented Sep 8, 2024

PR is currently blocked from landing due to unreliable CI

@nodejs-github-bot

This comment was marked as outdated.

@lemire
Copy link
Member Author

lemire commented Sep 8, 2024

@jasnell Indeed.

@anonrig
Copy link
Member

anonrig commented Sep 8, 2024

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

@lemire
Copy link
Member Author

lemire commented Sep 8, 2024

@anonrig I synced.

@nodejs-github-bot

This comment was marked as outdated.

@anonrig anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 8, 2024
@lemire
Copy link
Member Author

lemire commented Sep 8, 2024

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

@anonrig
Copy link
Member

anonrig commented Sep 8, 2024

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

set bunch of tests as flaky - #54802 🫡

@lemire
Copy link
Member Author

lemire commented Sep 9, 2024

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

@nodejs-github-bot
Copy link
Collaborator

@lemire
Copy link
Member Author

lemire commented Sep 9, 2024

@anonrig Stuck.

@anonrig
Copy link
Member

anonrig commented Sep 9, 2024

@anonrig Stuck.

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

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

@lemire
Copy link
Member Author

lemire commented Sep 10, 2024

@anonrig This will never go through, will it?

@nodejs-github-bot
Copy link
Collaborator

@aduh95 aduh95 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
Contributor

aduh95 commented Sep 17, 2024

This needs a rebase.

@lemire
Copy link
Member Author

lemire commented Sep 17, 2024

Made obsolete by 8191e1f (@anonrig).

I am dropping this PR.

@lemire lemire closed this Sep 17, 2024
@anonrig
Copy link
Member

anonrig commented Sep 17, 2024

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

@lemire
Copy link
Member Author

lemire commented Sep 17, 2024

@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
Member Author

lemire commented Sep 17, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants