crc32() improvements

php.internals

Alexander Valyalkin

22 years ago
Here is improved version of crc32() function. Features: 1) Automatic initialization of crc32tab[] at first call. So, the file crc32.h with definition of this tab is not nessesary any more now. 2) Speed is improved on large amount of data. 3) Less source size. Current verison has near 6.5Kb length (including crc32.h). My version has only 2.5Kb length. Below I provided a test (just copy->compile->test) and unified diff of /ext/standard/crc32.c =============cut============ #include <stdio.h> /****************************************************/ /* test data structure */ typedef struct { char *p; /* pointer to a data */ size_t len; /* length of a data */ } my_str; /* set of test data */ my_str s[] = { /* test strings from /ext/standard/tests/strings/crc32.phpt */ {"foo", 3}, {"bar", 3}, {"baz", 3}, {"grldsajkopallkjasd", 18}, /* my test strings */ {"", 0}, /* empty string */ {"\0", 1}, /* one null char */ {"a", 1}, /* one char */ {"ab", 2}, /* two chars */ {"a\0\0b", 4}, /* four chars */ {NULL, 0} }; /****************************************************/ /* faked PHP definitions */ #define PHP_NAMED_FUNCTION(a) void a(char *str1, size_t len1) #define zend_parse_parameters(foo, bar, s1, len) (*(s1) = str1, *(len) = len1, 1) #define TSRMLS_CC #define FAILURE 0 #define RETVAL_LONG(c) do {printf("str=[%s], len=[%d], crc32(unsigned)=[%lu]\n", str1, len1, (c)); return;} while (0) /****************************************************/ /* my crc32 function */ PHP_NAMED_FUNCTION(php_if_crc32) { static unsigned long int crc32tab[256], not_init = 1; if (not_init) { /* init crc32 table */ register unsigned long int tmp, i, j, flag_c; for (i = 0; i < 256; i++) { tmp = i; j = 8; do { if (tmp & 1) { tmp >>= 1; tmp ^= 0xEDB88320; /* CRC32 (CCITT-32) poly g(x) = 1 0000 0100 1100 0001 0001 1101 1011 0111 */ } else tmp >>= 1; j--; } while (j); crc32tab[i] = tmp; } not_init = 0; } /* crc32 calculations */ char *str; int len; if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "s", &str, &len) == FAILURE || len < 0) return; if (len == 0) RETVAL_LONG(0); register unsigned long int crc = ~0ul; register unsigned char *p = (unsigned char *) str; register int n = len; do { crc = (crc >> 8) ^ crc32tab[(crc ^ *p++) & 0xff]; n--; } while (n); RETVAL_LONG(~crc); } /***************************************************/ int main(int argc,char *argv[]) { int i = 0; while (s[i].p != NULL) { php_if_crc32(s[i].p, s[i].len); i++; } return 0; } =============cut============ the unified diff of crc32.c: =============cut============ --- crc32.c Tue Dec 31 19:35:26 2002 +++ crc32_new.c Sat Jun 12 19:01:56 2004 @@ -20,24 +20,41 @@ #include "php.h" #include "basic_functions.h" -#include "crc32.h" /* {{{ proto string crc32(string str) Calculate the crc32 polynomial of a string */ PHP_NAMED_FUNCTION(php_if_crc32) { - unsigned int crc = ~0; - char *p; - int len, nr; - - if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "s", &p, &nr) == FAILURE) { - return; + static unsigned long int crc32tab[256], not_init = 1; + if (not_init) { + /* init crc32 table */ + register unsigned long int tmp, i, j, flag_c; + for (i = 0; i < 256; i++) { + tmp = i; + j = 8; + do { + if (tmp & 1) { + tmp >>= 1; + tmp ^= 0xEDB88320; /* CRC32 (CCITT-32) poly g(x) = 1 0000 0100 1100 0001 0001 1101 1011 0111 */ + } else tmp >>= 1; + j--; + } while (j); + crc32tab[i] = tmp; } - - len = 0 ; - for (len += nr; nr--; ++p) { - CRC32(crc, *p); + not_init = 0; } + /* crc32 calculations */ + char *str; + int len; + if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "s", &str, &len) == FAILURE || len < 0) return; + if (len == 0) RETVAL_LONG(0); + register unsigned long int crc = ~0ul; + register unsigned char *p = (unsigned char *) str; + register int n = len; + do { + crc = (crc >> 8) ^ crc32tab[(crc ^ *p++) & 0xff]; + n--; + } while (n); RETVAL_LONG(~crc); } /* }}} */ =============cut============
-- Using Opera's revolutionary e-mail client: http://www.opera.com/m2/

Ard Biesheuvel

22 years ago
Alexander Valyalkin wrote:
> 1) Automatic initialization of crc32tab[] at first call. > So, the file crc32.h with definition of this tab is not > nessesary any more now.
First of all, crc32tab is no longer in the .text segment, so it will not be shared between forked processes, taking more memory space than necessary. Each process will have to initialise it as well, so the init loop will run more than once. Secondly, compiler optimisation is always more effective working on data known to be constant. So removing 'const' from cr32tab is likely to produce slower code. Also, you used long int instead of int, which means your code will break on 64-bit platforms.
> 2) Speed is improved on large amount of data.
I seriously doubt that, as the tight main loop of the function didn't change that much. Using 'register' won't help you here (Windows cl.exe officialy ignores it altogether, GCC is smart enough to figure it out for itself)
> 3) Less source size. Current verison has near 6.5Kb length > (including crc32.h). My version has only 2.5Kb length.
Yeah, you managed that. Well done.
-- Ard

Alexander Valyalkin

22 years ago
On Sun, 13 Jun 2004 11:24:12 +0200, Ard Biesheuvel <abies@php.net> wrote:
> > First of all, crc32tab is no longer in the .text segment, so it will not > be shared between forked processes, taking more memory space than > necessary. Each process will have to initialise it as well, so the init > loop will run more than once. > > Secondly, compiler optimisation is always more effective working on data > known to be constant. So removing 'const' from cr32tab is likely to > produce slower code. > > Also, you used long int instead of int, which means your code will break > on 64-bit platforms. > >> 2) Speed is improved on large amount of data. > > I seriously doubt that, as the tight main loop of the function didn't > change that much. Using 'register' won't help you here (Windows cl.exe > officialy ignores it altogether, GCC is smart enough to figure it out > for itself) >
Thank you for good explanation and comments. Now I understood that the current crc32 implementation is better than mine. But it consists some ugly bugs (read my comments): PHP_NAMED_FUNCTION(php_if_crc32) { unsigned int crc = ~0; char *p; int len, nr; /*!!! is special sense in [len] var here? Remove it! */ if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "s", &p, &nr) == FAILURE) { return; } /* !!! there is no error check nr < 0 */ len = 0 ; /* !!! remove it! */ for (len += nr; nr--; ++p) { CRC32(crc, *p); } RETVAL_LONG(~crc); } Below is corrected function with speed improvement in main cycle (up to 40% on my compiler) =========cut======== PHP_NAMED_FUNCTION(php_if_crc32) { unsigned int crc = ~0; unsigned char *p; int nr; if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "s", (char **)&p, &nr) == FAILURE || nr < 0) return; if (nr == 0) RETVAL_LONG(0); do { crc = (crc >> 8) ^ crc32tab[(unsigned char)crc ^ *p++]; } while (--nr); RETVAL_LONG(~crc); } =========cut========= And do not forget to remove CRC32() definition in crc32.h
-- Using Opera's revolutionary e-mail client: http://www.opera.com/m2/

Derick Rethans

22 years ago
On Mon, 14 Jun 2004, Alexander Valyalkin wrote:
> Thank you for good explanation and comments. > Now I understood that the current crc32 implementation is better than mine. > But it consists some ugly bugs (read my comments): > PHP_NAMED_FUNCTION(php_if_crc32) > { > unsigned int crc = ~0; > char *p; > int len, nr; /*!!! is special sense in [len] var here? Remove it! */ > > if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "s", &p, &nr) == > FAILURE) { > return; > } > /* !!! there is no error check nr < 0 */
Of course not, that's pointless as a string can never have a negative length.
> len = 0 ; /* !!! remove it! */
Why? It's used one line below and you HAVE to initalize a variable.
> for (len += nr; nr--; ++p) { > CRC32(crc, *p); > } > RETVAL_LONG(~crc); > } > > Below is corrected function with speed improvement in main cycle
It also violates our coding standards BIG time. And there is no reason to expand that macro at all, nor is your code regarding the || nr <0 needed. Please STOP wasting our time with those silly improvements which aren't improvements. You're wasting our and your own time. Derick k

Alexander Valyalkin

22 years ago
On Mon, 14 Jun 2004 11:00:33 +0200 (CEST), Derick Rethans <derick@php.net> wrote:
>> if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "s", &p, &nr) >> == >> FAILURE) { >> return; >> } >> /* !!! there is no error check nr < 0 */ > > Of course not, that's pointless as a string can never have a > negative length. >
:) Are you sure? I'm not. Look on declaration of [nr] variable: int nr; And answer, please, which value will be assigned to nr, if length of a string will be greater than 0x7fffffff on 32-bit architecture?
>> len = 0 ; /* !!! remove it! */ > > Why? It's used one line below and you HAVE to initalize a variable. >
I can't find any sense of the [len] variable. Can you?
>> Below is corrected function with speed improvement in main cycle > > It also violates our coding standards BIG time.
It is only idle talk. Can you provide any string from my code which violates your "coding standards"? By the way, your "coding standards" violates C standards on type of string (and any other byte arrays) length. Why are you use int instead of size_t?: typedef union _zvalue_value { long lval; /* long value */ double dval; /* double value */ struct { char *val; int len; /* !!!!!!!!!!!!! why int, not size_t ????? */ } str; HashTable *ht; /* hash table value */ zend_object obj; } zvalue_value;
> And there is no reason to expand that macro at all
Is significant speed improvement silly reason for you?
-- Using Opera's revolutionary e-mail client: http://www.opera.com/m2/

Christian Schneider

22 years ago
Alexander Valyalkin wrote:
> It is only idle talk. Can you provide any string from my code which > violates your "coding standards"?
Calm down. As I said before (obviously not clearly enough, I was hoping one of the 'project managers' would do that for me ;-)) you are missing the point why people reject your code. Facts are: a) People here are not interested in rewriting already working functions which have a REASONABLE speed. b) Read CODNIG_STANDARDS from the distribution, your code doesn't follow the indentation style etc. defined there for the whole project. This is necessary to allow collaboration. Individual preferences or standards defined elsewhere are of minor importance. c) Fix bugs from bugs.php.net marked as OPEN. If you do this then I'm positive that you'll be able to contribute to PHP. Regards, - Chris

Mike Benoit

22 years ago
On Mon, 2004-06-14 at 12:48 +0200, Christian Schneider wrote:
> Alexander Valyalkin wrote: > > It is only idle talk. Can you provide any string from my code which > > violates your "coding standards"? > > Calm down. As I said before (obviously not clearly enough, I was hoping > one of the 'project managers' would do that for me ;-)) you are missing > the point why people reject your code. > > Facts are: > a) People here are not interested in rewriting already working functions > which have a REASONABLE speed.
Why would people not be interested in speed improvements? Especially when they involve functions (crc32 and serialize) that do the heavy lifting in many caching implementations, such as Smarty and PEAR:: Cache_Lite. A 40% improvement, if true, would be pretty damn nice to say the least. I sure could use performance increases in this area as caching is key to my ACL class (http://phpgacl.sourceforge.net/) being usable on high volume sites.
-- Mike Benoit <ipso@snappymail.ca>

Ard Biesheuvel

22 years ago
Alexander Valyalkin wrote:
> :) Are you sure? I'm not. Look on declaration of [nr] variable: > int nr; > And answer, please, which value will be assigned to nr, if length of > a string will be greater than 0x7fffffff on 32-bit architecture?
The funny thing is that in this case, it doesn't matter if 'nr' is signed or unsigned, as it is decremented by 1 until it becomes zero. This will also work as expected for negative values. So the function will be correct for strings of up to 4GB.
> It is only idle talk. Can you provide any string from my code which > violates your "coding standards"?
All of the one-liners, basically. You should use more braces & whitespace.
> By the way, your "coding standards" violates C standards on type > of string (and any other byte arrays) length. Why are you use int > instead of size_t?:
Why should PHP strings conform to C string standards ?
>> And there is no reason to expand that macro at all > > Is significant speed improvement silly reason for you? >
You mean at compile-time ? Macros are expanded by the pre-processor, so the expanded code that is processed by the compiler is *identical* to your inlined code. So no speed increase here.
-- Ard

Alexander Valyalkin

22 years ago
On Mon, 14 Jun 2004 13:49:16 +0200, Ard Biesheuvel <abies@php.net> wrote:
> Alexander Valyalkin wrote: >> :) Are you sure? I'm not. Look on declaration of [nr] variable: >> int nr; >> And answer, please, which value will be assigned to nr, if length of >> a string will be greater than 0x7fffffff on 32-bit architecture? > > The funny thing is that in this case, it doesn't matter if 'nr' is > signed or unsigned, as it is decremented by 1 until it becomes zero. > This will also work as expected for negative values. So the function > will be correct for strings of up to 4GB. >
Yes, it is very funny :) But only in this case. How about another places in the sources?
>> It is only idle talk. Can you provide any string from my code which >> violates your "coding standards"? > > All of the one-liners, basically. You should use more braces & > whitespace. >
Well, this code is better than old one: =========cut========== PHP_NAMED_FUNCTION(php_if_crc32) { unsigned int crc = ~0; unsigned char *p; int nr; if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "s", (char **)&p, &nr) == FAILURE) { return; } if (nr) { do { crc = (crc >> 8) ^ crc32tab[(unsigned char)crc ^ *p++]; } while (--nr); } RETVAL_LONG(~crc); } =========cut==========
>>> And there is no reason to expand that macro at all >> Is significant speed improvement silly reason for you? > > You mean at compile-time ?
No, I meant the real speed improvement by 40% at run-time.
> Macros are expanded by the pre-processor, so the expanded code that is > processed by the compiler is *identical* to your inlined code. So no > speed increase here. >
Compare the macros with my inline code: #define CRC32(crc, ch) (crc = (crc >> 8) ^ crc32tab[(crc ^ (ch)) & 0xff]) crc = (crc >> 8) ^ crc32tab[(unsigned char)crc ^ *p++]; Are they identical?
-- Using Opera's revolutionary e-mail client: http://www.opera.com/m2/

Curt Zirzow

22 years ago
* Thus wrote Alexander Valyalkin (valyala@tut.by):
> Here is improved version of crc32() function. > Features: > 1) Automatic initialization of crc32tab[] at first call. > So, the file crc32.h with definition of this tab is not > nessesary any more now.
I'm not sure what advantage this is really giving you being that gcc will optimize where the data segment should be, vs putting the data directly into php's memory.
> 2) Speed is improved on large amount of data.
By using a registered int with a do while() statement vs a foreach() statement, I'd like to see some benchmarks against this.
> 3) Less source size. Current verison has near 6.5Kb length > (including crc32.h). My version has only 2.5Kb length.
Source size is futile.
> > Below I provided a test (just copy->compile->test) > and unified diff of /ext/standard/crc32.c > ... > > /****************************************************/ > /* my crc32 function */ > PHP_NAMED_FUNCTION(php_if_crc32) > { > static unsigned long int crc32tab[256], not_init = 1; > if (not_init) { > /* init crc32 table */ > register unsigned long int tmp, i, j, flag_c; > for (i = 0; i < 256; i++) { > tmp = i; > j = 8; > do { > if (tmp & 1) { > tmp >>= 1; > tmp ^= 0xEDB88320; /* CRC32 > (CCITT-32) poly g(x) = 1 0000 0100 > 1100 0001 0001 1101 1011 0111 */ > } else tmp >>= 1; > j--; > } while (j); > crc32tab[i] = tmp; > } > not_init = 0; > } > /* crc32 calculations */ > char *str; > int len; > if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "s", &str, &len) > == FAILURE || len < 0) return;
Have you read CODING_STANDARDS yet? Curt
-- I was working on a flat tax proposal, and I accidentally proved there's no god.

Alexander Valyalkin

22 years ago
On Sun, 13 Jun 2004 23:10:04 +0000, Curt Zirzow <curt@php.net> wrote:
> * Thus wrote Alexander Valyalkin (valyala@tut.by): >> Here is improved version of crc32() function. >> Features: >> 1) Automatic initialization of crc32tab[] at first call. >> So, the file crc32.h with definition of this tab is not >> nessesary any more now. > > I'm not sure what advantage this is really giving you being that > gcc will optimize where the data segment should be, vs putting the > data directly into php's memory. >
Yes, it's my fault.
> >> 2) Speed is improved on large amount of data. > > By using a registered int with a do while() statement vs a > foreach() statement, I'd like to see some benchmarks against this. >
Well. Try something like this: ===================cut=================== #include <stdio.h> #include <time.h> /* for use _rdtsc() */ #define N 10 * 1000 * 1000 int main(int argc,char *argv[]) { unsigned long long int t; unsigned int i; unsigned int crc = ~0; t = _rdtsc(); for (i = 0; i < N; i++) { crc ^= i; } printf("speed of for (i++) loop: %llu\n", _rdtsc() - t); i = N; t = _rdtsc(); for ( ; i--; ) { crc ^= i; } printf("speed of for (i--) loop: %llu\n", _rdtsc() - t); i = N; t = _rdtsc(); if (i > 0) do { crc ^=i; } while (--i); printf("speed of do while loop: %llu\n", _rdtsc() - t); return 0; } ===================cut===================
-- Using Opera's revolutionary e-mail client: http://www.opera.com/m2/