Skip to content

Commit 06a1bdd

Browse files
author
Ruben Bridgewater
committed
Fix js parser handling big values not fast enough
Fixes #678
1 parent 8bf794f commit 06a1bdd

File tree

4 files changed

+156
-79
lines changed

4 files changed

+156
-79
lines changed

‎README.md

Lines changed: 37 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -650,45 +650,45 @@ Here are results of `multi_bench.js` which is similar to `redis-benchmark` from
650650
hiredis parser (Lenovo T450s i7-5600U):
651651

652652
```
653-
Client count: 1, node version: 4.2.1, server version: 3.0.3, parser: hiredis
654-
PING, 1/1 min/max/avg/p95: 0/ 4/ 0.02/ 0.00 10001ms total, 38850.41 ops/sec
655-
PING, batch 50/1 min/max/avg/p95: 0/ 3/ 0.10/ 1.00 10001ms total, 488376.16 ops/sec
656-
SET 4B str, 1/1 min/max/avg/p95: 0/ 2/ 0.03/ 0.00 10001ms total, 35782.02 ops/sec
657-
SET 4B str, batch 50/1 min/max/avg/p95: 0/ 2/ 0.14/ 1.00 10001ms total, 349740.03 ops/sec
658-
SET 4B buf, 1/1 min/max/avg/p95: 0/ 5/ 0.04/ 0.00 10001ms total, 23497.75 ops/sec
659-
SET 4B buf, batch 50/1 min/max/avg/p95: 0/ 3/ 0.28/ 1.00 10001ms total, 177087.29 ops/sec
660-
GET 4B str, 1/1 min/max/avg/p95: 0/ 4/ 0.03/ 0.00 10001ms total, 37044.10 ops/sec
661-
GET 4B str, batch 50/1 min/max/avg/p95: 0/ 4/ 0.12/ 1.00 10001ms total, 421987.80 ops/sec
662-
GET 4B buf, 1/1 min/max/avg/p95: 0/ 4/ 0.03/ 0.00 10001ms total, 35608.24 ops/sec
663-
GET 4B buf, batch 50/1 min/max/avg/p95: 0/ 3/ 0.12/ 1.00 10001ms total, 416593.34 ops/sec
664-
SET 4KiB str, 1/1 min/max/avg/p95: 0/ 4/ 0.03/ 0.00 10001ms total, 30014.10 ops/sec
665-
SET 4KiB str, batch 50/1 min/max/avg/p95: 0/ 4/ 0.34/ 1.00 10001ms total, 147705.23 ops/sec
666-
SET 4KiB buf, 1/1 min/max/avg/p95: 0/ 4/ 0.04/ 0.00 10001ms total, 23803.52 ops/sec
667-
SET 4KiB buf, batch 50/1 min/max/avg/p95: 0/ 4/ 0.37/ 1.00 10001ms total, 132611.74 ops/sec
668-
GET 4KiB str, 1/1 min/max/avg/p95: 0/ 5/ 0.03/ 0.00 10001ms total, 34216.98 ops/sec
669-
GET 4KiB str, batch 50/1 min/max/avg/p95: 0/ 4/ 0.32/ 1.00 10001ms total, 153039.70 ops/sec
670-
GET 4KiB buf, 1/1 min/max/avg/p95: 0/ 3/ 0.03/ 0.00 10001ms total, 34169.18 ops/sec
671-
GET 4KiB buf, batch 50/1 min/max/avg/p95: 0/ 2/ 0.32/ 1.00 10001ms total, 153264.67 ops/sec
672-
INCR, 1/1 min/max/avg/p95: 0/ 3/ 0.03/ 0.00 10001ms total, 36307.17 ops/sec
673-
INCR, batch 50/1 min/max/avg/p95: 0/ 4/ 0.12/ 1.00 10001ms total, 412438.76 ops/sec
674-
LPUSH, 1/1 min/max/avg/p95: 0/ 4/ 0.03/ 0.00 10001ms total, 36073.89 ops/sec
675-
LPUSH, batch 50/1 min/max/avg/p95: 0/ 2/ 0.14/ 1.00 10001ms total, 355954.40 ops/sec
676-
LRANGE 10, 1/1 min/max/avg/p95: 0/ 2/ 0.03/ 0.00 10001ms total, 30395.66 ops/sec
677-
LRANGE 10, batch 50/1 min/max/avg/p95: 0/ 3/ 0.33/ 1.00 10001ms total, 149400.06 ops/sec
678-
LRANGE 100, 1/1 min/max/avg/p95: 0/ 2/ 0.06/ 1.00 10001ms total, 16814.62 ops/sec
679-
LRANGE 100, batch 50/1 min/max/avg/p95: 1/ 4/ 2.01/ 2.00 10002ms total, 24790.04 ops/sec
680-
SET 4MiB str, 1/1 min/max/avg/p95: 1/ 7/ 2.01/ 2.00 10002ms total, 496.90 ops/sec
681-
SET 4MiB str, batch 20/1 min/max/avg/p95: 100/ 135/ 109.58/ 125.00 10085ms total, 182.45 ops/sec
682-
SET 4MiB buf, 1/1 min/max/avg/p95: 1/ 5/ 1.87/ 2.00 10001ms total, 531.75 ops/sec
683-
SET 4MiB buf, batch 20/1 min/max/avg/p95: 52/ 77/ 58.90/ 68.45 10016ms total, 339.46 ops/sec
684-
GET 4MiB str, 1/1 min/max/avg/p95: 3/ 19/ 5.79/ 11.00 10005ms total, 172.51 ops/sec
685-
GET 4MiB str, batch 20/1 min/max/avg/p95: 73/ 112/ 89.89/ 107.00 10072ms total, 222.40 ops/sec
686-
GET 4MiB buf, 1/1 min/max/avg/p95: 3/ 13/ 5.35/ 9.00 10002ms total, 186.76 ops/sec
687-
GET 4MiB buf, batch 20/1 min/max/avg/p95: 76/ 106/ 85.37/ 98.00 10077ms total, 234.20 ops/sec
653+
Client count: 1, node version: 4.2.2, server version: 3.0.3, parser: hiredis
654+
PING, 1/1 min/max/avg/p95: 0/ 3/ 0.02/ 0.00 2501ms total, 39862.85 ops/sec
655+
PING, batch 50/1 min/max/avg/p95: 0/ 2/ 0.10/ 1.00 2501ms total, 491223.51 ops/sec
656+
SET 4B str, 1/1 min/max/avg/p95: 0/ 3/ 0.03/ 0.00 2501ms total, 36387.45 ops/sec
657+
SET 4B str, batch 50/1 min/max/avg/p95: 0/ 3/ 0.14/ 1.00 2501ms total, 346381.45 ops/sec
658+
SET 4B buf, 1/1 min/max/avg/p95: 0/ 2/ 0.04/ 0.00 2501ms total, 24395.84 ops/sec
659+
SET 4B buf, batch 50/1 min/max/avg/p95: 0/ 2/ 0.32/ 1.00 2501ms total, 156457.42 ops/sec
660+
GET 4B str, 1/1 min/max/avg/p95: 0/ 3/ 0.03/ 0.00 2501ms total, 36906.44 ops/sec
661+
GET 4B str, batch 50/1 min/max/avg/p95: 0/ 3/ 0.12/ 1.00 2501ms total, 425729.71 ops/sec
662+
GET 4B buf, 1/1 min/max/avg/p95: 0/ 2/ 0.03/ 0.00 2501ms total, 36221.91 ops/sec
663+
GET 4B buf, batch 50/1 min/max/avg/p95: 0/ 2/ 0.11/ 1.00 2501ms total, 430407.84 ops/sec
664+
SET 4KiB str, 1/1 min/max/avg/p95: 0/ 3/ 0.03/ 0.00 2501ms total, 30951.22 ops/sec
665+
SET 4KiB str, batch 50/1 min/max/avg/p95: 0/ 2/ 0.33/ 1.00 2501ms total, 150299.88 ops/sec
666+
SET 4KiB buf, 1/1 min/max/avg/p95: 0/ 2/ 0.04/ 1.00 2501ms total, 23919.63 ops/sec
667+
SET 4KiB buf, batch 50/1 min/max/avg/p95: 0/ 2/ 0.36/ 1.00 2501ms total, 139204.32 ops/sec
668+
GET 4KiB str, 1/1 min/max/avg/p95: 0/ 2/ 0.03/ 0.00 2501ms total, 32739.30 ops/sec
669+
GET 4KiB str, batch 50/1 min/max/avg/p95: 0/ 2/ 0.32/ 1.00 2501ms total, 154158.34 ops/sec
670+
GET 4KiB buf, 1/1 min/max/avg/p95: 0/ 2/ 0.03/ 0.00 2501ms total, 34654.94 ops/sec
671+
GET 4KiB buf, batch 50/1 min/max/avg/p95: 0/ 2/ 0.32/ 1.00 2501ms total, 153758.50 ops/sec
672+
INCR, 1/1 min/max/avg/p95: 0/ 2/ 0.03/ 0.00 2501ms total, 37530.19 ops/sec
673+
INCR, batch 50/1 min/max/avg/p95: 0/ 3/ 0.12/ 1.00 2501ms total, 415993.60 ops/sec
674+
LPUSH, 1/1 min/max/avg/p95: 0/ 1/ 0.03/ 0.00 2501ms total, 37409.04 ops/sec
675+
LPUSH, batch 50/1 min/max/avg/p95: 0/ 2/ 0.14/ 1.00 2501ms total, 354778.09 ops/sec
676+
LRANGE 10, 1/1 min/max/avg/p95: 0/ 3/ 0.03/ 0.00 2501ms total, 31768.49 ops/sec
677+
LRANGE 10, batch 50/1 min/max/avg/p95: 0/ 3/ 0.33/ 1.00 2501ms total, 151379.45 ops/sec
678+
LRANGE 100, 1/1 min/max/avg/p95: 0/ 2/ 0.06/ 1.00 2501ms total, 16801.68 ops/sec
679+
LRANGE 100, batch 50/1 min/max/avg/p95: 2/ 4/ 2.07/ 3.00 2501ms total, 24150.34 ops/sec
680+
SET 4MiB str, 1/1 min/max/avg/p95: 1/ 5/ 1.96/ 2.00 2501ms total, 510.20 ops/sec
681+
SET 4MiB str, batch 20/1 min/max/avg/p95: 83/ 108/ 94.44/ 106.40 2550ms total, 211.76 ops/sec
682+
SET 4MiB buf, 1/1 min/max/avg/p95: 1/ 7/ 2.06/ 3.00 2501ms total, 484.21 ops/sec
683+
SET 4MiB buf, batch 20/1 min/max/avg/p95: 38/ 48/ 40.90/ 46.00 2536ms total, 488.96 ops/sec
684+
GET 4MiB str, 1/1 min/max/avg/p95: 3/ 13/ 5.20/ 9.00 2503ms total, 192.17 ops/sec
685+
GET 4MiB str, batch 20/1 min/max/avg/p95: 74/ 105/ 87.24/ 104.00 2530ms total, 229.25 ops/sec
686+
GET 4MiB buf, 1/1 min/max/avg/p95: 3/ 11/ 5.01/ 9.00 2501ms total, 199.12 ops/sec
687+
GET 4MiB buf, batch 20/1 min/max/avg/p95: 78/ 93/ 84.23/ 91.90 2528ms total, 237.34 ops/sec
688688
```
689689

690-
The hiredis and js parser should most of the time be on the same level. The js parser lacks speed for large responses though.
691-
Therefor the hiredis parser is the default used in node_redis and we recommend using the hiredis parser. To use `hiredis`, do:
690+
The hiredis and js parser should most of the time be on the same level. But if you use Redis for big SUNION/SINTER/LRANGE/ZRANGE hiredis is significantly faster.
691+
Therefor the hiredis parser is the default used in node_redis. To use `hiredis`, do:
692692

693693
npm install hiredis redis
694694

‎changelog.md

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,13 @@
11
Changelog
22
=========
33

4+
## v.2.x.x - xx Nov, 2015
5+
6+
Bugfixes
7+
8+
- Fixed js parser handling big values slow ([@BridgeAR](https://github.com/BridgeAR))
9+
- The speed is now on par with the hiredis parser.
10+
411
## v.2.3.1 - 18 Nov, 2015
512

613
Bugfixes

‎lib/parsers/javascript.js

Lines changed: 13 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,8 @@ function JavascriptReplyParser() {
66
this.name = exports.name;
77
this._buffer = new Buffer(0);
88
this._offset = 0;
9+
this._big_offset = 0;
10+
this._chunks_size = 0;
911
this._buffers = [];
1012
}
1113

@@ -37,10 +39,6 @@ JavascriptReplyParser.prototype._parseResult = function (type) {
3739
}
3840
return new Error(this._buffer.toString('utf-8', start, end));
3941
} else if (type === 36) { // $
40-
// set a rewind point, as the packet could be larger than the
41-
// buffer in memory
42-
offset = this._offset - 1;
43-
4442
packetHeader = this.parseHeader();
4543

4644
// packets with a size of -1 are considered null
@@ -52,6 +50,8 @@ JavascriptReplyParser.prototype._parseResult = function (type) {
5250
start = this._offset;
5351

5452
if (end > this._buffer.length) {
53+
this._chunks_size = this._buffer.length - this._offset - 2;
54+
this._big_offset = packetHeader;
5555
throw new IncompleteReadBuffer('Wait for more data.');
5656
}
5757

@@ -60,6 +60,7 @@ JavascriptReplyParser.prototype._parseResult = function (type) {
6060

6161
return this._buffer.slice(start, end);
6262
} else if (type === 42) { // *
63+
// set a rewind point, as the packet is larger than the buffer in memory
6364
offset = this._offset;
6465
packetHeader = this.parseHeader();
6566

@@ -94,21 +95,20 @@ JavascriptReplyParser.prototype._parseResult = function (type) {
9495
};
9596

9697
JavascriptReplyParser.prototype.execute = function (buffer) {
97-
var i = buffer.length - 1;
9898

99-
while (buffer[i] !== 0x0a) {
100-
i--;
101-
if (i < 1) {
102-
this._buffers.push(buffer);
103-
return;
104-
}
99+
if (this._chunks_size !== 0 && this._big_offset > this._chunks_size + buffer.length) {
100+
this._buffers.push(buffer);
101+
this._chunks_size += buffer.length;
102+
return;
105103
}
106104

107105
if (this._buffers.length !== 0) {
108106
this._buffers.unshift(this._offset === 0 ? this._buffer : this._buffer.slice(this._offset));
109107
this._buffers.push(buffer);
110108
this._buffer = Buffer.concat(this._buffers);
111109
this._buffers = [];
110+
this._big_offset = 0;
111+
this._chunks_size = 0;
112112
} else if (this._offset >= this._buffer.length) {
113113
this._buffer = buffer;
114114
} else {
@@ -142,6 +142,8 @@ JavascriptReplyParser.prototype.run = function (buffer) {
142142

143143
this.send_reply(this._parseResult(type));
144144
} else if (type !== 10 && type !== 13) {
145+
// Reset the buffer so the parser can handle following commands properly
146+
this._buffer = new Buffer(0);
145147
var err = new Error('Protocol error, got "' + String.fromCharCode(type) + '" as reply type byte');
146148
this.send_error(err);
147149
}
@@ -169,7 +171,6 @@ JavascriptReplyParser.prototype._packetEndOffset = function () {
169171
while (this._buffer[offset] !== 0x0d && this._buffer[offset + 1] !== 0x0a) {
170172
offset++;
171173

172-
/* istanbul ignore if: activate the js parser out of memory test to test this */
173174
if (offset >= this._buffer.length) {
174175
throw new IncompleteReadBuffer('Did not see LF after NL reading multi bulk count (' + offset + ' => ' + this._buffer.length + ', ' + this._offset + ')');
175176
}

‎test/parser.spec.js

Lines changed: 99 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,7 @@
11
'use strict';
22

33
var assert = require('assert');
4-
var config = require("./lib/config");
54
var utils = require("../lib/utils");
6-
var redis = config.redis;
75
var parsers = [
86
require("../lib/parsers/javascript").Parser
97
];
@@ -73,37 +71,108 @@ describe('parsers', function () {
7371
assert.equal(reply_count, 3, "check reply should have been called three times");
7472
return done();
7573
});
76-
});
77-
});
7874

79-
// Activate this if you want to fry your cpu / memory
80-
describe.skip("test out of memory", function () {
81-
var args = config.configureClient('javascript', '127.0.0.1');
82-
var clients = new Array(300).join(" ").split(" ");
83-
var client;
84-
beforeEach(function (done) {
85-
client = redis.createClient.apply(redis.createClient, args);
86-
client.once("connect", function () {
87-
client.flushdb(done);
75+
it('multiple chunks in a bulk string', function (done) {
76+
var parser = new Parser();
77+
var reply_count = 0;
78+
function check_reply(reply) {
79+
reply = utils.reply_to_strings(reply);
80+
assert.strictEqual(reply, "abcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghij");
81+
reply_count++;
82+
}
83+
parser.send_reply = check_reply;
84+
85+
parser.execute(new Buffer('$100\r\nabcdefghij'));
86+
parser.execute(new Buffer('abcdefghijabcdefghijabcdefghij'));
87+
parser.execute(new Buffer('abcdefghijabcdefghijabcdefghij'));
88+
parser.execute(new Buffer('abcdefghijabcdefghijabcdefghij'));
89+
assert.strictEqual(reply_count, 0);
90+
parser.execute(new Buffer('\r\n'));
91+
assert.strictEqual(reply_count, 1);
92+
93+
parser.execute(new Buffer('$100\r'));
94+
parser.execute(new Buffer('\nabcdefghijabcdefghijabcdefghijabcdefghij'));
95+
parser.execute(new Buffer('abcdefghijabcdefghijabcdefghij'));
96+
parser.execute(new Buffer('abcdefghijabcdefghij'));
97+
assert.strictEqual(reply_count, 1);
98+
parser.execute(new Buffer(
99+
'abcdefghij\r\n' +
100+
'$100\r\nabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghij\r\n' +
101+
'$100\r\nabcdefghijabcdefghijabcdefghijabcdefghij'
102+
));
103+
assert.strictEqual(reply_count, 3);
104+
parser.execute(new Buffer('abcdefghijabcdefghijabcdefghij'));
105+
parser.execute(new Buffer('abcdefghijabcdefghijabcdefghij\r'));
106+
assert.strictEqual(reply_count, 3);
107+
parser.execute(new Buffer('\n'));
108+
109+
assert.equal(reply_count, 4, "check reply should have been called three times");
110+
return done();
88111
});
89-
});
90112

91-
it('reach limit and wait for further data', function (done) {
92-
setTimeout(done, 5000);
93-
clients.forEach(function(entry, a) {
94-
var max = 0;
95-
var client = redis.createClient.apply(redis.createClient, args);
96-
client.on('ready', function() {
97-
while (++max < 50) {
98-
var item = [];
99-
for (var i = 0; i < 100; ++i) {
100-
item.push('aaa' + (Math.random() * 1000000 | 0));
101-
}
102-
client.del('foo' + a);
103-
client.lpush('foo' + a, item);
104-
client.lrange('foo' + a, 0, 99);
105-
}
106-
});
113+
it('multiple chunks with arrays different types', function (done) {
114+
var parser = new Parser();
115+
var reply_count = 0;
116+
function check_reply(reply) {
117+
reply = utils.reply_to_strings(reply);
118+
assert.deepStrictEqual(reply, [
119+
'abcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghij',
120+
'test',
121+
100,
122+
new Error('Error message'),
123+
['The force awakens']
124+
]);
125+
reply_count++;
126+
}
127+
parser.send_reply = check_reply;
128+
129+
parser.execute(new Buffer('*5\r\n$100\r\nabcdefghij'));
130+
parser.execute(new Buffer('abcdefghijabcdefghijabcdefghij'));
131+
parser.execute(new Buffer('abcdefghijabcdefghijabcdefghij'));
132+
parser.execute(new Buffer('abcdefghijabcdefghijabcdefghij\r\n'));
133+
parser.execute(new Buffer('+test\r'));
134+
parser.execute(new Buffer('\n:100'));
135+
parser.execute(new Buffer('\r\n-Error message'));
136+
parser.execute(new Buffer('\r\n*1\r\n$17\r\nThe force'));
137+
assert.strictEqual(reply_count, 0);
138+
parser.execute(new Buffer(' awakens\r\n$5'));
139+
assert.strictEqual(reply_count, 1);
140+
return done();
141+
});
142+
143+
it('return normal errors', function (done) {
144+
var parser = new Parser();
145+
var reply_count = 0;
146+
function check_reply(reply) {
147+
assert.equal(reply.message, 'Error message');
148+
reply_count++;
149+
}
150+
parser.send_error = check_reply;
151+
152+
parser.execute(new Buffer('-Error '));
153+
parser.execute(new Buffer('message\r\n*3\r\n$17\r\nThe force'));
154+
assert.strictEqual(reply_count, 1);
155+
parser.execute(new Buffer(' awakens\r\n$5'));
156+
assert.strictEqual(reply_count, 1);
157+
return done();
158+
});
159+
160+
it('return null for empty arrays and empty bulk strings', function (done) {
161+
var parser = new Parser();
162+
var reply_count = 0;
163+
function check_reply(reply) {
164+
assert.equal(reply, null);
165+
reply_count++;
166+
}
167+
parser.send_reply = check_reply;
168+
169+
parser.execute(new Buffer('$-1\r\n*-'));
170+
assert.strictEqual(reply_count, 1);
171+
parser.execute(new Buffer('1'));
172+
assert.strictEqual(reply_count, 1);
173+
parser.execute(new Buffer('\r\n$-'));
174+
assert.strictEqual(reply_count, 2);
175+
return done();
107176
});
108177
});
109178
});

0 commit comments

Comments
 (0)