From 06a1bdd7b0bf72d46c71d92e9ca20d12f32e83bb Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Sun, 22 Nov 2015 16:58:51 +0100 Subject: [PATCH] Fix js parser handling big values not fast enough Fixes #678 --- README.md | 74 +++++++++++----------- changelog.md | 7 +++ lib/parsers/javascript.js | 25 ++++---- test/parser.spec.js | 129 +++++++++++++++++++++++++++++--------- 4 files changed, 156 insertions(+), 79 deletions(-) diff --git a/README.md b/README.md index bd7c8fd480..69681fbfbe 100644 --- a/README.md +++ b/README.md @@ -650,45 +650,45 @@ Here are results of `multi_bench.js` which is similar to `redis-benchmark` from hiredis parser (Lenovo T450s i7-5600U): ``` -Client count: 1, node version: 4.2.1, server version: 3.0.3, parser: hiredis - PING, 1/1 min/max/avg/p95: 0/ 4/ 0.02/ 0.00 10001ms total, 38850.41 ops/sec - PING, batch 50/1 min/max/avg/p95: 0/ 3/ 0.10/ 1.00 10001ms total, 488376.16 ops/sec - SET 4B str, 1/1 min/max/avg/p95: 0/ 2/ 0.03/ 0.00 10001ms total, 35782.02 ops/sec - SET 4B str, batch 50/1 min/max/avg/p95: 0/ 2/ 0.14/ 1.00 10001ms total, 349740.03 ops/sec - SET 4B buf, 1/1 min/max/avg/p95: 0/ 5/ 0.04/ 0.00 10001ms total, 23497.75 ops/sec - SET 4B buf, batch 50/1 min/max/avg/p95: 0/ 3/ 0.28/ 1.00 10001ms total, 177087.29 ops/sec - GET 4B str, 1/1 min/max/avg/p95: 0/ 4/ 0.03/ 0.00 10001ms total, 37044.10 ops/sec - GET 4B str, batch 50/1 min/max/avg/p95: 0/ 4/ 0.12/ 1.00 10001ms total, 421987.80 ops/sec - GET 4B buf, 1/1 min/max/avg/p95: 0/ 4/ 0.03/ 0.00 10001ms total, 35608.24 ops/sec - GET 4B buf, batch 50/1 min/max/avg/p95: 0/ 3/ 0.12/ 1.00 10001ms total, 416593.34 ops/sec - SET 4KiB str, 1/1 min/max/avg/p95: 0/ 4/ 0.03/ 0.00 10001ms total, 30014.10 ops/sec - SET 4KiB str, batch 50/1 min/max/avg/p95: 0/ 4/ 0.34/ 1.00 10001ms total, 147705.23 ops/sec - SET 4KiB buf, 1/1 min/max/avg/p95: 0/ 4/ 0.04/ 0.00 10001ms total, 23803.52 ops/sec - SET 4KiB buf, batch 50/1 min/max/avg/p95: 0/ 4/ 0.37/ 1.00 10001ms total, 132611.74 ops/sec - GET 4KiB str, 1/1 min/max/avg/p95: 0/ 5/ 0.03/ 0.00 10001ms total, 34216.98 ops/sec - GET 4KiB str, batch 50/1 min/max/avg/p95: 0/ 4/ 0.32/ 1.00 10001ms total, 153039.70 ops/sec - GET 4KiB buf, 1/1 min/max/avg/p95: 0/ 3/ 0.03/ 0.00 10001ms total, 34169.18 ops/sec - GET 4KiB buf, batch 50/1 min/max/avg/p95: 0/ 2/ 0.32/ 1.00 10001ms total, 153264.67 ops/sec - INCR, 1/1 min/max/avg/p95: 0/ 3/ 0.03/ 0.00 10001ms total, 36307.17 ops/sec - INCR, batch 50/1 min/max/avg/p95: 0/ 4/ 0.12/ 1.00 10001ms total, 412438.76 ops/sec - LPUSH, 1/1 min/max/avg/p95: 0/ 4/ 0.03/ 0.00 10001ms total, 36073.89 ops/sec - LPUSH, batch 50/1 min/max/avg/p95: 0/ 2/ 0.14/ 1.00 10001ms total, 355954.40 ops/sec - LRANGE 10, 1/1 min/max/avg/p95: 0/ 2/ 0.03/ 0.00 10001ms total, 30395.66 ops/sec - LRANGE 10, batch 50/1 min/max/avg/p95: 0/ 3/ 0.33/ 1.00 10001ms total, 149400.06 ops/sec - LRANGE 100, 1/1 min/max/avg/p95: 0/ 2/ 0.06/ 1.00 10001ms total, 16814.62 ops/sec - LRANGE 100, batch 50/1 min/max/avg/p95: 1/ 4/ 2.01/ 2.00 10002ms total, 24790.04 ops/sec - SET 4MiB str, 1/1 min/max/avg/p95: 1/ 7/ 2.01/ 2.00 10002ms total, 496.90 ops/sec - SET 4MiB str, batch 20/1 min/max/avg/p95: 100/ 135/ 109.58/ 125.00 10085ms total, 182.45 ops/sec - SET 4MiB buf, 1/1 min/max/avg/p95: 1/ 5/ 1.87/ 2.00 10001ms total, 531.75 ops/sec - SET 4MiB buf, batch 20/1 min/max/avg/p95: 52/ 77/ 58.90/ 68.45 10016ms total, 339.46 ops/sec - GET 4MiB str, 1/1 min/max/avg/p95: 3/ 19/ 5.79/ 11.00 10005ms total, 172.51 ops/sec - GET 4MiB str, batch 20/1 min/max/avg/p95: 73/ 112/ 89.89/ 107.00 10072ms total, 222.40 ops/sec - GET 4MiB buf, 1/1 min/max/avg/p95: 3/ 13/ 5.35/ 9.00 10002ms total, 186.76 ops/sec - GET 4MiB buf, batch 20/1 min/max/avg/p95: 76/ 106/ 85.37/ 98.00 10077ms total, 234.20 ops/sec +Client count: 1, node version: 4.2.2, server version: 3.0.3, parser: hiredis + PING, 1/1 min/max/avg/p95: 0/ 3/ 0.02/ 0.00 2501ms total, 39862.85 ops/sec + PING, batch 50/1 min/max/avg/p95: 0/ 2/ 0.10/ 1.00 2501ms total, 491223.51 ops/sec + SET 4B str, 1/1 min/max/avg/p95: 0/ 3/ 0.03/ 0.00 2501ms total, 36387.45 ops/sec + SET 4B str, batch 50/1 min/max/avg/p95: 0/ 3/ 0.14/ 1.00 2501ms total, 346381.45 ops/sec + SET 4B buf, 1/1 min/max/avg/p95: 0/ 2/ 0.04/ 0.00 2501ms total, 24395.84 ops/sec + SET 4B buf, batch 50/1 min/max/avg/p95: 0/ 2/ 0.32/ 1.00 2501ms total, 156457.42 ops/sec + GET 4B str, 1/1 min/max/avg/p95: 0/ 3/ 0.03/ 0.00 2501ms total, 36906.44 ops/sec + GET 4B str, batch 50/1 min/max/avg/p95: 0/ 3/ 0.12/ 1.00 2501ms total, 425729.71 ops/sec + GET 4B buf, 1/1 min/max/avg/p95: 0/ 2/ 0.03/ 0.00 2501ms total, 36221.91 ops/sec + GET 4B buf, batch 50/1 min/max/avg/p95: 0/ 2/ 0.11/ 1.00 2501ms total, 430407.84 ops/sec + SET 4KiB str, 1/1 min/max/avg/p95: 0/ 3/ 0.03/ 0.00 2501ms total, 30951.22 ops/sec + SET 4KiB str, batch 50/1 min/max/avg/p95: 0/ 2/ 0.33/ 1.00 2501ms total, 150299.88 ops/sec + SET 4KiB buf, 1/1 min/max/avg/p95: 0/ 2/ 0.04/ 1.00 2501ms total, 23919.63 ops/sec + SET 4KiB buf, batch 50/1 min/max/avg/p95: 0/ 2/ 0.36/ 1.00 2501ms total, 139204.32 ops/sec + GET 4KiB str, 1/1 min/max/avg/p95: 0/ 2/ 0.03/ 0.00 2501ms total, 32739.30 ops/sec + GET 4KiB str, batch 50/1 min/max/avg/p95: 0/ 2/ 0.32/ 1.00 2501ms total, 154158.34 ops/sec + GET 4KiB buf, 1/1 min/max/avg/p95: 0/ 2/ 0.03/ 0.00 2501ms total, 34654.94 ops/sec + GET 4KiB buf, batch 50/1 min/max/avg/p95: 0/ 2/ 0.32/ 1.00 2501ms total, 153758.50 ops/sec + INCR, 1/1 min/max/avg/p95: 0/ 2/ 0.03/ 0.00 2501ms total, 37530.19 ops/sec + INCR, batch 50/1 min/max/avg/p95: 0/ 3/ 0.12/ 1.00 2501ms total, 415993.60 ops/sec + LPUSH, 1/1 min/max/avg/p95: 0/ 1/ 0.03/ 0.00 2501ms total, 37409.04 ops/sec + LPUSH, batch 50/1 min/max/avg/p95: 0/ 2/ 0.14/ 1.00 2501ms total, 354778.09 ops/sec + LRANGE 10, 1/1 min/max/avg/p95: 0/ 3/ 0.03/ 0.00 2501ms total, 31768.49 ops/sec + LRANGE 10, batch 50/1 min/max/avg/p95: 0/ 3/ 0.33/ 1.00 2501ms total, 151379.45 ops/sec + LRANGE 100, 1/1 min/max/avg/p95: 0/ 2/ 0.06/ 1.00 2501ms total, 16801.68 ops/sec + LRANGE 100, batch 50/1 min/max/avg/p95: 2/ 4/ 2.07/ 3.00 2501ms total, 24150.34 ops/sec + SET 4MiB str, 1/1 min/max/avg/p95: 1/ 5/ 1.96/ 2.00 2501ms total, 510.20 ops/sec + SET 4MiB str, batch 20/1 min/max/avg/p95: 83/ 108/ 94.44/ 106.40 2550ms total, 211.76 ops/sec + SET 4MiB buf, 1/1 min/max/avg/p95: 1/ 7/ 2.06/ 3.00 2501ms total, 484.21 ops/sec + SET 4MiB buf, batch 20/1 min/max/avg/p95: 38/ 48/ 40.90/ 46.00 2536ms total, 488.96 ops/sec + GET 4MiB str, 1/1 min/max/avg/p95: 3/ 13/ 5.20/ 9.00 2503ms total, 192.17 ops/sec + GET 4MiB str, batch 20/1 min/max/avg/p95: 74/ 105/ 87.24/ 104.00 2530ms total, 229.25 ops/sec + GET 4MiB buf, 1/1 min/max/avg/p95: 3/ 11/ 5.01/ 9.00 2501ms total, 199.12 ops/sec + GET 4MiB buf, batch 20/1 min/max/avg/p95: 78/ 93/ 84.23/ 91.90 2528ms total, 237.34 ops/sec ``` -The hiredis and js parser should most of the time be on the same level. The js parser lacks speed for large responses though. -Therefor the hiredis parser is the default used in node_redis and we recommend using the hiredis parser. To use `hiredis`, do: +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. +Therefor the hiredis parser is the default used in node_redis. To use `hiredis`, do: npm install hiredis redis diff --git a/changelog.md b/changelog.md index 704b17054a..502eaa8a87 100644 --- a/changelog.md +++ b/changelog.md @@ -1,6 +1,13 @@ Changelog ========= +## v.2.x.x - xx Nov, 2015 + +Bugfixes + +- Fixed js parser handling big values slow ([@BridgeAR](https://github.com/BridgeAR)) + - The speed is now on par with the hiredis parser. + ## v.2.3.1 - 18 Nov, 2015 Bugfixes diff --git a/lib/parsers/javascript.js b/lib/parsers/javascript.js index e6f6ec0784..7decd0447b 100644 --- a/lib/parsers/javascript.js +++ b/lib/parsers/javascript.js @@ -6,6 +6,8 @@ function JavascriptReplyParser() { this.name = exports.name; this._buffer = new Buffer(0); this._offset = 0; + this._big_offset = 0; + this._chunks_size = 0; this._buffers = []; } @@ -37,10 +39,6 @@ JavascriptReplyParser.prototype._parseResult = function (type) { } return new Error(this._buffer.toString('utf-8', start, end)); } else if (type === 36) { // $ - // set a rewind point, as the packet could be larger than the - // buffer in memory - offset = this._offset - 1; - packetHeader = this.parseHeader(); // packets with a size of -1 are considered null @@ -52,6 +50,8 @@ JavascriptReplyParser.prototype._parseResult = function (type) { start = this._offset; if (end > this._buffer.length) { + this._chunks_size = this._buffer.length - this._offset - 2; + this._big_offset = packetHeader; throw new IncompleteReadBuffer('Wait for more data.'); } @@ -60,6 +60,7 @@ JavascriptReplyParser.prototype._parseResult = function (type) { return this._buffer.slice(start, end); } else if (type === 42) { // * + // set a rewind point, as the packet is larger than the buffer in memory offset = this._offset; packetHeader = this.parseHeader(); @@ -94,14 +95,11 @@ JavascriptReplyParser.prototype._parseResult = function (type) { }; JavascriptReplyParser.prototype.execute = function (buffer) { - var i = buffer.length - 1; - while (buffer[i] !== 0x0a) { - i--; - if (i < 1) { - this._buffers.push(buffer); - return; - } + if (this._chunks_size !== 0 && this._big_offset > this._chunks_size + buffer.length) { + this._buffers.push(buffer); + this._chunks_size += buffer.length; + return; } if (this._buffers.length !== 0) { @@ -109,6 +107,8 @@ JavascriptReplyParser.prototype.execute = function (buffer) { this._buffers.push(buffer); this._buffer = Buffer.concat(this._buffers); this._buffers = []; + this._big_offset = 0; + this._chunks_size = 0; } else if (this._offset >= this._buffer.length) { this._buffer = buffer; } else { @@ -142,6 +142,8 @@ JavascriptReplyParser.prototype.run = function (buffer) { this.send_reply(this._parseResult(type)); } else if (type !== 10 && type !== 13) { + // Reset the buffer so the parser can handle following commands properly + this._buffer = new Buffer(0); var err = new Error('Protocol error, got "' + String.fromCharCode(type) + '" as reply type byte'); this.send_error(err); } @@ -169,7 +171,6 @@ JavascriptReplyParser.prototype._packetEndOffset = function () { while (this._buffer[offset] !== 0x0d && this._buffer[offset + 1] !== 0x0a) { offset++; - /* istanbul ignore if: activate the js parser out of memory test to test this */ if (offset >= this._buffer.length) { throw new IncompleteReadBuffer('Did not see LF after NL reading multi bulk count (' + offset + ' => ' + this._buffer.length + ', ' + this._offset + ')'); } diff --git a/test/parser.spec.js b/test/parser.spec.js index 01dd38e539..c1444186b7 100644 --- a/test/parser.spec.js +++ b/test/parser.spec.js @@ -1,9 +1,7 @@ 'use strict'; var assert = require('assert'); -var config = require("./lib/config"); var utils = require("../lib/utils"); -var redis = config.redis; var parsers = [ require("../lib/parsers/javascript").Parser ]; @@ -73,37 +71,108 @@ describe('parsers', function () { assert.equal(reply_count, 3, "check reply should have been called three times"); return done(); }); - }); - }); - // Activate this if you want to fry your cpu / memory - describe.skip("test out of memory", function () { - var args = config.configureClient('javascript', '127.0.0.1'); - var clients = new Array(300).join(" ").split(" "); - var client; - beforeEach(function (done) { - client = redis.createClient.apply(redis.createClient, args); - client.once("connect", function () { - client.flushdb(done); + it('multiple chunks in a bulk string', function (done) { + var parser = new Parser(); + var reply_count = 0; + function check_reply(reply) { + reply = utils.reply_to_strings(reply); + assert.strictEqual(reply, "abcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghij"); + reply_count++; + } + parser.send_reply = check_reply; + + parser.execute(new Buffer('$100\r\nabcdefghij')); + parser.execute(new Buffer('abcdefghijabcdefghijabcdefghij')); + parser.execute(new Buffer('abcdefghijabcdefghijabcdefghij')); + parser.execute(new Buffer('abcdefghijabcdefghijabcdefghij')); + assert.strictEqual(reply_count, 0); + parser.execute(new Buffer('\r\n')); + assert.strictEqual(reply_count, 1); + + parser.execute(new Buffer('$100\r')); + parser.execute(new Buffer('\nabcdefghijabcdefghijabcdefghijabcdefghij')); + parser.execute(new Buffer('abcdefghijabcdefghijabcdefghij')); + parser.execute(new Buffer('abcdefghijabcdefghij')); + assert.strictEqual(reply_count, 1); + parser.execute(new Buffer( + 'abcdefghij\r\n' + + '$100\r\nabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghij\r\n' + + '$100\r\nabcdefghijabcdefghijabcdefghijabcdefghij' + )); + assert.strictEqual(reply_count, 3); + parser.execute(new Buffer('abcdefghijabcdefghijabcdefghij')); + parser.execute(new Buffer('abcdefghijabcdefghijabcdefghij\r')); + assert.strictEqual(reply_count, 3); + parser.execute(new Buffer('\n')); + + assert.equal(reply_count, 4, "check reply should have been called three times"); + return done(); }); - }); - it('reach limit and wait for further data', function (done) { - setTimeout(done, 5000); - clients.forEach(function(entry, a) { - var max = 0; - var client = redis.createClient.apply(redis.createClient, args); - client.on('ready', function() { - while (++max < 50) { - var item = []; - for (var i = 0; i < 100; ++i) { - item.push('aaa' + (Math.random() * 1000000 | 0)); - } - client.del('foo' + a); - client.lpush('foo' + a, item); - client.lrange('foo' + a, 0, 99); - } - }); + it('multiple chunks with arrays different types', function (done) { + var parser = new Parser(); + var reply_count = 0; + function check_reply(reply) { + reply = utils.reply_to_strings(reply); + assert.deepStrictEqual(reply, [ + 'abcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghij', + 'test', + 100, + new Error('Error message'), + ['The force awakens'] + ]); + reply_count++; + } + parser.send_reply = check_reply; + + parser.execute(new Buffer('*5\r\n$100\r\nabcdefghij')); + parser.execute(new Buffer('abcdefghijabcdefghijabcdefghij')); + parser.execute(new Buffer('abcdefghijabcdefghijabcdefghij')); + parser.execute(new Buffer('abcdefghijabcdefghijabcdefghij\r\n')); + parser.execute(new Buffer('+test\r')); + parser.execute(new Buffer('\n:100')); + parser.execute(new Buffer('\r\n-Error message')); + parser.execute(new Buffer('\r\n*1\r\n$17\r\nThe force')); + assert.strictEqual(reply_count, 0); + parser.execute(new Buffer(' awakens\r\n$5')); + assert.strictEqual(reply_count, 1); + return done(); + }); + + it('return normal errors', function (done) { + var parser = new Parser(); + var reply_count = 0; + function check_reply(reply) { + assert.equal(reply.message, 'Error message'); + reply_count++; + } + parser.send_error = check_reply; + + parser.execute(new Buffer('-Error ')); + parser.execute(new Buffer('message\r\n*3\r\n$17\r\nThe force')); + assert.strictEqual(reply_count, 1); + parser.execute(new Buffer(' awakens\r\n$5')); + assert.strictEqual(reply_count, 1); + return done(); + }); + + it('return null for empty arrays and empty bulk strings', function (done) { + var parser = new Parser(); + var reply_count = 0; + function check_reply(reply) { + assert.equal(reply, null); + reply_count++; + } + parser.send_reply = check_reply; + + parser.execute(new Buffer('$-1\r\n*-')); + assert.strictEqual(reply_count, 1); + parser.execute(new Buffer('1')); + assert.strictEqual(reply_count, 1); + parser.execute(new Buffer('\r\n$-')); + assert.strictEqual(reply_count, 2); + return done(); }); }); });