I did a little profiling and optimization of the code, particularly for writing messages. In my usage I was able to yield approximately a 280% increase in speed with these changes.
I hope to look into this more, but here are some ideas for further optimization...
I noticed AMQPConnection->write() appears to be called approximately 3 times for every message that is written. Is this necessary, or could we reduce this to a 1:1 ratio and write one batch request for each message?
AMQPWriter::chrbytesplit() is still slow. Is it possible to pack() a 64-bit integer instead of using this function?
- performance.patch 9.71KB
Comment #1
Posted on Mar 13, 2009 by Happy DogI was going through and performing optimizations of the library and came up with many of the same changes in the provided patch. Removing the hexdump call on every write, removing the count() from flushbits and changing write_long() to use a simple pack() are the most important improvements.
As far as write_longlong() goes, I got a significant speed improvement with this, which I believe should be a portable change:
public function write_longlong($n) { $this->flushbits(); $c = 4294967296; // 2^64 $b = bcmod($n, $c); $a = bcdiv($n, $c); $this->out .= pack('N', $a); $this->out .= pack('N', $b); }
Comment #2
Posted on Mar 13, 2009 by Helpful Elephant@evendir Thanks for sharing. I just tested that function and I got a significant performance increase as well. Nice work!
I've attached an updated patch that also includes some other optimizations I made.
- performance.patch.2 10.7KB
Comment #3
Posted on Mar 14, 2009 by Happy LionI am concerned a bit, now new write_longlong() would work on 32-bit systems. In particular the following line maybe be treated as 'signed'. BTW it is 2^32, not 2^64 :)
$c = 4294967296; // 2^64
Could somebody please verify that this function works as expected on both 32 and 64 bit systems? Maybe we need to make a simple unit test for things like this?
Comment #4
Posted on Mar 14, 2009 by Helpful Elephant@krokodil I agree, it's important for these things to be tested on both 32 & 64 bit systems. I'm using it on a 32 bit system and it appears to work without problem.
One thing I failed to adjust in the updated patch could have both a performance and portability impact: $c = 4294967296; should be $c = '4294967296';. Aside from the fact that bcmod() and bcdiv() expect string inputs (and perform better when inputs are such), I think declaring $c as a string would circumvent PHP's funky internal integer datatype.
Comment #5
Posted on Mar 14, 2009 by Happy LionI have added unit tests to the project. To run from command line:
cd tests php all_tests.php
I have tested and current code base passes all tests on both 32 and 64 bit system. Last patch provided here fails unit tests:
deco /Users/lord/src/php-amqplib/tests> php all_tests.php PHP-AMQP tests (64bit) 1) Equal expectation fails at character 4 with [994284929023] and [994294967296] at [/Users/lord/src/php- amqplib/tests/wire_format_test.php line 42] in testWriteLong in AMQPWriterTests in wire_format_test.php FAILURES!!!
We need a new patch which passes all tests.
P.S. Feel free to add more tests cases.
Comment #6
Posted on Oct 3, 2009 by Quick RhinoThis version of the patch passes the tests on macos darwin 32bit and Centos-Linux 64bit.
You can also find it at http://github.com/bkw/php-amqp/commits/issue3
I've also added some more tests (moretests.patch)
regards, bkw
- performance.patch.3 23.93KB
- moretests.patch 3.11KB
Comment #7
Posted on Mar 21, 2010 by Happy LionLyolik please review these bugs.
Status: New
Labels:
Type-Defect
Priority-Medium