v.2.0-proposal #3
Conversation
…rformance. Almost 3x faster.
Handle big data in constant time again Small performance improvements here and there Add ReplyError subclass
Don't use `continue` Make sure the returned strings are always proper utf8 values Make sure the jit knows has as much information as possible Don't return a bulk string before `\n` is present Improve hiredis warnings
|
LGTM |
|
I ran the new parser with ioredis, and the benchmark result is impressive. Awesome work! However, when parsing a big array, new parser seems to be slower than the hiredis: var bigArraySize = 100;
var bigArray = '*' + bigArraySize + '\r\n';
for (var i = 0; i < bigArraySize; i++) {
bigArray += '$';
var size = (Math.random() * 10 | 0) + 1;
bigArray += size + '\r\n' + lorem.slice(0, size) + '\r\n';
}
var bigArrayList = bigArray.split('\r\n').map(function (str) {
return new Buffer(str + '\r\n');
}).slice(0, -1);
// BIG ARRAYS
suite.add('\nOLD CODE: * bigArray', function () {
for (var i = 0; i < bigArrayList.length; i++) {
parserOld.execute(bigArrayList[i]);
}
})
suite.add('HIREDIS: * bigArray', function () {
for (var i = 0; i < bigArrayList.length; i++) {
parserHiRedis.execute(bigArrayList[i]);
}
})
suite.add('NEW CODE: * bigArray', function () {
for (var i = 0; i < bigArrayList.length; i++) {
parser.execute(bigArrayList[i]);
}
})The result: |
|
I double checked my code above and found a mistake in it. I've updated the code and the result in the above comment. It turns out the new parser is faster than the old one, but slower than hiredis. |
|
The array example is not good, as all the Edit: :D |
It is going to increase in size if required
This improves bulk strings with multiple chunks significantly
Also free the bufferPool over time again
|
I updated the benchmark numbers and they look very promising to me 😃 I'm still wondering why Currently I think it's difficult to get much better results than the ones we reached. @Salakar @luin do you mind doing another code review? I can move the new commits to another branch if you wish. |
| var bufferPool = new Buffer(64 * 1024) | ||
| var interval = null |
There was a problem hiding this comment.
Wouldn't this and bufferPool cause inconsistencies if running more than one parser as they are module scoped. Perhaps move into constructor?
Edit: nvm, not necessary
|
|
As per our call, this is all good, so I say LGTM and released, unless @luin has any issues? Further perf tweaks can be done in v2 minor versions if needed. |
|
I just ran the benchmark and it looks good to me. 👍 |
|
@luin what's the perf increases like on ioredis? |
|
@Salakar I didn't run it with ioredis. I just ran I think the new parser is fast enough to replace hiredis. |
|
Are we ready to go with this? 😸 Any objections to me merging and publishing a new version? @luin might potentially a good time to fit this into ioredis v2 before it's release? |
|
No, there are regressions left while concatenating bulk strings that I addressed locally but I want to rewrite it a bit more but currently I'm occupied by hack 4 humanity. |
|
@Salakar I'm going to do more tests this weekend. If the new parser is fast enough that we don't need to introduce |
This is a complete rewrite of the JS parser done by @Salakar with additional work by me.
As the hiredis parser is now likely slower in every case, I went ahead and deprecated it and removed any hints about it in the documentation. It's still usable by using the undocumented name option, but it'll print a warning in that case.
All errors returned by the parser are from now on "ReplyErrors" with a limited stack, as the stack is not meaningful at all and requires computation time.
The benchmark was run on a Lenovo T450s with an i7 on NodeJS 6.1.
Supersedes and closes #2
Edit: I updated the benchmark numbers and the the comment about removing hiredis, as it's currently only deprecated to keep backwards compatibility.