latest version of phpecc made the performance HORIBLE
Opened this issue · 10 comments
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
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
Yeah totally agree.
I've been thinking about what we might be able to do there. The problem
is multiplication uses this conditional swap, but there's a bunch of
places where scalar multiplication is only done with safe values (like r
= $G->mul($randomK)->getX()).
I've been dicking around with secp256k1, I can put what I have up for
you, literally only secp256k1_start/stop works, secp256k1_ecdsa_verify
was the next most likely thing to try since it's verify-only (no writing
to references) but it borks.
It's testing my abilities of dealing with C types and their conversions
:P I might go back to SWIG since I didn't get that one compiled.
On 23/02/15 19:07, Ruben de Vries wrote:
I know it was because of the side channel attack fix and that was for
good reason, 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
—
Reply to this email directly or view it on GitHub
#66.
Thomas Kerin
My PGP key can be found here
http://pgp.mit.edu/pks/lookup?op=get&search=0x3F0D2F83A2966155
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1
iQJ8BAEBCgBmBQJU63xVXxSAAAAAAC4AKGlzc3Vlci1mcHJAbm90YXRpb25zLm9w
ZW5wZ3AuZmlmdGhob3JzZW1hbi5uZXQ2MzI1MzM4QjJGOTU5OEUzREMzQzc0MzAz
RjBEMkY4M0EyOTY2MTU1AAoJED8NL4OilmFVPUgP/j2cFux7KbjLvAGs0Q1f4K2b
txK8w0qhprhync7Z5F0gA0qZHCKRFWVVWxYzxSEM5o5skKDN3C6Yx41ZNQq3LE01
jrb04O2EQYjkmfnd3XdSqlMoQMtGGm+xixD+OUUfsilC7kWt2W25miJ7I8DRWB1i
hQI8drtrY5hb7Vmv1WawbFAupE4dv0RoI1mKiTdvM5sRrT5XNkBmW78GP/VXtFqQ
1Er/e8tgxCFT5nRO8FY2olZAdbRkz/mdN/vp+LNfsYlFI0rnRXD6EoQiV+O5Sp2k
fbjvXrENKWJQ/TQVbWDhWQhIWYt2jWgTSvb4k1ITxTRowhA3f0O2/ySIVBuI5w37
0m51tCHTMIFLWIsB6uQeKZ0NXpv2whgYGSSygYQlSfFhsbhZ/DEN5d5uakaRSIpa
7hEhU/hFqpQbNPT7+hiWH8lt8wpZk5CyaTlWOwiWLZPSdfNlOMt+blqKki80e74H
EWDXLn1735R/WNtP/r4q5JMWl8jBqN2fqpknK2mNH3y2MxjbhPDwbc6/Cd3cYkEF
mcuOMGPFeIQbrFOvpnaTSJqfhOVXqxK1BHJ5amciDjorqTyBs2BDt9SbBTpRp8h7
FoMn1dASSRBXB3NrWS3ih1YUTjCEzWwRsjdtwmjsg1lH3NsvHHxtsndhvQFvnPv/
MDFmpkK9I89WFzp+E76C
=UVKG
-----END PGP SIGNATURE-----
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.
secp256k1_ec_pubkey_create was missing secp256k1_start(SECP256K1_START_SIGN); which instead of throwing errors it will give werid ass random return values, keep that in mind if you run into the same weird shit ;-)
also when it returns a compressed $pubKey you have to substr it with the $pubKeyLen, we should move that into C I guess, but using substr for now does the trick.
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 apt or probably pear since that avoid having to deal with windows and mac, but at least I will want to make it a 1 liner to install
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 php-secp256k1 installable from the APT repo. PEAR is dead, composer is the new black for PHP libraries.
@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..