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 — buffer: optimize decoding wrapped base64 data by aqrln · Pull Request #12146 · nodejs/node

Conversation

@aqrln

@aqrln aqrln commented Mar 31, 2017 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

The fast base64 decoder used to switch to the slow one permanently when
it saw a whitespace or other garbage character. Since the most common
situation such characters may be encountered in is line-wrapped base64
data, a more profitable strategy is to decode a single 24-bit group with
the slow decoder and then continue running the fast algorithm.

Refs: #12114

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

buffer

Sorry, something went wrong.

Uh oh!

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

All reactions
@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++. labels Mar 31, 2017
@aqrln

aqrln commented Mar 31, 2017

Copy link
Copy Markdown
Contributor Author

I haven't run the whole benchmark suite yet, only symlinked the exiting and the new benchmarks into a new directory locally:

                                             improvement confidence      p.value
 base64/buffer-base64-decode-wrapped.js n=32     11.04 %        *** 4.413651e-27
 base64/buffer-base64-decode.js n=32             -1.49 %            5.613341e-02
All reactions

Sorry, something went wrong.

Uh oh!

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

This was referenced Mar 31, 2017
src: use const parameters in base64_decode_slow() #12144
Closed
base64 decoder could be 2x faster when decoding wrapped base64 #12114
Closed
@Fishrock123

Fishrock123 commented Mar 31, 2017

Copy link
Copy Markdown
Contributor

CI: https://ci.nodejs.org/job/node-test-pull-request/7140/

All reactions

Sorry, something went wrong.

Uh oh!

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

@jasnell
jasnell requested review from addaleax and trevnorris March 31, 2017 15:39
jasnell
jasnell reviewed Mar 31, 2017
View reviewed changes
Comment thread benchmark/buffers/buffer-base64-decode-wrapped.js Outdated

@jasnell jasnell Mar 31, 2017

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.

minor nit but this can be simplified just a bit by doing...

const line = 'abcd'.repeat(charsPerLine / 4) + '\n';
const buffer = Buffer.alloc(bytesCount, line);

Sorry, something went wrong.

Uh oh!

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

All reactions

@aqrln aqrln Mar 31, 2017

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.

Done. Thanks for the suggestion!

Sorry, something went wrong.

Uh oh!

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

All reactions
@aqrln
buffer: optimize decoding wrapped base64 data
ff77c72
The fast base64 decoder used to switch to the slow one permanently when
it saw a whitespace or other garbage character.  Since the most common
situation such characters may be encountered in is line-wrapped base64
data, a more profitable strategy is to decode a single 24-bit group with
the slow decoder and then continue running the fast algorithm.

Refs: nodejs#12114
@aqrln
aqrln force-pushed the base64-optimization branch from 1fb578c to ff77c72 Compare March 31, 2017 20:27
@aqrln

aqrln commented Mar 31, 2017

Copy link
Copy Markdown
Contributor Author

Rebased to incorporate changes from #11995.

All reactions

Sorry, something went wrong.

Uh oh!

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

addaleax
addaleax approved these changes Mar 31, 2017
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.

Thank you!

Sorry, something went wrong.

Uh oh!

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

All reactions
@aqrln

aqrln commented Apr 4, 2017

Copy link
Copy Markdown
Contributor Author

Can I get a fresh CI run?

All reactions

Sorry, something went wrong.

Uh oh!

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

@trevnorris

trevnorris commented Apr 4, 2017

Copy link
Copy Markdown
Contributor

CI: https://ci.nodejs.org/job/node-test-pull-request/7192/

All reactions

Sorry, something went wrong.

Uh oh!

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

trevnorris
trevnorris approved these changes Apr 4, 2017
View reviewed changes

@trevnorris trevnorris left a comment

Copy link
Copy Markdown
Contributor

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.

not sure I'm qualified to sign off on this but LGTM

Sorry, something went wrong.

Uh oh!

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

All reactions
@aqrln

aqrln commented Apr 4, 2017 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor Author

/cc @bnoordhuis (based on Git history)

All reactions

Sorry, something went wrong.

Uh oh!

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

jasnell
jasnell approved these changes Apr 4, 2017
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.

This LGTM but it would be really nice to have some cctests for this header file

Sorry, something went wrong.

Uh oh!

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

All reactions
@jasnell

jasnell commented Apr 4, 2017

Copy link
Copy Markdown
Member

(to be clear, the cctest can be added separately :-) ...)

All reactions

Sorry, something went wrong.

Uh oh!

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

jasnell pushed a commit that referenced this pull request Apr 4, 2017
@aqrln @jasnell
buffer: optimize decoding wrapped base64 data
e77a83f
The fast base64 decoder used to switch to the slow one permanently when
it saw a whitespace or other garbage character.  Since the most common
situation such characters may be encountered in is line-wrapped base64
data, a more profitable strategy is to decode a single 24-bit group with
the slow decoder and then continue running the fast algorithm.

PR-URL: #12146
Ref: #12114
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@jasnell

jasnell commented Apr 4, 2017

Copy link
Copy Markdown
Member

Landed in e77a83f

All reactions

Sorry, something went wrong.

Uh oh!

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

@jasnell jasnell closed this Apr 4, 2017
@aqrln
aqrln deleted the base64-optimization branch April 4, 2017 16:46
@jasnell jasnell mentioned this pull request Apr 4, 2017
8.0.0 Release Proposal #12220
Closed
aqrln added a commit to aqrln/node that referenced this pull request Apr 5, 2017
@aqrln
test: add basic cctest for base64.h
3c48697
This commit adds C++ tests for `base64_encode()` and `base64_decode()`
functions defined in `base64.h`.  The functionality is already being
tested indirectly in JavaScript tests for Buffer, but it won't hurt to
test the low-level functions too, especially given that they aren't only
used in the internal Buffer implementation, Chrome inspector protocol
support relies upon them too.

Refs: nodejs#12146 (comment)
@aqrln aqrln mentioned this pull request Apr 5, 2017
test: add basic cctest for base64.h #12238
Closed
3 tasks done
aqrln added a commit that referenced this pull request Apr 8, 2017
@aqrln
test: add basic cctest for base64.h
b03f1f0
This commit adds C++ tests for `base64_encode()` and `base64_decode()`
functions defined in `base64.h`.  The functionality is already being
tested indirectly in JavaScript tests for Buffer, but it won't hurt to
test the low-level functions too, especially given that they aren't only
used in the internal Buffer implementation, Chrome inspector protocol
support relies upon them too.

PR-URL: #12238
Refs: #12146 (comment)
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Apr 10, 2017
@aqrln Italo A. Casas
buffer: optimize decoding wrapped base64 data
805ebef
The fast base64 decoder used to switch to the slow one permanently when
it saw a whitespace or other garbage character.  Since the most common
situation such characters may be encountered in is line-wrapped base64
data, a more profitable strategy is to decode a single 24-bit group with
the slow decoder and then continue running the fast algorithm.

PR-URL: nodejs#12146
Ref: nodejs#12114
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@italoacasas italoacasas mentioned this pull request Apr 10, 2017
v7.9.0 Release Proposal #12319
Merged
2 tasks done
@MylesBorins MylesBorins added the lts-watch-v6.x label Apr 18, 2017
@MylesBorins

MylesBorins commented Apr 18, 2017

Copy link
Copy Markdown
Contributor

should we backport?

Assuming if so we should wait a bit

All reactions

Sorry, something went wrong.

Uh oh!

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

@MylesBorins MylesBorins added the baking-for-lts PRs whose changes need more time before an LTS backport. label May 15, 2017
@addaleax

addaleax commented Jun 13, 2017

Copy link
Copy Markdown
Member

should we backport?

It seems to be the root cause of #13657, so no.

aqrln 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.

@addaleax addaleax added dont-land-on-v4.x and removed baking-for-lts PRs whose changes need more time before an LTS backport. lts-watch-v6.x labels Jun 13, 2017
gibfahn pushed a commit that referenced this pull request Jun 18, 2017
@aqrln @gibfahn
test: add basic cctest for base64.h
f7abf36
This commit adds C++ tests for `base64_encode()` and `base64_decode()`
functions defined in `base64.h`.  The functionality is already being
tested indirectly in JavaScript tests for Buffer, but it won't hurt to
test the low-level functions too, especially given that they aren't only
used in the internal Buffer implementation, Chrome inspector protocol
support relies upon them too.

PR-URL: #12238
Refs: #12146 (comment)
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
gibfahn pushed a commit that referenced this pull request Jun 20, 2017
@aqrln @gibfahn
test: add basic cctest for base64.h
003a485
This commit adds C++ tests for `base64_encode()` and `base64_decode()`
functions defined in `base64.h`.  The functionality is already being
tested indirectly in JavaScript tests for Buffer, but it won't hurt to
test the low-level functions too, especially given that they aren't only
used in the internal Buffer implementation, Chrome inspector protocol
support relies upon them too.

PR-URL: #12238
Refs: #12146 (comment)
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 11, 2017
@aqrln @MylesBorins
test: add basic cctest for base64.h
19d9551
This commit adds C++ tests for `base64_encode()` and `base64_decode()`
functions defined in `base64.h`.  The functionality is already being
tested indirectly in JavaScript tests for Buffer, but it won't hurt to
test the low-level functions too, especially given that they aren't only
used in the internal Buffer implementation, Chrome inspector protocol
support relies upon them too.

PR-URL: #12238
Refs: #12146 (comment)
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
@aqrln aqrln mentioned this pull request Jul 21, 2017
src: fix decoding base64 with whitespace #13660
Merged
3 tasks done
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes
@addaleax addaleax addaleax approved these changes
+1 more reviewer
@trevnorris trevnorris trevnorris approved these changes
Reviewers whose approvals may not affect merge requirements

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. c++ Issues and PRs that require attention from people who are familiar with C++.

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

@aqrln @Fishrock123 @trevnorris @jasnell @MylesBorins @addaleax @nodejs-github-bot
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.