buffer: optimize hex parsing.#7602

chjj · 2016-07-07T23:10:58Z

Checklist

make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

buffer

Description of change

Hex parsing can be a slight bottle-neck for anyone who does Buffer.from(str, 'hex') or buf.write(str, 'hex') frequently. 6 potential comparisons as well as some arithmetic per character wasn't cutting it for me. A single pointer access per character is a lot faster (results in a near 2x speedup on 1kb hex strings).

# Current HEAD: $ ./out/Release/node benchmark/buffers/buffer-hex.js buffers/buffer-hex.js len="0" n="10000000": 6006950.59809 buffers/buffer-hex.js len="1" n="10000000": 1524912.32297 buffers/buffer-hex.js len="64" n="10000000": 1213341.44991 buffers/buffer-hex.js len="1024" n="10000000": 248060.16367 # With hex parsing optimized: $ ./out/Release/node benchmark/buffers/buffer-hex.js buffers/buffer-hex.js len="0" n="10000000": 6130801.93323 buffers/buffer-hex.js len="1" n="10000000": 1717385.59814 buffers/buffer-hex.js len="64" n="10000000": 1367649.36959 buffers/buffer-hex.js len="1024" n="10000000": 463809.34484

addaleax · 2016-07-07T23:15:29Z

src/string_bytes.cc

-return10 + (c - 'a');
-returnstatic_cast<unsigned>(-1);
-}
+constint8_t unhex_table[256] =


static const int8_t?

addaleax · 2016-07-07T23:48:30Z

src/string_bytes.cc

-unsigned b = hex2bin(src[i * 2 + 1]);
+unsigned a = unhex(src[i * 2 + 0]);
+unsigned b = unhex(src[i * 2 + 1]);
 if (!~a || !~b)


Does this still work with unhex()’s return type changed? It seems to me some of the speedup may come from the compiler eliminating this branch completely because it knows the upper bits of a and b will never be set:
$ ./node-master -p 'Buffer(10).write("abcdxx", "hex")' 2 $ ./node -p 'Buffer(10).write("abcdxx", "hex")' 3
(aka we need more tests to cover these cases?)

@addaleax, good catch. I didn't notice the type difference. It looks like you're right.
Applying this patch:
diff --git a/src/string_bytes.cc b/src/string_bytes.cc index c216c5d..cb0e78a 100644 --- a/src/string_bytes.cc+++ b/src/string_bytes.cc@@ -173,9 +173,9 @@ size_t hex_decode(char* buf, const size_t srcLen){size_t i; for (i = 0; i < len && i * 2 + 1 < srcLen; ++i){- unsigned a = unhex(src[i * 2 + 0]);- unsigned b = unhex(src[i * 2 + 1]);- if (!~a || !~b)+ uint8_t a = unhex(src[i * 2 + 0]);+ uint8_t b = unhex(src[i * 2 + 1]);+ if ((a | b) & 0x80) return i; buf[i] = (a << 4) | b}
Kills some of the gained perf, but large hex strings are still faster:
$ ./out/Release/node benchmark/buffers/buffer-hex.js buffers/buffer-hex.js len="0" n="10000000": 5888692.25695 buffers/buffer-hex.js len="1" n="10000000": 1471220.48559 buffers/buffer-hex.js len="64" n="10000000": 1206331.66340 buffers/buffer-hex.js len="1024" n="10000000": 385088.76228
I'll see what else I can do, and probably add a test for bad hex strings.

jasnell · 2016-07-08T03:50:49Z

test/parallel/test-buffer-badhex.js

+var common = require('../common');
+var assert = require('assert');
+
+var Buffer = require('buffer').Buffer;


const here please

Fishrock123 · 2016-07-08T08:59:55Z

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

Fishrock123 · 2016-07-08T09:44:04Z

@chjj Please run make lint :)

addaleax · 2016-07-08T10:16:59Z

test/parallel/test-buffer-badhex.js

+ assert.deepStrictEqual(buf4, new Buffer([0, 0, 0, 0]));
+ assert.equal(buf4.write('xxabcd', 0, 'hex'), 0);
+ assert.deepStrictEqual(buf4, new Buffer([0, 0, 0, 0]));
+ assert.equal(buf4.write('xxab', 1, 'hex'), 0);


nit: assert.strictEqual is generally preferred over assert.equal, especially for comparisons with 0. :)

addaleax · 2016-07-08T10:18:52Z

LGTM with nits and a happy linter.

chjj · 2016-07-08T19:37:31Z

Cleaned up and linted.

addaleax · 2016-07-08T21:34:52Z

LGTM, nice work!

addaleax · 2016-07-08T21:36:07Z

CI: https://ci.nodejs.org/job/node-test-commit/4027/

addaleax · 2016-07-17T16:23:05Z

Landed in 151d316, thanks!

PR-URL: #7602 Reviewed-By: Anna Henningsen <[email protected]>

Notable changes: * **buffer**: * Improve performance of Buffer.from(str, 'hex') and Buffer#write(str, 'hex'). (Christopher Jeffrey) #7602 * Fix creating from zero-length ArrayBuffer. (Ingvar Stepanyan) #7176 * **deps**: Upgrade to V8 5.0.71.xx. (Ben Noordhuis) #7531 * **repl**: Fix issue with function redeclaration. (Prince J Wesley) #7794 * **util**: Fix inspecting of boxed symbols. (Anna Henningsen) #7641 PR-URL: #7782

Notable changes: * **buffer**: * Improve performance of Buffer.from(str, 'hex') and Buffer#write(str, 'hex'). (Christopher Jeffrey) #7602 * Fix creating from zero-length ArrayBuffer. (Ingvar Stepanyan) #7176 * **deps**: * Upgrade to V8 5.0.71.xx. (Ben Noordhuis) #7531 * Backport V8 instanceof bugfix (Franziska Hinkelmann) #7638 * **repl**: Fix issue with function redeclaration. (Prince J Wesley) #7794 * **util**: Fix inspecting of boxed symbols. (Anna Henningsen) #7641 PR-URL: #7782

### Notable changes * **buffer**: * Improve performance of Buffer.from(str, 'hex') and Buffer#write(str, 'hex'). (Christopher Jeffrey) [#7602](nodejs/node#7602) * Fix creating from zero-length ArrayBuffer. (Ingvar Stepanyan) [#7176](nodejs/node#7176) * **deps**: * Upgrade to V8 5.0.71.xx. (Ben Noordhuis) [#7531](nodejs/node#7531) * Backport V8 instanceof bugfix (Franziska Hinkelmann) [#7638](nodejs/node#7638) * **repl**: Fix issue with function redeclaration. (Prince J Wesley) [#7794](nodejs/node#7794) * **util**: Fix inspecting of boxed symbols. (Anna Henningsen) [#7641](nodejs/node#7641)

This test was recently (at the time of writing) introduced in 151d316 and could be cleaned up a bit. Refs: nodejs#7602 PR-URL: nodejs#7773 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Minwoo Jung <[email protected]> Reviewed-By: Ilkka Myller <[email protected]> Reviewed-By: James M Snell <[email protected]>

This test was recently (at the time of writing) introduced in 151d316 and could be cleaned up a bit. Refs: #7602 PR-URL: #7773 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Minwoo Jung <[email protected]> Reviewed-By: Ilkka Myller <[email protected]> Reviewed-By: James M Snell <[email protected]>

PR-URL: #7602 Reviewed-By: Anna Henningsen <[email protected]>

This test was recently (at the time of writing) introduced in 151d316 and could be cleaned up a bit. Refs: #7602 PR-URL: #7773 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Minwoo Jung <[email protected]> Reviewed-By: Ilkka Myller <[email protected]> Reviewed-By: James M Snell <[email protected]>

PR-URL: #7602 Reviewed-By: Anna Henningsen <[email protected]>

This test was recently (at the time of writing) introduced in 151d316 and could be cleaned up a bit. Refs: #7602 PR-URL: #7773 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Minwoo Jung <[email protected]> Reviewed-By: Ilkka Myller <[email protected]> Reviewed-By: James M Snell <[email protected]>

PR-URL: #7602 Reviewed-By: Anna Henningsen <[email protected]>

This test was recently (at the time of writing) introduced in 151d316 and could be cleaned up a bit. Refs: #7602 PR-URL: #7773 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Minwoo Jung <[email protected]> Reviewed-By: Ilkka Myller <[email protected]> Reviewed-By: James M Snell <[email protected]>

buffer: optimize hex_decode.
5a8b48e

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jul 7, 2016

addaleax reviewed Jul 7, 2016
View reviewed changes

src/string_bytes.cc Outdated
return10 + (c - 'a');
returnstatic_cast<unsigned>(-1);
}
constint8_t unhex_table[256] =
Copy link
Member
addaleaxJul 7, 2016
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
static const int8_t?

Trott added buffer Issues and PRs related to the buffer subsystem. benchmark Issues and PRs related to the benchmark subsystem. labels Jul 7, 2016

addaleax reviewed Jul 7, 2016
View reviewed changes

buffer: fix hex parsing and add bad hex string test.
ae21267

jasnell reviewed Jul 8, 2016
View reviewed changes

addaleax reviewed Jul 8, 2016
View reviewed changes

buffer: lint for hex parsing optimization.
1627283

addaleax closed this Jul 17, 2016

addaleax pushed a commit that referenced this pull request Jul 17, 2016
buffer: optimize hex_decode
151d316
PR-URL: #7602 Reviewed-By: Anna Henningsen <[email protected]>

Fishrock123 mentioned this pull request Jul 17, 2016
test: clean up test-buffer-badhex #7773
Merged
2 tasks

evanlucas pushed a commit that referenced this pull request Jul 19, 2016
buffer: optimize hex_decode
ba6ab7c
PR-URL: #7602 Reviewed-By: Anna Henningsen <[email protected]>

evanlucas mentioned this pull request Jul 19, 2016
v6.3.1 Release Proposal #7782
Merged

evanlucas pushed a commit that referenced this pull request Jul 20, 2016
buffer: optimize hex_decode
4158272
PR-URL: #7602 Reviewed-By: Anna Henningsen <[email protected]>

cjean-fr mentioned this pull request Jul 24, 2016
Upgrade node to 6.3.1 ScoopInstaller/Scoop#966
Merged

MylesBorins pushed a commit that referenced this pull request Sep 30, 2016
buffer: optimize hex_decode
f2916f8
PR-URL: #7602 Reviewed-By: Anna Henningsen <[email protected]>

MylesBorins added the land-on-v4.x label Sep 30, 2016

rvagg pushed a commit that referenced this pull request Oct 18, 2016
buffer: optimize hex_decode
df4f845
PR-URL: #7602 Reviewed-By: Anna Henningsen <[email protected]>

MylesBorins pushed a commit that referenced this pull request Oct 26, 2016
buffer: optimize hex_decode
17c4187
PR-URL: #7602 Reviewed-By: Anna Henningsen <[email protected]>

MylesBorins mentioned this pull request Oct 26, 2016
V4.6.2 proposal #9298
Closed

Uh oh!

buffer: optimize hex parsing.#7602

buffer: optimize hex parsing. #7602

Uh oh!

Conversation

chjj commented Jul 7, 2016

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

addaleaxJul 7, 2016

Choose a reason for hiding this comment

Uh oh!

addaleaxJul 7, 2016

Choose a reason for hiding this comment

Uh oh!

chjjJul 7, 2016

Choose a reason for hiding this comment

Uh oh!

jasnellJul 8, 2016

Choose a reason for hiding this comment

Uh oh!

Fishrock123 commented Jul 8, 2016

Uh oh!

Fishrock123 commented Jul 8, 2016

Uh oh!

addaleaxJul 8, 2016

Choose a reason for hiding this comment

Uh oh!

addaleax commented Jul 8, 2016

Uh oh!

chjj commented Jul 8, 2016

Uh oh!

addaleax commented Jul 8, 2016

Uh oh!

addaleax commented Jul 8, 2016

Uh oh!

addaleax commented Jul 17, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants