Here is improved version of crc32()
function.
Features:
- Automatic initialization of crc32tab[] at first call.
So, the file crc32.h with definition of this tab is not
nessesary any more now. - Speed is improved on large amount of data.
- 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/
Alexander Valyalkin wrote:
- 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.
- 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)
- 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
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.
- 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/
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
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/
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
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
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 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/
- Thus wrote Alexander Valyalkin (valyala@tut.by):
Here is improved version of
crc32()
function.
Features:
- 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.
- 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.
- 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.
- Thus wrote Alexander Valyalkin (valyala@tut.by):
Here is improved version of
crc32()
function.
Features:
- 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.
- 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/