I ran across this today and I simply could not believe what I was seeing. It's right up there on Daily WTF - or should be, anyway. First, a little set-up...
This code is part of an incoming exchange data decoder. The Exchange will send messages on udp multicast, and it's up to us to grab them, decode them, place them in out message formats, and pass them on to all waiting listeners. What's important to realize is that these decoders are supposed to be efficient and fast. After all, they are decoding hundreds of thousands of messages a second. It's a lot of data.
So... the exchange dictates it's message format, and as in the olden days of the mainframe, most all the data is in fixed-length ASCII records. Specifically, the integer for the size of an order might be 8 characters and look like this:
120.....
where the '.'s are spaces. Eight characters total, in ASCII format for the number 120. Simple. Not very efficient, but simple.
Since these fixed-length records will be end-to-end, there's no terminating NULL characters to make it easier to parse - you have to know what you're looking for. Well, the code I saw started with this method:
/*
* Convert unsafe char array string to int.
*/
inline int uatoi(const char *nptr, size_t l)
{
int rv = 0;
char *nullTermStr = cmalloc2(l + 1);
if (nullTermStr == NULL) {
errno = ENOMEM;
return INT_MAX;
}
memcpy(nullTermStr, nptr, l);
rv = atoi(nullTermStr);
free(nullTermStr);
return rv;
}
where:
/*
* Malloc2 that returns a char pointer.
*/
inline char *cmalloc2(size_t size)
{
return (char *) malloc2(size);
}
and:
/*
* Malloc memory and initialize it to zero
*/
inline void *malloc2(size_t size)
{
if (size < 0)
return NULL;
void *ptr = calloc(size, 1);
return ptr;
}
OK... this is really quite stunning. You want to parse a (char *) and so you duplicate it, by calling a useless method and then calloc with a repeat count of 1, parse it and then free the memory. And this is fast? For upwards of 10 fields a message, hundreds of thousands of times a second?
When I re-wrote the functionality I was decidedly simpler:
/*
* Convert unsafe char array string to int.
*/
inline int uatoi( char *nptr, size_t width )
{
char hold = nptr[width];
nptr[width] = '\0';
int retval = atoi(nptr);
nptr[width] = hold;
return retval;
}
Sure, I had to "loose" the const in the signature because I was modifying the data as I parsed it, but hey - it's a message from some data source - that's OK. It's also the same "logic" of using atoi() in the decoding. But now I'm not calling something to create some memory and then copying it, and destroying it. I can't believe they didn't look at all this before. It's incredible!
I know there comes a time when people don't look at the code anymore and just think the whole thing is too complicated... but really... guys... let's try a little harder. This is a horrible performance penalty for parsing. It should have been looked at long ago.
I guess I'm the one that decided to really look at it.
