GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||
GeezerVison is showing github.com — buffer: optimize decoding wrapped base64 data by aqrln · Pull Request #12146 · nodejs/node Conversation
|
|
I haven't run the whole benchmark suite yet, only symlinked the exiting and the new benchmarks into a new directory locally: |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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.
There was an error while loading. Please reload this page.
There was a problem 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.
There was an error while loading. Please reload this page.
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
1fb578c to
ff77c72
Compare
|
Rebased to incorporate changes from #11995. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Can I get a fresh CI run? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
/cc @bnoordhuis (based on Git history) |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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.
There was an error while loading. Please reload this page.
|
(to be clear, the cctest can be added separately :-) ...) |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
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]>
|
Landed in e77a83f |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
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)
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]>
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]>
|
should we backport? Assuming if so we should wait a bit |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
It seems to be the root cause of #13657, so no. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
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]>
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]>
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]>
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.