Hello Johannes. I wanted to make clear the reason why I think this is better as a php extension rather than done in pure php.
The reason is in the unmasking that is required by RFC6455 websocket protocol specification. This unmasking takes too long in pure php and is greatly sped up in C. Using a pure PHP solution, I can unmask and process 25M binary file on average in 20 seconds while unmasking that same 25M binary file in C takes only a second or 2. This is a problem I experienced when trying to create a websockets app that handles uploading large files (binary frames) over websockets. It was taking upwards of 20 minutes to process a 1GB file. However, that same 1GB file can was uploaded, unmasked, and saved to disk in approx 2 minutes using my extension. So, while a pure php solution is fine for handling things like a chat server with messages less than 8K, it doesn't scale well when messages get big. Also, thank you for your critique. I will modify and commit. -- Ray Perea Snap Web Systems Like us on Facebook Follow us on Twitter On Dec 27, 2012, at 2:07 PM, Johannes Schlüter <[email protected]> wrote: > Hi, > > On Thu, 2012-12-27 at 09:31 -0600, Ray Perea wrote: >> Hello. My name is Ray Perea of Snap Web Systems. (http://www.snapws.com) >> I am writing this list because I would like to publish my PHP extension in >> PECL >> >> The extension introduces some functions for reading from and writing to >> websockets (RFC6455) >> >> These functions are: >> ws_send() - Send data to a websocket connection >> ws_read() - Read data from a websocket connection (Can read binary frames >> and files) >> ws_close() - Sends a close frame to the websocket client >> >> I have published the source code at sourceforge: >> https://sourceforge.net/p/phpwebsockets >> >> Please have a look and see if you would like me to publish my extension in >> PECL. >> Thanks in advance for your consideration. > > I took a quick look on the code and found some common flaws I see in > proposals. So instead of listing them here I started a wiki page: > https://wiki.php.net/internals/reciew_comments please have a look at the > comments there. > > Some additional comments on the code: > > * It would be great if you would follow the PHP CODING_STANDARDS > (see that file in php-src) for my mind it is simpler to read ;-) > * For php_websockets.h please check the comments on the wiki, > especially avoid includes like #include <stdint.h> > * E_ERROR is a very bad error code. We try to limit its usage to > cases where we can't recover at all. This will disable most ways > for the user to handle errors. > * In ws_read there is a strange code part > if (zend_is_callable()) { > } > is there any reason for that? - I assume you want to extend that > for a callback? If yes please look at the "f" modifier for > zend_parse_parameters() and the family of > zend_call_method[_with[1-4]param[s]] functions/macros from > zend_interfaces.h > * Overall the code is quite weak on error handling, especially > stream operations are often not checked for an error but run > into undefined behaviour in case of errors > * In WebsocketsSendClose() you're passing a length for the reason > but are completely ignoring it but call strlen() on the message. > Sometimes you actually calculate it two times - once before > calling, once inside the function. The suggested way is to use > sizeof() on the constant strings and pass that through. > * In different functions you are calling TSRMLS_FETCH() even > though you don't need the TSRM context. In ZTS mode this > operation is quite expensive. When possible (and needed) you > should pass the TSRMLS parameters through ... TSRMLS_FETCH() > should be a lat resort in cases were you can't pass it through > (i.e. callbacks to TSRM-unaware things) > * Some type usage might also lead to errors, i.e. > php_stream_read() returns size_t but you're assigning to a > uint32_t which might cause issues on systems where size_t is 64 > bit (even though it's unlikely a single read would return a few > gigs of data ;-)) > > Looks like a long list, but all minor things in fact :-) > > Going away from those code-centric things: I wonder why this is > implemented using custom functions. Won't it be nicer to implement this > as custom stream type, or maybe even a stream filter? All it seems to do > is enrich stream messages ... > > I also wonder what benefits you expect from implementing this in C over > a pur PHP approach. I expect most time to be spent in streams > themselves ... I always suggest to use pure PHP things as they are > simpler to write, maintain, deploy, monitor and debug. > > johannes > > > >
