-
Notifications
You must be signed in to change notification settings - Fork 86
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
latest version of phpecc made the performance HORIBLE #66
Comments
-----BEGIN PGP SIGNED MESSAGE----- Yeah totally agree. I've been thinking about what we might be able to do there. The problem I've been dicking around with secp256k1, I can put what I have up for It's testing my abilities of dealing with C types and their conversions On 23/02/15 19:07, Ruben de Vries wrote:
Thomas Kerin My PGP key can be found here iQJ8BAEBCgBmBQJU63xVXxSAAAAAAC4AKGlzc3Vlci1mcHJAbm90YXRpb25zLm9w |
I guess it's worth documenting our effort :P |
It might be worth getting this PHP extension added to the Debian/Ubuntu apt repositiories for general use. |
Definitely! It's bare bones right now and not everything is implemented. Currently (on my system) it dies due do a memory corruption error. If I comment something unrelated to the secp256k1_.. function call, it works fine. (last test in test.php) @rubensayshi is on a long flight today, so left it that way so he could observe (and bypass) if he gets to it/ Comparing 10,000 runs of secp256k1_ecdsa_verify() (3 seconds) with 100 of PHPECC's verify() (84 seconds), we're looking at a 2800x performance increase! |
Also, ec_pubkey_create gives the wrong result (compared to brainwallet.org) so not sure what's up there. |
haha, moving discussion here huh? I started using phpunit, we can add more vectors later.
also when it returns a compressed and yea I have the same seg fault still happening, just now it happens in the decompress test, but fuck that we'll get someone experienced to code review this later and he'll know :P |
also @btcdrak apart from this being good for various reasons, but one of my personal reasons is is to get our SDK faster, so I'll aim (and have company time to invest) to get it into |
like I hinted in the other comment, if we're gonna push through with this then I'll try to get someone to code review it, free or paid, I'll poke around in /r/php and the php mailinglist once we have a little more. |
@rubensayshi Great news. It should definitely be a compiled extension |
@rubensayshi not necessarily move it altogether, just mentioning on this issue that we're trying to do something about the performance :) Interesting, yeah a few gotchyas to work around it seems.. But getting this reviewed and onto APT would be awesome! Also with regards the memory issue, it seemed commenting $pubkey2 'fixed' it, even though it's memory is never touched in the decompress function.. It could be building up as it gets used more.. |
I know it was because of a fix for a side channel attack or something like that and that that's something important but the difference is kinda big ...
below isn't a proper benchmark, but just to compare a simple test that does some signing and verifying.
is there anything we can do to optimize this?
or is the only way for that to move away from a pure PHP implementation?
1.0 branch
$ phpunit tests/SignVerifyMessageTest.php
Time: 5.76 seconds, Memory: 4.25Mb
$ hhvmunit tests/SignVerifyMessageTest.php
Time: 4.72 seconds, Memory 7.50Mb
master
$ phpunit tests/SignVerifyMessageTest.php
Time: 30.29 seconds, Memory: 4.25Mb
$ hhvmunit tests/SignVerifyMessageTest.php
Time: 15.61 seconds, Memory 7.59Mb
PS. hhvm different on 1.0 branch is relatively small because it has more bootstrap time than PHP
The text was updated successfully, but these errors were encountered: