Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/faster crypto #664

Closed
wants to merge 5 commits into from
Closed

Conversation

indutny
Copy link
Member

@indutny indutny commented Jan 30, 2015

Right now this is only about crypto.createHash().update().digest(), but soon will add other methods here if you do not have any objections.

cc @bnoordhuis . Btw, would be nice to have @iojs/crypto

Handle most popular cases in a trie-style, branching on a first
character.

Remove useless HandleScope which was only eating time without producing
any value.
Boosts speed up to 10% on primitive `createHash().update().digest()`
benchmark.
@indutny
Copy link
Member Author

indutny commented Jan 30, 2015

Btw, it still spends >15% of time in malloc()/free().

screen shot 2015-01-30 at 15 15 41

@indutny
Copy link
Member Author

indutny commented Jan 30, 2015

Whoa replaced all uses of StringBytes in crypto. @bnoordhuis: please take a look.

@indutny
Copy link
Member Author

indutny commented Jan 30, 2015

Ha, just created @iojs/crypto

@indutny
Copy link
Member Author

indutny commented Jan 30, 2015

Oh, forgot to say. Overall improvement is about 15%.


node::Utf8Value encoding(isolate, encoding_v);

return ParseEncoding(*encoding, _default);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can probably do a little better still by also passing encoding->length(). Then you can filter on length first before you start comparing bytes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, wouldn't it be cheaper to do the comparison in JS and just pass integers to C++ land? I suspect that the biggest overhead is from the new[]/delete[] calls, not the string comparison.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm +1 for JS-land thing, but for now - let's do it the way it is. encoding->length() doesn't seem to be improving situation much on my benchmarks.

@bnoordhuis
Copy link
Member

LGTM. Left some comments but nothing substantial.

indutny added a commit that referenced this pull request Jan 30, 2015
Handle most popular cases in a trie-style, branching on a first
character.

Remove useless HandleScope which was only eating time without producing
any value.

PR-URL: #664
Reviewed-By: Ben Noordhuis <[email protected]>
indutny added a commit that referenced this pull request Jan 30, 2015
PR-URL: #664
Reviewed-By: Ben Noordhuis <[email protected]>
indutny added a commit that referenced this pull request Jan 30, 2015
Boosts speed up to 10% on primitive `createHash().update().digest()`
benchmark.

PR-URL: #664
Reviewed-By: Ben Noordhuis <[email protected]>
@indutny
Copy link
Member Author

indutny commented Jan 30, 2015

Landed in c6367e7, aca2011, 3d4e96f. Thank you!

@indutny indutny closed this Jan 30, 2015
@indutny indutny deleted the feature/faster-crypto branch January 30, 2015 14:48
@piscisaureus
Copy link
Contributor

Speaking of naming, I think StringBytes::InlineDecoder isn't the greatest of names either.

@indutny
Copy link
Member Author

indutny commented Jan 30, 2015

@piscisaureus suggestions?

@piscisaureus
Copy link
Contributor

First of all the Inline part is misleading; this class doesn't inline anything, it tries to avoid memory allocations. So EmbeddedDecoder or OnStackDecoder would have been better, the latter being more clear about the intended use case but not necessarily true.

To take it a step further, an InlineDecoder instance basically represents a string that's been copied out of the v8 heap so an appropriate name would be StringBytes (*).

The current StringBytes type actually doesn't represent anything, it's basically acts as a namespace for a couple of jsstring-to-cstring conversion helper functions. A better name would be StringDecoder. Where I think Decoder isn't free of confusion, maybe Convertor would be better?

(*) You might argue that the InlineDecoder just represents a memory allocation, because it's possible to call Decode() multiple times. However it looks like it's unsafe to do so; I wonder why you made Decode a method in the first place. In other words I think InlineDecoder::InlineDecoder (Handle<String> string, Handle<Value> encoding, enum encoding default_encoding) would have been a more appropriate signature for the constructor.

@piscisaureus
Copy link
Contributor

BTW I think this is a good addition.
There are other cases where v8::String::Utf8Value causes many small memory allocations; using this approach would make it more efficient.

@indutny
Copy link
Member Author

indutny commented Jan 30, 2015

@piscisaureus there is no v8 API to do such on-stack allocations.

@indutny
Copy link
Member Author

indutny commented Jan 30, 2015

@piscisaureus oh, we could use WriteUtf8!

@indutny
Copy link
Member Author

indutny commented Jan 30, 2015

@piscisaureus opened a PR as you suggested #670

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants